Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite System.Net.Http.Json functional tests to use a custom HttpMessageHandler #38733

Merged
merged 4 commits into from
Jul 7, 2020

Conversation

akoeplinger
Copy link
Member

@akoeplinger akoeplinger commented Jul 3, 2020

All of the tests start a socket-based loopback server which doesn't work on WebAssembly.
We can do the tests using a custom HttpMessageHandler instead of the loopback server as well.

@ghost
Copy link

ghost commented Jul 3, 2020

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

@eerhardt
Copy link
Member

eerhardt commented Jul 3, 2020

System.Net.Http.Json is used by the default Blazor WASM template, and I'd assume the majority of Blazor WASM apps that request JSON payloads over Http. Is there any other way we can test the functionality without creating a socket-based loopback server?

…sageHandler

All of the tests start a socket-based loopback server which doesn't work on WebAssembly.
We can do the tests using a custom HttpMessageHandler instead of the loopback server as well.
@akoeplinger akoeplinger changed the title WASM: Disable System.Net.Http.Json functional tests Rewrite System.Net.Http.Json functional tests to use a custom HttpMessageHandler Jul 3, 2020
@akoeplinger
Copy link
Member Author

@eerhardt I rewrote the tests to remove the dependency on the socket-based server

Copy link
Member

@mdh1418 mdh1418 left a comment

Choose a reason for hiding this comment

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

Looks good besides those two parts that weren't clear to me.

@akoeplinger akoeplinger requested a review from a team July 6, 2020 20:59
@ManickaP ManickaP requested a review from jozkee July 7, 2020 08:14
@ManickaP
Copy link
Member

ManickaP commented Jul 7, 2020

This will probably collide with #38635.
If, by any miracle, I'm first to merge, I can help with fixing follow up conflicts, just let me know.

Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

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

Just one question; otherwise, looks good.

Comment on lines -52 to -55
for (int i = 0; i < NumRequests; i++)
{
await server.HandleRequestAsync(content: json, headers: headers);
}
Copy link
Member

Choose a reason for hiding this comment

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

Why is that you only have to call HandleRequestAsync once now?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jozkee In the previous implementation we actually had to create four responses from the socket-based loopback server, one for each request since the callback was only invoked once.

Now we invoke the callback for each request so this is no longer necessary.

@akoeplinger akoeplinger merged commit 4c1b842 into dotnet:master Jul 7, 2020
@akoeplinger akoeplinger deleted the wasm-sys-net-http-json branch July 7, 2020 20:01
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants