-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Ensure serverHeartbeatEvent is sent before opening a connection #1715
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
Conversation
Added Heartbeat prose test Migrated DefaultServerMonitorSpecification to JUnit 5. JAVA-5230
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.
Pull Request Overview
This PR migrates the legacy Groovy DefaultServerMonitorSpecification
to JUnit 5 tests and refines the heartbeat-started event emission in DefaultServerMonitor
.
- Introduced
DefaultServerMonitorTest.java
with new JUnit 5 tests covering success, failure, and ordering of heartbeat events before socket connect. - Removed the old
DefaultServerMonitorSpecification.groovy
. - Updated
DefaultServerMonitor.java
to add aVisibleForTesting
accessor and de-duplicateserverHeartbeatStarted
events using a new flag.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
driver-core/src/test/unit/com/mongodb/internal/connection/DefaultServerMonitorTest.java | Added JUnit 5 tests for heartbeat events and ordering before connection open |
driver-core/src/test/unit/com/mongodb/internal/connection/DefaultServerMonitorSpecification.groovy | Removed legacy Groovy-based spec after migrating tests to JUnit 5 |
driver-core/src/main/com/mongodb/internal/connection/DefaultServerMonitor.java | Added getServerMonitor() for testing, alreadyLoggedHeartBeatStarted flag, and refined heartbeat-started logic |
Comments suppressed due to low confidence (2)
driver-core/src/main/com/mongodb/internal/connection/DefaultServerMonitor.java:161
- [nitpick] Consider renaming 'alreadyLoggedHeartBeatStarted' to 'alreadyLoggedHeartbeatStarted' to follow consistent camelCase for the term 'heartbeat'.
private volatile boolean alreadyLoggedHeartBeatStarted = false;
driver-core/src/main/com/mongodb/internal/connection/DefaultServerMonitor.java:224
- Add a unit test that simulates multiple heartbeat cycles to verify the 'alreadyLoggedHeartBeatStarted' flag behavior and ensure only one 'ServerHeartbeatStartedEvent' is emitted per cycle.
boolean shouldStreamResponses = shouldStreamResponses(currentServerDescription);
driver-core/src/test/unit/com/mongodb/internal/connection/DefaultServerMonitorTest.java
Outdated
Show resolved
Hide resolved
driver-core/src/main/com/mongodb/internal/connection/DefaultServerMonitor.java
Show resolved
Hide resolved
driver-core/src/test/unit/com/mongodb/internal/connection/DefaultServerMonitorTest.java
Outdated
Show resolved
Hide resolved
…ultServerMonitorTest.java
driver-core/src/test/unit/com/mongodb/internal/connection/DefaultServerMonitorTest.java
Show resolved
Hide resolved
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.
LGTM!
@@ -166,7 +166,7 @@ public void serverDescriptionChanged(final ServerDescriptionChangedEvent event) | |||
* <a href="https://github.com/mongodb/specifications/blob/master/source/server-discovery-and-monitoring/server-discovery-and-monitoring-tests.md#connection-pool-management">Connection Pool Management</a>. | |||
*/ | |||
@Test | |||
@Ignore | |||
@Ignore("JAVA-4484 - events are not guaranteed to be delivered in order") |
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.
👍
Added Heartbeat prose test
Migrated DefaultServerMonitorSpecification to JUnit 5.
JAVA-5230