-
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 test improvements #56043
quic test improvements #56043
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue DetailsThis is test-only change
contributes to #55642
|
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.
Some small comments, otherwise LGTM, thanks!
src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs
Outdated
Show resolved
Hide resolved
@@ -28,6 +30,10 @@ public abstract class QuicTestBase<T> | |||
public X509Certificate2 ServerCertificate = System.Net.Test.Common.Configuration.Certificates.GetServerCertificate(); | |||
public X509Certificate2 ClientCertificate = System.Net.Test.Common.Configuration.Certificates.GetClientCertificate(); | |||
|
|||
public const int PassingTestTimeout = 30_000; |
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.
Why don't you declare it as TimeSpan
? You wouldn't need to add those overloads to TaskTimeoutExtensions.cs and you'd have avoided mistakes like WaitAsync(TimeSpan.FromSeconds(PassingTestTimeout))
- seconds instead of milliseconds. Unless you really want there super long timeout.
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.
Some tests suites have actually both - one created from the other. I may do that for consistency.
src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs
Outdated
Show resolved
Hide resolved
@@ -30,10 +30,10 @@ public async Task UnidirectionalAndBidirectionalStreamCountsWork() | |||
{ | |||
using QuicListener listener = CreateQuicListener(); | |||
using QuicConnection clientConnection = CreateQuicConnection(listener.ListenEndPoint); | |||
Task<QuicConnection> serverTask = listener.AcceptConnectionAsync().AsTask(); |
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.
SInce this is a common, repeated pattern, I wonder if we should have a helper method for it, like CreateClientAndServer or something like that. Or EstablishConnection or something.
We have RunClientServer, but that's not really the same.
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.
yah. I wanted to like SslStream. But if I return tuple we loose the using var
.
We would need to than use good old
using (clientConn)
using (serverConn)
{
....
}
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.
BTW the helper would be also nice if we want to bet in any retry logs for the flaky listener if we don't find way how to fix it.
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.
Yeah, it's a little verbose because of the using stuff.
One idea would be to define something like
struct QuicConnectionPair : IDisposable
{
public QuicConnection Client { get; }
public QuicConnection Server { get; }
public void Dispose() { Client.Dispose(); Server.Dispose(); }
}
public static ValueTask<ConnectionPair> EstablishConnectionAsync(...) { ... }
Then you can just write code like this:
using (var connectionPair = await EstablishConnectionAsync(...);
// use connectionPair.Client and connectionPair.Server here
If we feel really ambitious we could generalize ConnectionPair
to something like DisposablePair<T1, T2>
. And maybe add some implicit conversion ops from ValueTuple<T1, T2>
...
@stephentoub already has something vaguely like this for the Stream Conformance Tests.
This is test-only change
ITestOutputHelper
so we can capture more data on failurescontributes to #55642