CSHARP-4988: Fix flaky ServerMonitor test #1277
Conversation
| mockConnection.EnqueueCommandResponseMessage(CreateHeartbeatCommandResponseMessage(), null); | ||
| mockConnection.EnqueueCommandResponseMessage(CreateHeartbeatCommandResponseMessage(), null); | ||
| // Have to setup small delay on the heartbeat responses to emulate streamableServer functionality. | ||
| mockConnection.EnqueueCommandResponseMessage(CreateHeartbeatCommandResponseMessage(), TimeSpan.FromMilliseconds(500)); |
There was a problem hiding this comment.
Is it possible to reduce the delay a bit more to like maybe 100ms?
There was a problem hiding this comment.
I think it would be great if could not rely on timings at all, and then have much lower intervals like in the rest of tests (5ms).
There was a problem hiding this comment.
So after additional discussion with Boris we decided to remove delays at all, as well as verification of the Start method was called after the very first heartbeat (because the way we detecting the first heartbeat was done is not that reliable)
| @@ -307,9 +307,10 @@ public void RoundTripTimeMonitor_should_be_started_only_once_if_using_streaming_ | |||
| var subject = CreateSubject(out var mockConnection, out _, out var mockRoundTripTimeMonitor, capturedEvents, serverMonitorSettings: serverMonitorSettings); | |||
There was a problem hiding this comment.
Also I think you might want remove the serverMonitorSettings variable before this. Looking at the code I don't think it is needed since we will be streaming heartbeats with the server.
There was a problem hiding this comment.
Boris pointed to the fact that we still need this to override the default settings.
| @@ -303,23 +303,17 @@ public void InitializeHelloProtocol_should_use_streaming_protocol_when_available | |||
| public void RoundTripTimeMonitor_should_be_started_only_once_if_using_streaming_protocol() | |||
| { | |||
| var capturedEvents = new EventCapturer().Capture<ServerHeartbeatSucceededEvent>(); | |||
| var serverMonitorSettings = new ServerMonitorSettings(TimeSpan.FromSeconds(5), TimeSpan.FromMilliseconds(10)); | |||
There was a problem hiding this comment.
I think we still need this to override the higher default interval.
There was a problem hiding this comment.
OK, will revert this =)
No description provided.