Add support for Content Headers to BasicHtmlWebResponseObject and HtmlWebResponseObject#4494
Add support for Content Headers to BasicHtmlWebResponseObject and HtmlWebResponseObject#4494TravisEz13 merged 4 commits intoPowerShell:masterPowerShell/PowerShell:masterfrom markekraus:GetRawContentHeadermarkekraus/PowerShell:GetRawContentHeaderCopy head branch name to clipboard
Conversation
|
@markekraus, |
|
@markekraus Could you please add tests? |
|
@iSazonov Tests Added. I wasn't exactly sure what tests to add. There is already significant validation being done on the |
|
@iSazonov doh... corrected. |
|
I agree with @iSazonov . The tests in the issue look like a good starting point. |
|
@TravisEz13 Thanks. I added the 2 test that make sense. The other tests from the issue won't ever work unless CoreFX drastically changes their approach in |
73d19ed to
823dd03
Compare
|
@TravisEz13 @SteveL-MSFT Rebased PR on master to add test fixes from #4512. The new tests are passing. Let me know if I need to make any other changes. |
823dd03 to
75b6f54
Compare
| }; | ||
|
|
||
| foreach (var entry in response.Headers) | ||
| foreach(var headerCollection in headerCollections) |
There was a problem hiding this comment.
Minor point, insert a space after 'foreach' here and 'if' below to match the rest of the code.
| { | ||
| headers[entry.Key] = entry.Value; | ||
| } | ||
| if(BaseResponse.Content != null){ |
There was a problem hiding this comment.
Minor note: place '{' on a new line.
There was a problem hiding this comment.
@dantraMSFT corrected, along with space between if and (.
| #endregion charset encoding tests | ||
|
|
||
| It "Verifies Invoke-WebRequest includes Content headers in Headers property" { | ||
| $command = "Invoke-WebRequest -Uri 'http://httpbin.org/get'" |
There was a problem hiding this comment.
Use HTTPListener here instead of http://httpbin.org/get.
The tests under Context "BasicHtmlWebResponseObject Encoding tests" illustrate using 'test=response&output=$otuput' to pass HTML you want to get in the response. You should consider extending httplistener's 'response' option to allow additional options, such as specifying a content-type header value or other headers to support your testing. let me know if you need any help with it.
There was a problem hiding this comment.
@dantraMSFT Thanks. I will work on this and submit more and better tests with your suggestions.
|
|
||
| #endregion charset encoding tests | ||
|
|
||
| It "Verifies Invoke-WebRequest includes Content headers in Headers property" { |
There was a problem hiding this comment.
I think these two tests need to be either be extended or additional tests added.
1: I don't believe verifying the presence of the header is sufficient; I believe the test should verify the value(s) as well.
2: You added logic to support headers with multiple values so you need to test that case.
|
@dantraMSFT I have added the requested test changes and httplistener now supports a |
| $statusCode = $queryItems["statuscode"] | ||
| $contentType = $queryItems["contenttype"] | ||
| $output = $queryItems["output"] | ||
| if ($queryItems['headers']) |
There was a problem hiding this comment.
It would be helpful to add a comment here that illustrates how the 'headers' value should be formatted; especially since this is the only documentation we have for the listener. :)
At a minimum, show the json structure. Even better would be to show a code sample similar to what you used in your tests.
There was a problem hiding this comment.
@dantraMSFT added. comments with examples
|
LGTM |
|
@markekraus Can you trigger of feature tests? They seems to have failed last time on Travis CI. |
42f5aca to
07d7b3d
Compare
|
@adityapatwardhan feature tag added. |
|
@adityapatwardhan all tests passed this time. (last time an unrelated test hung accessing a 3rd party website). |
|
LGTM. Thanks for great work! |
Fixes #4467
System.Net.Http.HttpResponseMessagehas split the Content headers from the Response headers. This split appears to be intentional and permanent. This split results in theContent-TypeandContent-Lengthheaders to be missing from theWebResponseObject.Headersdictionary and from theRawContentproperty ofBasicHtmlWebResponseObjectandHtmlWebResponseObject.This adds the Content headers back to those locations to make it similar to 5.1.
This also adds support to
RawContentfor multiple response headers with the same field name.System.Net.Http.Headers.HttpContentHeadersandSystem.Net.Http.Headers.HttpResponseHeadersimplement values as arrays.