Fix ClientWebSocket error message when endpoint is not a real websocket #29159
Conversation
|
@dotnet-bot test Outerloop Linux x64 Debug Build please @dotnet-bot test Outerloop UWP CoreCLR x64 Release Build |
| @@ -29,7 +28,7 @@ public class ConnectTest : ClientWebSocketTestBase | |||
|
|
|||
| Assert.Equal(WebSocketError.Success, ex.WebSocketErrorCode); | |||
| Assert.Equal(WebSocketState.Closed, cws.State); | |||
| Assert.Equal(ResourceHelper.GetExceptionMessage("net_webstatus_ConnectFailure"), ex.Message); | |||
| Assert.Equal(exceptionMessage, ex.Message); | |||
There was a problem hiding this comment.
It's possible this test will fail in UAP since string resources from our CoreFx libraries are stripped out.
Instead of this Assert.Equal, there is a test helper "Assert" class that should be used here when wanting to test exception messages.
There was a problem hiding this comment.
See patterns like this in the code: ClientWebSocketUnitTest.cs:
AssertExtensions.Throws<ObjectDisposedException>(
() => cws.ReceiveAsync(segment, ct).GetAwaiter().GetResult(),
expectedException.Message);There was a problem hiding this comment.
Thanks for pointing this out! I vaguely remember that we need to use AssertExtensions for UAP run, because we don't want to validate the message, as it could be translated in future. I think this is the case.
Will fix.
| Assert.Equal(ResourceHelper.GetExceptionMessage("net_webstatus_ConnectFailure"), ex.Message); | ||
|
|
||
| // The .Net Native toolchain optimizes away exception messages. | ||
| if (!PlatformDetection.IsNetNative) |
There was a problem hiding this comment.
Changed to PlatformDetection instead of AssertExtension, because we also want to validate the ex.WebSocketErrorCode above.
There was a problem hiding this comment.
You should still be able to use AssertExtension.Throws and save the exception it gives you back. And then check the error code. Using AssertExtension can remove you needing to check for "IsNetNative".
There was a problem hiding this comment.
AssertExtension doesn't contain method overload to examine WebSocketException (or Exception in general) error message field and return the Exception. Which means, if we use other overload to save the Exception, we still need to use IsNetNative check in our test code to verify the ErrorMessage.
- It's possible to add a method overload inside
AssertExtensionclass dedicated forWebSocketException, but the overhead is that we need to addSystem.Net.WebSocketsreference inside theCoreFx.private.TestUtilities, and I'm not sure if that's desirable (Currently that project has no dependency on System.Net). - It's not feasible to add a method overload for
Exceptionin general that, do both check error message and return the exception. Because all possible distinct function signatures have been taken. Or we will need to add public API (new methods) for that class.
My preference is to use the current if check in our test code, if the desire for checking WebSocketException is high, we could consider add it to the common AssertExtension.
@davidsh Do you have any thoughts on this?
There was a problem hiding this comment.
Thx for looking into this. Let's just keep it the way you're doing it in this PR.
|
@dotnet-bot test Outerloop Linux x64 Debug Build please @dotnet-bot test Outerloop UWP CoreCLR x64 Release Build |
|
@dotnet-bot Test Outerloop UWP NETNative x86 Release Build |
| @@ -29,7 +28,12 @@ public class ConnectTest : ClientWebSocketTestBase | |||
|
|
|||
| Assert.Equal(WebSocketError.Success, ex.WebSocketErrorCode); | |||
| Assert.Equal(WebSocketState.Closed, cws.State); | |||
| Assert.Equal(ResourceHelper.GetExceptionMessage("net_webstatus_ConnectFailure"), ex.Message); | |||
|
|
|||
| // The .Net Native toolchain optimizes away exception messages. | |||
|
@dotnet-bot test Outerloop Linux x64 Debug Build please @dotnet-bot test Outerloop UWP CoreCLR x64 Release Build |
…et (dotnet/corefx#29159) * fix clientwebsocket error message * address feedback * fix comment Commit migrated from dotnet/corefx@0c15e75
Close: #20360