Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

HTTP2: Modify HTTP2 tests to run against platform handlers as well#34630

Merged
geoffkizer merged 4 commits intodotnet:masterfrom
geoffkizer:http2tests
Feb 8, 2019
Merged

HTTP2: Modify HTTP2 tests to run against platform handlers as well#34630
geoffkizer merged 4 commits intodotnet:masterfrom
geoffkizer:http2tests

Conversation

@geoffkizer
Copy link

Currently, HTTP2 specific tests are only running against SocketsHttpHandler. Modify them to also run against platform handlers, and modify tests as appropriate to deal with differences in behavior.

I haven't tested this with CurlHandler yet; I'll look at the results of the CI pass and see what needs to be modified.

@dotnet/ncl

@geoffkizer geoffkizer self-assigned this Jan 16, 2019
@geoffkizer geoffkizer added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jan 16, 2019
@geoffkizer
Copy link
Author

@dotnet-bot test outerloop Linux x64 debug build
@dotnet-bot test outerloop OSX x64 debug build
@dotnet-bot test outerloop Windows x64 debug build

@davidsh davidsh added test enhancement Improvements of test source code area-System.Net.Http labels Jan 16, 2019
@davidsh davidsh added this to the 3.0 milestone Jan 16, 2019
@geoffkizer
Copy link
Author

@dotnet-bot test outerloop Linux x64 debug build
@dotnet-bot test outerloop OSX x64 debug build
@dotnet-bot test outerloop Windows x64 debug build

2 similar comments
@geoffkizer
Copy link
Author

@dotnet-bot test outerloop Linux x64 debug build
@dotnet-bot test outerloop OSX x64 debug build
@dotnet-bot test outerloop Windows x64 debug build

@geoffkizer
Copy link
Author

@dotnet-bot test outerloop Linux x64 debug build
@dotnet-bot test outerloop OSX x64 debug build
@dotnet-bot test outerloop Windows x64 debug build

@geoffkizer
Copy link
Author

I added some basic GOAWAY tests and proper connection shutdown handling in the loopback server.

RstStreamFrame resetStream = new RstStreamFrame(FrameFlags.None, 0x2, streamId);
await server.WriteFrameAsync(resetStream);

await Assert.ThrowsAsync<HttpRequestException>(async () => await sendTask);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: what you have is fine, but for reference this could also just be:

await Assert.ThrowsAsync<HttpRequestException>(() => sendTask);

@geoffkizer
Copy link
Author

In the interest of getting this in, I'm just disabling the platform handlers using an #if. The tests do run successfully against WinHttpHandler, but they don't run by default. We can revisit this later if desired.

@geoffkizer
Copy link
Author

@dotnet-bot test outerloop Linux x64 debug build
@dotnet-bot test outerloop OSX x64 debug build
@dotnet-bot test outerloop Windows x64 debug build

@geoffkizer geoffkizer removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 6, 2019
@geoffkizer
Copy link
Author

Removing the no-merge label since this is ready to merge.

@dotnet/ncl please take a look.

@geoffkizer
Copy link
Author

@dotnet-bot test this please

@geoffkizer
Copy link
Author

@dotnet-bot test outerloop Linux x64 debug build
@dotnet-bot test outerloop OSX x64 debug build
@dotnet-bot test outerloop Windows x64 debug build


await server.EstablishConnectionAsync();
await server.ReadRequestHeaderAsync();
int streamId = await server.ReadRequestHeaderAsync();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason to add this line if we don't use the streamId?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, probably a copy-and-paste thing.

if (!IsWinHttpHandler)
{
// The client should close the connection as this is a fatal connection level error.
Assert.Null(await server.ReadFrameAsync(TimeSpan.FromSeconds(30)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating this.

Copy link
Contributor

@rmkerr rmkerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@geoffkizer geoffkizer merged commit 073bf47 into dotnet:master Feb 8, 2019
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
HTTP2: Modify HTTP2 tests to run against platform handlers as well

Commit migrated from dotnet/corefx@073bf47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Net.Http test enhancement Improvements of test source code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants