Add PollingRequestQueueTimeout and PollingRequestMaximumMessageProcessingTimeout tests #564
Conversation
…singTimeout tests
|
|
||
| stopwatch.Elapsed.Should() | ||
| .BeGreaterThan(halibutTimeoutsAndLimits.PollingRequestQueueTimeout, "Should have waited longer than the PollingRequestQueueTimeout").And | ||
| .BeLessThan(responseDelay + TimeSpan.FromSeconds(5), "Should have received the response after the 10 second delay + 5 second buffer"); |
There was a problem hiding this comment.
nit: Is this last assertion necessary? If it took 15.1s on a busy build server would that still be ok since the call completed successfully and it exceeds the PollingRequestQueueTimeout
There was a problem hiding this comment.
No it's redundant isn't it
There was a problem hiding this comment.
Removed
|
|
||
| stopwatch.Elapsed.Should().BeLessThan(TimeSpan.FromSeconds(15), "Should have timed out quickly"); | ||
|
|
||
| connectionsObserver.ConnectionAcceptedCount.Should().Be(1, "A single TCP connection should have been created"); |
There was a problem hiding this comment.
nit: What is this assertion trying to show? is it that the request was picked up?
There was a problem hiding this comment.
It's more relevant to cooperative cancellation, slightly redundant in the tests current form - see the Wait below. The aim was to try to ensure the request was cancelled to the socket causing reconnects.
There was a problem hiding this comment.
I've left the test as it is so it's "ready" for cooperative cancellation
| // Leaving these asserts as when cooperative cancellation is supported it should cause them to fail at which point they can be fixed to assert cancellation to the socket works as expected. | ||
| connectionsObserver.ConnectionClosedCount.Should().Be(0, "Cancelling the PendingRequest does not cause the TCP Connection to be cancelled to stop the in-flight request"); | ||
| connectionsObserver.ConnectionAcceptedCount.Should().Be(1, "The Service won't have reconnected after the request was cancelled"); | ||
| }, TimeSpan.FromSeconds(30), Logger, CancellationToken); |
There was a problem hiding this comment.
nit: Can this be reduced?
There was a problem hiding this comment.
nit: is 30s special for some reason?
There was a problem hiding this comment.
No, I think it was lifted from some other test that decided to use 30 seconds. Enough time to not be flakey but not too much time to fail to slowly!!
What would you suggest this timeout should be @LukeButters
There was a problem hiding this comment.
I have no better suggestion, just checking if it is special :)
Background
Add PollingRequestQueueTimeout and PollingRequestMaximumMessageProcessingTimeout tests
Cherry-picked from this reverted PR #558
How to review this PR
Quality ✔️
Pre-requisites