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

[browser][websocket] Fix for Close Description validation #45536

Merged
merged 9 commits into from
Jan 5, 2021
Merged

[browser][websocket] Fix for Close Description validation #45536

merged 9 commits into from
Jan 5, 2021

Conversation

kjpou1
Copy link
Contributor

@kjpou1 kjpou1 commented Dec 3, 2020

The description validation exception was not being percolated through to the unit test

The tests are expecting an exception.

 fail: [FAIL] System.Net.WebSockets.Client.Tests.CloseTest.CloseAsync_CloseDescriptionIsMaxLengthPlusOne_ThrowsArgumentException(server: ws://corefx-net-http11.azurewebsites.net/WebSocket/EchoWebSocket.ashx)
  info: Assert.Throws() Failure
  info: Expected: typeof(System.ArgumentException)
  info: Actual:   (No exception was thrown)
  info:    at System.AssertExtensions.Throws[ArgumentException](String expectedParamName, Action action)
  info:    at System.Net.WebSockets.Client.Tests.CloseTest.CloseAsync_CloseDescriptionIsMaxLengthPlusOne_ThrowsArgumentException(Uri server)
  info: --- End of stack trace from previous location ---

Running the outerloop tests locally passes correctly with this modification which follows the ManagedWebSocket.cs verification sequence.

See #45470 for tests

Fix #45531

- Issue #45531
- The description validation exception was not being percolated through to the unit test
@ghost
Copy link

ghost commented Dec 3, 2020

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

Issue Details

The description validation exception was not being percolated through to the unit test

The tests are expecting an exception.

 fail: [FAIL] System.Net.WebSockets.Client.Tests.CloseTest.CloseAsync_CloseDescriptionIsMaxLengthPlusOne_ThrowsArgumentException(server: ws://corefx-net-http11.azurewebsites.net/WebSocket/EchoWebSocket.ashx)
  info: Assert.Throws() Failure
  info: Expected: typeof(System.ArgumentException)
  info: Actual:   (No exception was thrown)
  info:    at System.AssertExtensions.Throws[ArgumentException](String expectedParamName, Action action)
  info:    at System.Net.WebSockets.Client.Tests.CloseTest.CloseAsync_CloseDescriptionIsMaxLengthPlusOne_ThrowsArgumentException(Uri server)
  info: --- End of stack trace from previous location ---

Running the outerloop tests locally passes correctly with this modification.

See #45470 for tests

Fix #45531

Author: kjpou1
Assignees: -
Labels:

area-System.Net

Milestone: -

@kjpou1 kjpou1 self-assigned this Dec 3, 2020
@kjpou1 kjpou1 added the arch-wasm WebAssembly architecture label Dec 3, 2020
@radical
Copy link
Member

radical commented Dec 4, 2020

Just trying to understand the situation with tests here.

  • Do all the enabled tests pass for websockets?
    • If so, then are there some disabled tests that this would be fixing?
      • If so, then can we enable those tests with this PR?

I'm just wondering if there are any tests that we'll be able to see, going from FAIL to PASS.

In the PR that you linked, I see some tests being marked with ActiveIssue for 45531, which I guess will be removed after this is merged.

@kjpou1
Copy link
Contributor Author

kjpou1 commented Dec 4, 2020

The ActiveIssue for 45531 will be removed

@kjpou1
Copy link
Contributor Author

kjpou1 commented Dec 7, 2020

Can we get another review?

@kjpou1 kjpou1 requested a review from stephentoub December 7, 2020 04:25
@kjpou1
Copy link
Contributor Author

kjpou1 commented Dec 7, 2020

Build iOS x64 Release AllSubsets_Mono failing in all CI jobs #45654

@kjpou1 kjpou1 requested a review from BrennanConroy December 8, 2020 06:01
@kjpou1
Copy link
Contributor Author

kjpou1 commented Dec 8, 2020

The failure for Build iOS x64 Release AllSubsets_Mono failing in all CI jobs is unrelated see issue: #45468

/cc @marek-safar @stephentoub @lewing

kjpou1 and others added 4 commits December 9, 2020 09:45
…browser-close-desc-validation

# Conflicts:
#	src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs
@kjpou1 kjpou1 merged commit 867ba80 into dotnet:master Jan 5, 2021
@kjpou1 kjpou1 deleted the wasm-browser-close-desc-validation branch January 11, 2021 04:49
@karelz karelz added this to the 6.0.0 milestone Jan 26, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.Net
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[browser][websockets][tests] Tests for CloseDescription Max Length are failing
7 participants