Use LoopbackServer for HttpWebRequestTests #37451

ViktorHofer · 2019-05-05T16:06:09Z

Fixes https://github.com/dotnet/corefx/issues/22849
Fixes https://github.com/dotnet/corefx/issues/37430

Enabling a few tests which weren't running.

I left the synchronous tests as they were as I wasn't sure what the pattern for testing with the LoopBackServer synchronously is.

src/System.Net.Requests/tests/HttpWebRequestTest.cs

stephentoub

Thank you for working to move a bunch of these to loopback.

davidsh · 2019-05-28T13:56:10Z

@ViktorHofer Are you still working on this PR?

ViktorHofer · 2019-05-28T15:54:10Z

Yes I'm willing to finish this but need a recommendation here: #37451 (comment)

ViktorHofer · 2019-05-29T13:27:08Z

@stephentoub @davidsh for SSL I had to set request.ServerCertificateValidationCallback = (a, b, c, d) => true; so that the validation check for the local certificate is skipped. Is that right or am I expected to trust the certificate on my box?

davidsh · 2019-05-29T13:36:42Z

@stephentoub @davidsh for SSL I had to set request.ServerCertificateValidationCallback = (a, b, c, d) => true; so that the validation check for the local certificate is skipped. Is that right or am I expected to trust the certificate on my box?

We don't want tests to rely on machine configuration (like setting a test certificate to be trusted on a dev box). So, yes, use the callback you showed. There are some exceptions to this rule for some detailed TLS testing we do, but for these tests we just want to exercise TLS.

ViktorHofer · 2019-05-29T14:18:40Z

OK great, then this PR should be ready to go in. Please take another look. Thanks.

src/System.Net.Requests/tests/HttpWebRequestTest.cs

stephentoub · 2019-05-29T14:28:44Z

src/System.Net.Requests/tests/HttpWebRequestTest.cs

+                    string strContent = sr.ReadToEnd();
+                    Assert.True(strContent.Contains(RequestBody));
+                }
+            }, server => server.HandleRequestAsync(content: RequestBody), options);


This isn't right. The previous version was using an echo server, and by the client code checking that the response contained the request body, it was thus checking that the request body was correctly sent to the server (which then echo'd it). This change is making it so that the server simply ignores the request body and instead simply sends back the same content, without verifying that's actually what it received.

I agree with @stephentoub on this point. HttpWebRequest builds on HttpClient and we already have detailed tests for the HttpClient layer. So, we use these HttpWebRequest tests mainly to verify end-to-end functionality. And using an echo server helps satisfy that requirement.

src/System.Net.Requests/tests/HttpWebRequestTest.cs

stephentoub · 2019-05-29T14:31:16Z

src/System.Net.Requests/tests/HttpWebRequestTest.cs

-                Assert.True(headersString.Contains(headersPartialContent));
-            }
+                Assert.Equal(HeadersPartialContent, response.Headers[HttpResponseHeader.ContentType]);
+            }, server => server.HandleRequestAsync(headers: new HttpHeaderData[] { new HttpHeaderData("Content-Type", HeadersPartialContent) }), options);


Same echo server issue. It seems like we should add a server.Echo() method that functionally does the same thing the remote echo server was doing.

stephentoub

Generally looks good, a few nits, but two real issues. I suggest you revert the changes to the two problematic tests I commented on and get the rest of the changes merged. Then we can revisit needing a loopback server echo function.

ViktorHofer · 2019-05-29T23:06:22Z

I suggest you revert the changes to the two problematic tests I commented on and get the rest of the changes merged

I implemented the echo functionality inline in the two tests as I thought it would be less work. After playing around with the server I noticed that the connection.ReadToEndAsync call doesn't read until the end but only the chunked part that was transmitted at that point. Instead I'm now reading from the stream directly...

ViktorHofer · 2019-05-30T00:07:46Z

All the tests are failing on UWP: System.IO.IOException : Authentication failed because the remote party has closed the transport stream.. Am I missing something? The other failures are unrelated.

ViktorHofer · 2019-05-31T06:58:43Z

Ping @davidsh, see question above

davidsh · 2019-05-31T19:54:59Z

Ping @davidsh, see question above

I'm seeing errors in NETFX and UWP runs:

NETFX:

Unhandled Exception of Type System.ArgumentException
Message :
System.ArgumentException : The 'Content-Type' header must be modified using the appropriate property or method.
Parameter name: name

And the UWP failures as well:

All the tests are failing on UWP: System.IO.IOException : Authentication failed because the remote party has closed the transport stream.. Am I missing something? The other failures are unrelated.

I think the UWP failures are due to a limitation of trying to do loopback w/ TLS/SSL in the UWP environment. The tests worked fine using a remote server with TLS. But loopback in UWP is always problematic due to running under app container. I'm not sure what the exact problem is though without investigating it.

I don't think there is value in spending a lot of time on this since it is some kind of test bug/limitation with UWP. So, I would recommend you add this to those tests to skip it on UWP:

[SkipOnTargetFramework(TargetFrameworkMonikers.Uap, "Loopback server with TLS has problems on UWP")]

ViktorHofer · 2019-06-02T16:14:27Z

Thanks for the help @davidsh and @stephentoub. We should update the docs that mention the LoopbackServer at some point.

stephentoub · 2019-06-02T16:15:50Z

We should update the docs that mention the LoopbackServer at some point

Which docs?

ViktorHofer · 2019-06-02T16:18:01Z

https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/writing-tests.md#loopbackserver

* Use LoopbackServer for HttpWebRequestTests Commit migrated from dotnet/corefx@d57a1a2

ViktorHofer added the area-System.Net label May 5, 2019

ViktorHofer requested review from davidsh and stephentoub May 5, 2019 16:06

ViktorHofer self-assigned this May 5, 2019

ViktorHofer force-pushed the HttpWebRequestTestLoopback branch from da9fb56 to 9a2385c Compare May 5, 2019 21:53

stephentoub reviewed May 6, 2019

View reviewed changes

src/System.Net.Requests/tests/HttpWebRequestTest.cs Outdated Show resolved Hide resolved

stephentoub reviewed May 6, 2019

View reviewed changes

src/System.Net.Requests/tests/HttpWebRequestTest.cs Outdated Show resolved Hide resolved

stephentoub reviewed May 6, 2019

View reviewed changes

src/System.Net.Requests/tests/HttpWebRequestTest.cs Outdated Show resolved Hide resolved

stephentoub reviewed May 6, 2019

View reviewed changes

src/System.Net.Requests/tests/HttpWebRequestTest.cs Outdated Show resolved Hide resolved

stephentoub reviewed May 6, 2019

View reviewed changes

src/System.Net.Requests/tests/HttpWebRequestTest.cs Outdated Show resolved Hide resolved

stephentoub reviewed May 6, 2019

View reviewed changes

src/System.Net.Requests/tests/HttpWebRequestTest.cs Outdated Show resolved Hide resolved

stephentoub reviewed May 6, 2019

View reviewed changes

ViktorHofer mentioned this pull request May 6, 2019

[master] Update dependencies from dotnet/core-setup #37464

Merged

ViktorHofer added 2 commits May 29, 2019 14:13

Use LoopbackServer for HttpWebRequestTests

c9de86e

PR feedback, add ssl checks

c732b84

ViktorHofer requested a review from stephentoub May 29, 2019 13:26

ViktorHofer force-pushed the HttpWebRequestTestLoopback branch from 9a2385c to c732b84 Compare May 29, 2019 13:30

stephentoub reviewed May 29, 2019

View reviewed changes

src/System.Net.Requests/tests/HttpWebRequestTest.cs Outdated Show resolved Hide resolved

stephentoub reviewed May 29, 2019

View reviewed changes

src/System.Net.Requests/tests/HttpWebRequestTest.cs Show resolved Hide resolved

stephentoub reviewed May 29, 2019

View reviewed changes

src/System.Net.Requests/tests/HttpWebRequestTest.cs Show resolved Hide resolved

stephentoub reviewed May 29, 2019

View reviewed changes

src/System.Net.Requests/tests/HttpWebRequestTest.cs Outdated Show resolved Hide resolved

stephentoub reviewed May 29, 2019

View reviewed changes

stephentoub approved these changes May 29, 2019

View reviewed changes

Fix echo validation tests

aee09ee

Fix failing netfx and uwp tests

d0591f0

ViktorHofer merged commit d57a1a2 into dotnet:master Jun 2, 2019

ViktorHofer deleted the HttpWebRequestTestLoopback branch June 2, 2019 16:13

karelz added this to the 3.0 milestone Jul 16, 2019

ViktorHofer mentioned this pull request Feb 1, 2020

Run code analyzer on test projects to search for missing Fact/Theory attributes dotnet/runtime#29695

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Use LoopbackServer for HttpWebRequestTests (dotnet/corefx#37451)

d852a83

* Use LoopbackServer for HttpWebRequestTests Commit migrated from dotnet/corefx@d57a1a2

Use LoopbackServer for HttpWebRequestTests #37451

Use LoopbackServer for HttpWebRequestTests #37451

Uh oh!

Conversation

ViktorHofer commented May 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

davidsh commented May 28, 2019

Uh oh!

ViktorHofer commented May 28, 2019

Uh oh!

ViktorHofer commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidsh commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ViktorHofer commented May 29, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stephentoub May 29, 2019

Choose a reason for hiding this comment

Uh oh!

davidsh May 29, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stephentoub May 29, 2019

Choose a reason for hiding this comment

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

ViktorHofer commented May 29, 2019

Uh oh!

ViktorHofer commented May 30, 2019

Uh oh!

ViktorHofer commented May 31, 2019

Uh oh!

davidsh commented May 31, 2019

Uh oh!

ViktorHofer commented Jun 2, 2019

Uh oh!

stephentoub commented Jun 2, 2019

Uh oh!

ViktorHofer commented Jun 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ViktorHofer commented May 5, 2019 •

edited

Loading

ViktorHofer commented May 29, 2019 •

edited

Loading

davidsh commented May 29, 2019 •

edited

Loading