JAVA-6174 Fix sdam eventType to assert serverDescriptionChangedEvent#1978
Conversation
- 3-bucket partition in UnifiedTest.java - ServerDescriptionChangedEvent support in EventMatcher/ContextElement - waitForEvent connectionClosedEvent support - Unskip backpressure-server-description-unchanged spec test
There was a problem hiding this comment.
Pull request overview
Updates the unified test runner’s SDAM event handling so serverDescriptionChangedEvent is recognized and asserted separately from heartbeat events, enabling previously skipped SDAM/backpressure spec coverage to run.
Changes:
- Unskips the SDAM backpressure spec test
backpressure-server-description-unchanged-on-min-pool-size-population-error. - Refactors SDAM
expectEventscomparison to 3-way partition expected events into topology vsserverDescriptionChangedEventvs heartbeat events. - Extends event type normalization and context rendering to support
ServerDescriptionChangedEvent.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestModifications.java | Removes the skip for the backpressure SDAM spec file so it runs again. |
| driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java | Partitions SDAM expected events into topology / serverDescriptionChanged / heartbeat and asserts them via the appropriate listeners. |
| driver-sync/src/test/functional/com/mongodb/client/unified/EventMatcher.java | Broadens getEventType to include all Server* events and adds a note about awaited matching limitations. |
| driver-sync/src/test/functional/com/mongodb/client/unified/ContextElement.java | Ensures ServerDescriptionChangedEvent can be rendered without assuming an awaited field. |
Comments suppressed due to low confidence (1)
driver-sync/src/test/functional/com/mongodb/client/unified/EventMatcher.java:528
serverMonitorEventMatchesis now also exercised forServerDescriptionChangedEvent(via SDAM event comparisons), but it does not validateserverDescriptionChangedEvent-specific contents likenewDescription(it only checksawaited). This makes SDAM event assertions weaker than the existingserverDescriptionChangedEventMatchesused bywaitForEvent/assertEventCount. Consider either extending this matcher to handleServerDescriptionChangedEvent(including content validation) or ensuringServerDescriptionChangedEventexpectations are routed to a separate matcher that usesserverDescriptionChangedEventMatches.
private static <T> boolean serverMonitorEventMatches(
final BsonDocument expectedEventContents,
final T event,
@Nullable final AssertionContext context) {
if (expectedEventContents.size() > 1) {
throw new UnsupportedOperationException("Matching for the following event is not implemented " + expectedEventContents.toJson());
}
// TODO JAVA-6174: 'awaited' matching is only supported for ServerHeartbeat* events; a non-empty
// 'serverDescriptionChangedEvent' with an 'awaited' field will fail in getAwaitedFromServerMonitorEvent.
if (expectedEventContents.containsKey("awaited")) {
boolean expectedAwaited = expectedEventContents.getBoolean("awaited").getValue();
boolean actualAwaited = getAwaitedFromServerMonitorEvent(event);
boolean awaitedMatches = expectedAwaited == actualAwaited;
if (context != null) {
assertTrue(context.getMessage("Expected `awaited` to match"), awaitedMatches);
}
return awaitedMatches;
}
return true;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Route ServerDescriptionChangedEvent to serverDescriptionChangedEventMatches so 'newDescription' content is actually verified and 'awaited' on a non-heartbeat event no longer crashes in getAwaitedFromServerMonitorEvent.
| } else if (eventClassName.startsWith("Connection")) { | ||
| return eventClassName.replace("Connection", "connection"); | ||
| } else if (eventClassName.startsWith("ServerHeartbeat")) { | ||
| } else if (eventClassName.startsWith("Server")) { |
There was a problem hiding this comment.
broadens the event-type mapping from only heartbeat events to all server events.
nhachicha
changed the title
rozza
left a comment
rozza
left a comment
There was a problem hiding this comment.
My code review picked up:
Broadening startsWith("ServerHeartbeat") → startsWith("Server") now matches ServerOpeningEvent, ServerClosedEvent, and ServerDescriptionChangedEvent. While the transformation (lowercase first char) produces valid camelCase names for all, this changes fallback behavior from throwing UnsupportedOperationException to silently producing an event type string for events not previously handled. If ServerOpeningEvent or ServerClosedEvent ever accidentally flow through this path, the test would silently produce incorrect assertions rather than failing fast.
I think thats probably a caveat to a lot of the unified test code. Its potentially fragile to future changes.
So LGTM - the only concerns I have are about the complexity of this code in general!
JAVA-6174