-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[QUIC] Fixed some tests leaking unobserved exceptions #104444
Conversation
8df8d22
to
a90fa47
Compare
Tagging subscribers to this area: @dotnet/ncl |
src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicCipherSuitesPolicyTests.cs
Outdated
Show resolved
Hide resolved
@@ -62,11 +63,14 @@ public void NoSupportedCiphers_ThrowsArgumentException(TlsCipherSuite[] ciphers) | |||
return ValueTask.FromResult(serverOptions); | |||
} | |||
}; | |||
Assert.ThrowsAsync<ArgumentException>(async () => await CreateQuicListener(listenerOptions)); | |||
await using var listener = await CreateQuicListener(listenerOptions); |
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.
How come this was supposed to be throwing before?
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.
it is possible it was never throwing and the test was wrong, the Tasks were not awaited in this test at all before this PR
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.
Looking at this again, the QuicListener creation should not throw, because we don't check the CipherSuitesPolicy there, we only access it when we get an incoming connection.
src/libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestCollection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicCipherSuitesPolicyTests.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.
LGTM with few nits
ab8584e
to
546f421
Compare
Fixes leaking unobserved exceptions from S.N.Quic tests.
Adds print out for the leaking exceptions - I can remove it if it's considered spam.
The remaining issue comes from
QuicConnection._connectionCloseTcs
but the fix is a bit more involved, I'll do it in separate PR.Contributes to #80111