[HTTP/3] Reenable interop tests#56867
Conversation
ghost
added
the
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsRe-enabled interop tests. Added some more external servers. https://quic.rocks:4433/ was randomly reporting as UNREACHABLE, so I removed it. Fixes #54726
|
|
|
||
| Assert.Equal(HttpStatusCode.OK, responseB.StatusCode); | ||
| Assert.NotEqual(3, responseB.Version.Major); | ||
| Assert.Equal(3, responseB.Version.Major); |
There was a problem hiding this comment.
How did this test ever work?? :)
| Method = HttpMethod.Get, | ||
| RequestUri = new Uri(uri, UriKind.Absolute), | ||
| Version = HttpVersion.Version30, | ||
| Version = HttpVersion.Version20, |
There was a problem hiding this comment.
Does this actually matter? The first request should not be processed via HTTP3 regardless, because we haven't received the Alt-Svc header yet. In fact isn't that the point of setting Version30 here -- to validate that even though we requested 30, we won't get it on the first request because we haven't gotten an Alt-Svc yet?
There was a problem hiding this comment.
They were all failing with the RequestA being directly processed as H/3. The problem was in the test using a handler replacing HttpVersionPolicy.RequestVersionOrLower with HttpVersionPolicy.RequestVersionExact. I fixed that.
| Version = HttpVersion.Version30, | ||
| VersionPolicy = HttpVersionPolicy.RequestVersionExact | ||
| }; | ||
| using HttpResponseMessage response = await client.SendAsync(request, HttpCompletionOption.ResponseContentRead).WaitAsync(TimeSpan.FromSeconds(20)); |
There was a problem hiding this comment.
Can we validate the actual content too? I don't know how these endpoints work. If they have fixed content that will never change, seems like we could validate that we receive the correct content.
Or if not, can we at least validate that the content exists, i.e. we receive > 0 bytes?
There was a problem hiding this comment.
I added a check for non-empty content.
I don't think we should check for a particular content value since these are external and can change any time.
geoffkizer
left a comment
geoffkizer
left a comment
There was a problem hiding this comment.
Product change looks fine -- some questions re test code above.
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Failures are unrelated. |
4 participants
Re-enabled interop tests. Added some more external servers.
Added tests which also buffer the content and fixed handling of 0-byte length DATA frame.
https://quic.rocks:4433/ was randomly reporting as UNREACHABLE, so I removed it.
Fixes #54726