Repository navigation
Add more HttpListener WebSocket tests - #20398
Conversation
|
Does anyone know why I'm getting failures with Windows 8.1? It seems that the |
| } | ||
|
|
||
| [ActiveIssue(20246)] // CI hanging frequently | ||
| [ActiveIssue(20246, TestPlatforms.AnyUnix)] // CI hanging frequently |
There was a problem hiding this comment.
I don't understand when you're choosing to use AnyUnix vs IsWindowsImplementation. I disabled this everywhere because it repros regardless of platform when using the managed implementation.
It shouldn't matter. The RFC describes it as being case insensitive. |
| HttpListenerWebSocketContext socketContext = await context.AcceptWebSocketAsync("SubProtocol"); | ||
|
|
||
| Assert.Equal("SubProtocol", context.Response.Headers["Sec-WebSocket-Protocol"]); | ||
| Assert.Equal("Upgrade", context.Response.Headers["Connection"]); |
There was a problem hiding this comment.
All of these comparisons should be done case insensitive.
|
|
||
| [ConditionalFact(nameof(IsNotWindows7OrUapCore))] | ||
| public async Task AcceptWebSocketAsync_SocketSpoofingAsWebSocket_ThrowsWebSocketException() | ||
| public async Task AcceptWebSocketAsync_ValidWebSocket_SetsUpHeadersInResponse() |
There was a problem hiding this comment.
Are you not seeing random failures in your new tests when running the managed implementation on Windows?
|
|
||
| if (ClientConnectTask?.IsCompleted == true) | ||
| { | ||
| ClientConnectTask?.Dispose(); |
There was a problem hiding this comment.
You don't need to dispose of the task. Please remove this and the corresponding changes in GetWebSocketContext.
| Assert.Null(result.CloseStatus); | ||
| Assert.Null(result.CloseStatusDescription); | ||
|
|
||
| Assert.Equal(Text, Encoding.ASCII.GetString(receivedBytes)); |
There was a problem hiding this comment.
There's technically no guarantee that you'll receive the same number of bytes in a single receive; it's valid for the implementation to split it up amongst several packets such that you'll get the data across multiple receives. It's of course very likely this will pass, but it could be the source of sporadic failures in the future.
stephentoub
left a comment
stephentoub
left a comment
There was a problem hiding this comment.
Thanks for adding all of these, Hugh!
| else | ||
| { | ||
| // AuthenticationSchemes.Digest, AuthenticationSchemes.Negotiate, AuthenticationSchemes.NTLM. | ||
| throw new NotImplementedException(); |
There was a problem hiding this comment.
I think this should be PlatformNotSupportedException
- Cleanup some test code - Add HttpListener web socket tests - Fix some more Windows porting bugs - Baseline managed test failures
- Change CopyPrincipal to throw PNSE instead of NotImplementedException - Fix test attribution to allow "msbuild /t:rebuildandtest" to work with both Windows and managed implementations - Fix IsWindowsImplementation to recognize both netcoreapp and netfx implementations - Fix Assert.Equal calls that should be case-insensitive - Move Get_Disposed_ThrowsObjectDisposedException to also apply to managed implementation - Remove unnecessary Task disposal in HttpListenerWebSocketTests - Add missing await of ClientConnectTask in several tests, where the lack of it resulted in sporadic failures due to race conditions - Replace ReceiveAsync calls with custom ReceiveAllAsync calls where necessary
|
Sorry for the delay thanks for addressing the PR feedback! |
Contributes to #13618
Contributes to #20389
Contributes to #20391