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

[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 •
edited
Loading

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

@kjpou1
[browser][websocket] Fix for Close Description validation
1f90a64 
- 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
@kjpou1 kjpou1 mentioned this pull request Dec 3, 2020
Merged
@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
Copy link
Contributor Author

kjpou1 commented Dec 7, 2020

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

@runfoapp runfoapp bot mentioned this pull request Dec 7, 2020
Closed
@kjpou1
Copy link
Contributor Author

kjpou1 commented Dec 8, 2020 •
edited
Loading

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 kjpou1 mentioned this pull request Dec 9, 2020
Closed
kjpou1 and others added 4 commits December 9, 2020 09:45
@kjpou1
Merge branch 'master' of https://github.com/dotnet/runtime into wasm-…
8eded00 
…browser-close-desc-validation

# Conflicts:
#	src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs
@kjpou1
Fix merge conflict
2b9d862
@lewing
Merge remote-tracking branch 'upstream/master' into wasm-browser-clos…
6e8026c 
…e-desc-validation
@lewing
Remove active issue
c78e2f7
@runfoapp runfoapp bot mentioned this pull request Dec 11, 2020
Closed
@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.
Reviewers

@stephentoub stephentoub stephentoub approved these changes

@lewing lewing Awaiting requested review from lewing

@marek-safar marek-safar Awaiting requested review from marek-safar

@steveisok steveisok Awaiting requested review from steveisok

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy

Assignees

@kjpou1 kjpou1

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.

[browser][websockets][tests] Tests for CloseDescription Max Length are failing
7 participants
@kjpou1 @radical @stephentoub @BrennanConroy @karelz @Dotnet-GitSync-Bot @lewing