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.

Sign up for GitHub

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

Jump to bottom

[wasm][testing] hosting echo server in xharness process #52923

Merged
merged 6 commits into from
Jun 9, 2021
Merged

[wasm][testing] hosting echo server in xharness process #52923

merged 6 commits into from
Jun 9, 2021

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented May 18, 2021 •
edited
Loading

This makes WebSocket test on WASM easier to work with, by moving the tests to inner loop. In order to do that we are hosting the echo test server which the tests use, in the xharness process. Because wasm is always tested with xharness.

@ghost
Copy link

ghost commented May 18, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Companion change to dotnet/xharness#593

  • build echo test server and pass it to xharness
  • WebSocket need to run in browser/chromeDriver, not in V8 console
  • move WebSocket tests to inner loop for browser
  • doc fix
Author: pavelsavara
Assignees: -
Labels:

area-System.Net
Milestone: -

This was referenced May 18, 2021
Closed
Closed
Merged
@pavelsavara pavelsavara requested a review from radical June 3, 2021 14:33
@pavelsavara
Copy link
Member Author

fail: Failed to find the middleware assembly at /__w/1/s/artifacts/bin/NetCoreServer/net6.0-Release/NetCoreServer.dll

@pavelsavara pavelsavara marked this pull request as ready for review June 4, 2021 13:53
@pavelsavara pavelsavara requested a review from mdh1418 June 4, 2021 13:53
@pavelsavara pavelsavara added the arch-wasm WebAssembly architecture label Jun 4, 2021
@ghost
Copy link

ghost commented Jun 4, 2021

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

This makes WebSocket test on WASM easier to work with, by moving the tests to inner loop. In order to do that we are hosting the echo test server which the tests use, in the xharness process. Because wasm is always tested with xharness.

Author: pavelsavara
Assignees: -
Labels:

arch-wasm, area-System.Net
Milestone: -

mdh1418
mdh1418 reviewed Jun 4, 2021
View reviewed changes
eng/testing/tests.props Outdated Show resolved Hide resolved
mdh1418
mdh1418 reviewed Jun 4, 2021
View reviewed changes
eng/testing/tests.props Outdated Show resolved Hide resolved
@pavelsavara pavelsavara requested a review from mdh1418 June 4, 2021 15:52
mdh1418
mdh1418 approved these changes Jun 4, 2021
View reviewed changes
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.

Overall looks good to me, thanks!

The remaining nits I have are the naming in Configuration.Http.cs and using NetCoreAppCurrent instead in src/libraries/Common/tests/System/Net/Prerequisites/NetCoreServer/NetCoreServer.csproj

@pavelsavara pavelsavara requested a review from wfurt June 4, 2021 16:13
@radical radical requested a review from lewing June 4, 2021 16:18
radical
radical reviewed Jun 4, 2021
View reviewed changes
docs/workflow/testing/libraries/testing-wasm.md Show resolved Hide resolved
docs/workflow/testing/libraries/testing-wasm.md Outdated Show resolved Hide resolved
eng/testing/tests.props Outdated Show resolved Hide resolved
eng/testing/tests.props Outdated Show resolved Hide resolved
src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.RemoteServer.cs Outdated Show resolved Hide resolved
src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.RemoteServer.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Headers.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs Outdated Show resolved Hide resolved
src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.RemoteServer.cs Outdated Show resolved Hide resolved
src/libraries/Common/tests/System/Net/Http/PostScenarioTest.cs Outdated Show resolved Hide resolved
@pavelsavara
Copy link
Member Author

#52923 (comment)
[SkipOnPlatform(TestPlatforms.Browser, "Host is ignored on Browser")] do you believe we could fix this @radical ? Then ActiveIssue would be more appropriate.

@ManickaP ManickaP self-requested a review June 7, 2021 12:06
@radical
Copy link
Member

radical commented Jun 7, 2021

#52923 (comment)
[SkipOnPlatform(TestPlatforms.Browser, "Host is ignored on Browser")] do you believe we could fix this @radical ? Then ActiveIssue would be more appropriate.

As @mdh1418 asked, What does this mean? Is m.Headers.Host problematic?, could you just explain the reasoning? What fails, and how does it fail?

@pavelsavara
- improve doc
63e863b 
- move tests to inner loop
- more granular ActiveIssue dotnet#53592 for lack of TRACE
- more granular ActiveIssue dotnet#53591 for content on GET/HEAD
- more granular ActiveIssue dotnet#53874 for HttpRequestMessage.Headers.Host
- more granular ActiveIssue dotnet#53872 for NPE on System.Net.Http.BrowserHttpHandler
- fix HTTP vs HTTPS test configuration `Http2SecureRemoteEchoServer`
- include echo middleware in xharness server
- include middleware in Helix correlation payload
@pavelsavara
document dotnet#53876
3fdc1d9
@pavelsavara
Copy link
Member Author

As @mdh1418 asked, What does this mean? Is m.Headers.Host problematic?, could you just explain the reasoning? What fails, and how does it fail?

Created active issue instead #53874

ManickaP
ManickaP reviewed Jun 8, 2021
View reviewed changes
Copy link
Member

@ManickaP ManickaP 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, just a question about the split of secure server for H/2. I'll approve once it's cleared.

src/libraries/Common/tests/System/Net/Configuration.Http.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.WebSockets.Client/tests/System.Net.WebSockets.Client.Tests.csproj Outdated Show resolved Hide resolved
ManickaP
ManickaP approved these changes Jun 9, 2021
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

Thanks!

@pavelsavara pavelsavara changed the title [wasm][testing] hosting webSocket echo server in xharness process [wasm][testing] hosting echo server in xharness process Jun 9, 2021
radical
radical approved these changes Jun 9, 2021
View reviewed changes
Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

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

Thanks for your patience, LGTM! 👍
You might want to run the outerloop tests to make sure that this didn't break anything.

@pavelsavara
Copy link
Member Author

/azp run runtime-libraries-mono outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara pavelsavara merged commit fb61be4 into dotnet:main Jun 9, 2021
@karelz karelz mentioned this pull request Jun 17, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Jul 9, 2021
@karelz karelz added this to the 6.0.0 milestone Jul 15, 2021
@pavelsavara pavelsavara deleted the pr_wasm_network_test_server branch July 29, 2021 08:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@radical radical radical approved these changes

@ManickaP ManickaP ManickaP approved these changes

@mdh1418 mdh1418 mdh1418 approved these changes

@wfurt wfurt Awaiting requested review from wfurt

@lewing lewing Awaiting requested review from lewing

Assignees
No one assigned
Labels
arch-wasm WebAssembly architecture area-System.Net
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

[wasm] HttpResponseMessage.Headers is empty [wasm][HTTP] TypeError: Failed to fetch
5 participants
@pavelsavara @radical @ManickaP @mdh1418 @karelz