[QUIC] Improve idle timeout tests #73228
Conversation
ghost
assigned 
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsNoticed the test when looking into other stuff. The original test did not set idle timeout and so it took long time to run. I also added a check for the right exception from a QuicStream
|
| await clientStream.WriteAsync(new byte[1]); | ||
| using QuicStream serverStream = await serverConnection.AcceptInboundStreamAsync(); | ||
|
|
||
| var acceptTask = serverConnection.AcceptInboundStreamAsync(); |
| var acceptTask = serverConnection.AcceptInboundStreamAsync(); | ||
|
|
||
| // Wait for idle timeout | ||
| await Task.Delay(TimeSpan.FromSeconds(10)); |
There was a problem hiding this comment.
Does it need to be 10 seconds? Can we decrease both the idle timeout and this value?
There was a problem hiding this comment.
We probably can, we might not even need this delay (it will block and wait on the next assert).
What timeout would you think appropriate? 1s? I did not want to have it too low to avoid flakiness on CI.
There was a problem hiding this comment.
Can we write the test in a way that's more resilient? For example, issue a read against the stream, time how long it takes for it to fail, and then validate the time is within some acceptable range?
There was a problem hiding this comment.
I don't see how that makes the test more resilient... The way I see it we care about the QuicErrorCode thrown in the QuicException, not necessarily about the precision of the timing (which is difficult enough since we don't know when MsQuic will actually send/receive the packets
There was a problem hiding this comment.
It's a combination of resiliency and how much time we're willing to spend in CI. The way you have it written, you're forced to choose a large value that makes the test take much longer in hopes that it'll be long enough to catch any flakiness in CI. The way I suggested, you don't have to choose a time to wait at all; you simply wait for the read to complete, validate it threw the expected exception, and validate that the time it took to complete was within some acceptable range. Basically, I'm not sure what purpose this Task.Delay is serving, only to make the test take longer.
There was a problem hiding this comment.
I'm not sure what purpose this Task.Delay is serving
I noted above that the delay can be removed, the test can wait/block on the asserts that follow so that there is no unnecessary delay. My question was more about how long do you think the IdleTimeout value should be.
There was a problem hiding this comment.
Ok. I think it should be short, like 1 second, unless there's something about a short timeout that makes that problematic.
|
Test failures are unrelated |
|
Test failure is probably dotnet/arcade#9009 |
Noticed the test when looking into other stuff. The original test did not set idle timeout and so it took long time to run. I also added a check for the right exception from a QuicStream