Skip to content

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rozza
Copy link
Member

@rozza rozza commented May 22, 2025

Added Heartbeat prose test
Migrated DefaultServerMonitorSpecification to JUnit 5.

JAVA-5230

Added Heartbeat prose test
Migrated DefaultServerMonitorSpecification to JUnit 5.

JAVA-5230
@rozza rozza requested review from Copilot, a team and katcharov and removed request for a team May 22, 2025 09:18
Copy link

@Copilot Copilot AI left a 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 a VisibleForTesting accessor and de-duplicate serverHeartbeatStarted 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);

@rozza rozza removed the request for review from katcharov May 22, 2025 10:56
@rozza rozza marked this pull request as draft May 22, 2025 10:56
@rozza rozza requested a review from katcharov May 22, 2025 12:23
@rozza rozza marked this pull request as ready for review May 22, 2025 12:23
assertTrue(latch.await(5, TimeUnit.SECONDS), "Timed out waiting for heartbeat");

// Then
List<String> expectedEvents = asList("serverHeartbeatStartedEvent", "client connected", "client hello received", "serverHeartbeatFailedEvent");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test method corresponds to https://github.com/mongodb/specifications/pull/1483/files#diff-af358d9f2fc605678592489c97e6d557b5fa0fcb447f6619d503dc7a62c1ee71R268

But what do the other methods correspond to? Should this method be part of a ...ProseTest.java class?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants