-
Notifications
You must be signed in to change notification settings - Fork 5.2k
ClientWebSocket Loopback Server tests #117255
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
Conversation
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only poking my nose a bit in stuff I know 😄
src/libraries/Common/tests/System/Net/Configuration.WebSockets.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/tests/System/Net/Http/GenericLoopbackServer.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs
Outdated
Show resolved
Hide resolved
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
...braries/Common/tests/System/Net/Prerequisites/NetCoreServer/Handlers/EchoWebSocketHandler.cs
Show resolved
Hide resolved
.../Common/tests/System/Net/Prerequisites/NetCoreServer/Handlers/EchoWebSocketHeadersHandler.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR expands WebSocket test coverage by running external “OuterLoop” tests in the inner loop against a loopback server, adds HTTP/2 support, and unifies the test class hierarchy.
- Refactored test data sources (
EchoServers
,EchoServersAndBoolean
) to useUri[]
and streamlined MemberData generation. - Consolidated WebSocket setup into
WebSocketHelper
(addedConnectAsync
,TestEcho
, and UTF-8 helpers) and removedWebSocketData
. - Updated
.csproj
files to include new loopback test files and removed obsolete ones.
Reviewed Changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/libraries/System.Net.WebSockets/tests/WebSocketCreateTest.cs | Refactored echo server MemberData to use Uri[] and boolean combinations |
src/libraries/System.Net.WebSockets.Client/tests/WebSocketHelper.cs | Added ConnectAsync , TestEcho , and ToUtf8 /FromUtf8 ; removed WebSocketData |
src/libraries/System.Net.WebSockets.Client/tests/ClientWebSocketTestBase.cs | Unified MemberData generation, added HTTP/2 query-header handling |
Comments suppressed due to low confidence (3)
src/libraries/System.Net.WebSockets.Client/tests/ClientWebSocketTestBase.cs:18
- [nitpick] Consider renaming
EchoServers_Values
to something likeEchoServerUris
for clearer intent and to follow C# naming conventions without underscores.
public static readonly Uri[] EchoServers_Values = System.Net.Test.Common.Configuration.WebSockets.GetEchoServers();
src/libraries/System.Net.WebSockets.Client/tests/LoopbackServer/LoopbackWebSocketServer.cs:135
- [nitpick] The
Options
record has grown to many properties; consider splitting HTTP/1.1 vs HTTP/2 options into separate types or using nested builders to improve clarity and reduce complexity.
public record class Options()
src/libraries/System.Net.WebSockets.Client/tests/WebSocketHelper.cs:189
- [nitpick] Calling
Encoding.UTF8.GetBytes
allocates a new byte array on each call; consider reusing a sharedEncoding
instance or using a pooled buffer for high-volume or large-message scenarios.
public static ArraySegment<byte> ToUtf8(string text)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just starting 😄
Publishing what I have so far, so it won't get lost.
src/libraries/System.Net.WebSockets.Client/tests/LoopbackServer/LoopbackWebSocketServer.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More feedback. Most is opinionated so don't take it as some directive.
...braries/Common/tests/System/Net/Prerequisites/NetCoreServer/Handlers/EchoWebSocketHandler.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets.Client/tests/LoopbackServer/LoopbackWebSocketServer.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets.Client/tests/LoopbackServer/LoopbackWebSocketServer.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets.Client/tests/WebSocketHelper.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets.Client/tests/WebSocketHelper.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets.Client/tests/WebSocketHelper.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not poking into this more.
src/libraries/System.Net.WebSockets.Client/tests/LoopbackServer/LoopbackWebSocketServer.Http.cs
Outdated
Show resolved
Hide resolved
/ba-g (unrelated) wasm timeout is already tracked by dotnet/dnceng#3008 |
UPD: all related CI passed (innerloop+outerloop+wasm)
Fixes #73849
Fixes #28957
[WIP until CI validation on all platforms]Expanding test coverage by bringing all of the OuterLoop external server tests to also run in inner loop against the Loopback server.
Innerloop tests before:
Innerloop tests after (181->661, +265%):
Known gaps: