[Event Hubs Client] Track Two (Test Stability) #9188
Conversation
The focus of these changes is to tweak some of the timings and loosen some assumptions around the non-deterministic areas of the tests to offer better stability in the nightly runs, where there is a higher resource contention and timing tends to vary more than with local runs.
jsquire
added
Event Hubs
Client
jsquire
requested review from
kinelski,
AlexGhiondea,
serkantkaraca and
ShivangiReja
| @@ -2140,7 +2139,7 @@ await using (var connection = new EventHubConnection(connectionString)) | |||
|
|
|||
| await using (var invalidProxyConsumer = new EventHubConsumerClient(EventHubConsumerClient.DefaultConsumerGroupName, connectionString, options)) | |||
| { | |||
| var readOptions = new ReadEventOptions { MaximumWaitTime = TimeSpan.FromMilliseconds(250) }; | |||
| var readOptions = new ReadEventOptions { MaximumWaitTime = null }; | |||
There was a problem hiding this comment.
Behavior on Linux and macOS is different for this scenario than it is on Windows. They do not immediately terminate with a web socket exception, but continue to attempt to connect until the timeout is exhausted. The previous setting was causing the test to terminate before the timeout period. Since it is not important that the read pulls events, setting no wait time and allowing it to block until the timeout period for connections (60 seconds) expires correctly executes on all platforms.
Note that the cancellation token continues to prevent stalling by allowing for twice the connection timeout, just in case.
| @@ -69,6 +69,7 @@ public void ReadLastEnqueuedEventPropertiesDelegatesToTheConsumer() | |||
| Assert.That(information.Offset, Is.EqualTo(lastEvent.LastPartitionOffset), "The offset should match."); | |||
| Assert.That(information.EnqueuedTime, Is.EqualTo(lastEvent.LastPartitionEnqueuedTime), "The last enqueue time should match."); | |||
| Assert.That(information.LastReceivedTime, Is.EqualTo(lastEvent.LastPartitionPropertiesRetrievalTime), "The retrieval time should match."); | |||
| Assert.That(mockConsumer.IsClosed, Is.False, "The consumer should not have been closed or disposed of."); | |||
There was a problem hiding this comment.
Why do we need to assert that? It doesn't feel like something we should worry about in this scenario.
There was a problem hiding this comment.
There was previously a difficult-to-detect issue where garbage collection was kicking in and the test was failing when the run was slow. Adding this check here is mostly unnecessary other than it offers an active reference to the variable, preventing the compiler from optimizing such that it may be collected.
I had originally left a comment with some silly does-nothing property access outside of an assert, but that looked and felt odd.
|
|
||
| foreach (var receivedMessage in receivedEvents.Where(item => item != null).Select(item => Encoding.UTF8.GetString(item.Body.ToArray()))) | ||
| { | ||
| Assert.That(sourceEventMessages.Contains(receivedMessage), $"The message: { receivedEvents } was not in the source set or has appeared more than oncesss."); |
There was a problem hiding this comment.
I'm going to take a note and defer this in order to avoid pushing just this change.
Summary
The focus of these changes is to tweak some of the timings and loosen some assumptions around the non-deterministic areas of the tests to offer better stability in the nightly runs, where there is a higher resource contention and timing tends to vary more than with local runs.
Last Upstream Rebase
Wednesday, December 18, 3:39pm (EST)
Related and Follow-Up Issues