Replace httpbin.org/encoding Tests with WebListener #4869
Conversation
markekraus
requested review from
TravisEz13,
adityapatwardhan and
daxian-dbw
as code owners
|
@markekraus, |
There was a problem hiding this comment.
Should it be "text/html"?
There was a problem hiding this comment.
Doh... fixed. thanks
There was a problem hiding this comment.
Please add newline at the end.
|
Tests failed due to a "spelling" error: https://travis-ci.org/PowerShell/PowerShell/jobs/277524298#L4389 |
markekraus
force-pushed
the
HttpBinEncodingUtf8
branch
from
bae9800 to
acd99c0
Compare
There was a problem hiding this comment.
Is this test sufficient? Do we really check Utf8 not Utf16/32 here?
Can we also protect from changing the encoding of this file? Does it make sense use `u{...} ?
The source file contains text in many languages. Does it make sense to ckeck multiple languages.
There was a problem hiding this comment.
I wasn't clear on what the original tests were trying to accomplish and I'm not proficient with encoding to know what is a good test or not. It was clear to me that they were definitely not sufficient for UTF8, so I tried to add something that would at least eliminate ASCII. After the response headers are available for Invoke-RestMethod, it would be possible to test the response encoding similar to the Invoke-WebRequest. But I'm definitely open to suggestion on how to improve these tests.
There was a problem hiding this comment.
On the source file. If we can narrow down a specific UTF8 character or string, We can switch to generating the string from the controller and remove the view. That way we do not need to worry about the encoding change. I'd need guidance on what to put there, though.
There was a problem hiding this comment.
I think this is an improvement over what we had before. If you think the tests can be further improved, perhaps an issue would be a better way to address the feedback.
There was a problem hiding this comment.
I think this is good. If my research on this is correct, It's no easy task to test a string for its encoding This at least include some UTf8 chars.
Opened #4890 for further improvements
There was a problem hiding this comment.
Thanks for open tracking Issue!
Closed.
|
@adityapatwardhan Can you update your review? |
markekraus
force-pushed
the
HttpBinEncodingUtf8
branch
from
31918b5 to
2728e3c
Compare
iSazonov
left a comment
iSazonov
left a comment
There was a problem hiding this comment.
@markekraus Thanks for your contribution!
5 participants
Invoke-RestMethodtest. I'm not sure what the test was originally meant to accomplish but there is not an equivalentInvoke-WebRequesttest and there are already other tests forInvoke-RestMethodwhich testtext/htmlresponses. httpbin.org/encoding/utf8 did not returntext/plainanyway. *shrugsreference #2504