Test refactoring #92
Test refactoring #92
Conversation
WalkthroughAdds deterministic test synchronization and per-room message tracking utilities to distributed Socket.IO tests; introduces automatic room joining from handshake URL parameters and a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedCommonTest.java (1)
880-891: Race condition: connect listeners registered after connect() is called.The
connect()calls on lines 880-881 are made before theEVENT_CONNECTlisteners are registered on lines 884-889. This creates a race condition where the connect event may fire before the listener is attached, causingconnectLatchto never count down.Apply this fix to register listeners before connecting:
+ a.on(Socket.EVENT_CONNECT, args -> { + connectLatch.countDown(); + }); + b.on(Socket.EVENT_CONNECT, args -> { + connectLatch.countDown(); + }); a.connect(); b.connect(); CountDownLatch joinLatch = new CountDownLatch(2); CountDownLatch connectLatch = new CountDownLatch(2); - a.on(Socket.EVENT_CONNECT, args -> { - connectLatch.countDown(); - }); - b.on(Socket.EVENT_CONNECT, args -> { - connectLatch.countDown(); - });
♻️ Duplicate comments (5)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastPubSubSingleChannelUnreliableTest.java (1)
90-113: Duplicated listener configuration across nodes and test files.The
get-my-roomshandler and connect listener logic is identical to the other Hazelcast test files. Consider the helper method extraction suggested inDistributedHazelcastRingBufferMultiChannelTest.javato reduce this repetition.Also applies to: 134-157
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastPubSubMultiChannelUnReliableTest.java (2)
56-58: Outdated comment references Redis instead of Hazelcast.// ------------------------------------------- - // Redis + Node Setup + // Hazelcast + Node Setup // -------------------------------------------
90-113: Duplicated listener configuration - same pattern as other test files.Same duplication pattern already noted. The helper method extraction would benefit all four Hazelcast test files.
Also applies to: 136-159
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferSingleChannelTest.java (2)
56-58: Outdated comment references Redis instead of Hazelcast.// ------------------------------------------- - // Redis + Node Setup + // Hazelcast + Node Setup // -------------------------------------------
90-113: Duplicated listener configuration across nodes.Same duplication pattern as the other Hazelcast test files. Consider the shared helper method approach.
Also applies to: 135-158
🧹 Nitpick comments (13)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java (1)
131-154: Consider extracting duplicated listener setup to reduce maintenance burden.The event listeners here are identical to node1's setup (lines 86-109). While duplication in test setup is common, extracting this logic to a helper method would make future updates easier and reduce the risk of divergence between nodes.
Example refactor:
private void setupTestListeners(SocketIOServer node) { node.addEventListener("get-my-rooms", String.class, (client, data, ackSender) -> { if (ackSender.isAckRequested()) { ackSender.sendAckData(client.getAllRooms()); } }); node.addConnectListener(client -> { Map<String, List<String>> params = client.getHandshakeData().getUrlParams(); List<String> joinParams = params.get("join"); if (joinParams == null || joinParams.isEmpty()) { return; } Set<String> rooms = joinParams.stream() .flatMap(v -> Arrays.stream(v.split(","))) .map(String::trim) .filter(s -> !s.isEmpty()) .collect(Collectors.toSet()); rooms.forEach(client::joinRoom); }); }Then call
setupTestListeners(node1)andsetupTestListeners(node2)in the respective setup sections.netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonPubSubMultiChannelUnReliableTest.java (2)
85-89: Test helper looks good, but consider removing the unused parameter.The get-my-rooms event handler correctly returns the client's room membership via ACK. However, the
dataparameter is declared but never used.If the event doesn't require data, consider using
Object.classand naming it more explicitly, or documenting why it's present:- node1.addEventListener("get-my-rooms", String.class, (client, data, ackSender) ->{ + node1.addEventListener("get-my-rooms", String.class, (client, ignored, ackSender) ->{ if (ackSender.isAckRequested()){ ackSender.sendAckData(client.getAllRooms()); } });
130-153: Consider extracting common event listener setup to reduce duplication.The get-my-rooms event listener and connect listener logic is identical between node1 and node2. While duplication in test setup is sometimes acceptable for clarity, extracting to a helper method could improve maintainability.
Consider extracting the common setup:
private void configureTestEventListeners(SocketIOServer node) { node.addEventListener("get-my-rooms", String.class, (client, ignored, ackSender) -> { if (ackSender.isAckRequested()) { ackSender.sendAckData(client.getAllRooms()); } }); node.addConnectListener(client -> { Map<String, List<String>> params = client.getHandshakeData().getUrlParams(); List<String> joinParams = params.get("join"); if (joinParams == null || joinParams.isEmpty()) { return; } Set<String> rooms = joinParams.stream() .flatMap(v -> Arrays.stream(v.split(","))) .map(String::trim) .filter(s -> !s.isEmpty()) .collect(Collectors.toSet()); rooms.forEach(client::joinRoom); }); }Then call
configureTestEventListeners(node1)andconfigureTestEventListeners(node2)after creating each node.netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonPubSubSingleChannelUnreliableTest.java (1)
130-153: LGTM with optional refactor suggestion.The get-my-rooms handler and connect listener for node2 are identical to node1 and implement the same test infrastructure correctly.
While the duplication is intentional for independent node setup, consider extracting the common logic into helper methods if this pattern continues to expand.
Example refactor:
private void registerGetMyRoomsHandler(SocketIOServer node) { node.addEventListener("get-my-rooms", String.class, (client, data, ackSender) -> { if (ackSender.isAckRequested()) { ackSender.sendAckData(client.getAllRooms()); } }); } private void registerAutoJoinListener(SocketIOServer node) { node.addConnectListener(client -> { Map<String, List<String>> params = client.getHandshakeData().getUrlParams(); List<String> joinParams = params.get("join"); if (joinParams == null || joinParams.isEmpty()) { return; } Set<String> rooms = joinParams.stream() .flatMap(v -> Arrays.stream(v.split(","))) .map(String::trim) .filter(s -> !s.isEmpty()) .collect(Collectors.toSet()); rooms.forEach(client::joinRoom); }); }Then in setup:
registerGetMyRoomsHandler(node1); registerAutoJoinListener(node1); // ... later ... registerGetMyRoomsHandler(node2); registerAutoJoinListener(node2);netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java (1)
133-156: Logic is correct; consider extracting shared handlers.The node2 handlers are identical to node1 and work correctly. However, the ~50 lines of duplicated logic between the two nodes could be extracted into helper methods (e.g.,
createGetMyRoomsHandler()andcreateAutoJoinConnectListener()) to improve maintainability and reduce the risk of divergence during future updates.Example refactor:
+ private void registerCommonHandlers(SocketIOServer node) { + node.addEventListener("get-my-rooms", String.class, (client, data, ackSender) -> { + if (ackSender.isAckRequested()) { + ackSender.sendAckData(client.getAllRooms()); + } + }); + + node.addConnectListener(client -> { + Map<String, List<String>> params = client.getHandshakeData().getUrlParams(); + List<String> joinParams = params.get("join"); + if (joinParams == null || joinParams.isEmpty()) { + return; + } + + Set<String> rooms = joinParams.stream() + .flatMap(v -> Arrays.stream(v.split(","))) + .map(String::trim) + .filter(s -> !s.isEmpty()) + .collect(Collectors.toSet()); + + rooms.forEach(client::joinRoom); + }); + }Then call
registerCommonHandlers(node1);andregisterCommonHandlers(node2);after setting up the join/leave room event listeners for each node.netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonReliableSingleChannelTest.java (2)
85-108: Extract duplicated event and connect listeners into a helper method.The
get-my-roomsevent listener and connect listener logic are duplicated identically between node1 and node2. Since this PR focuses on test refactoring, consider extracting this setup into a reusable helper method to improve maintainability and reduce duplication.Apply this pattern:
+ private void setupNodeEventListeners(SocketIOServer node) { + node.addEventListener("get-my-rooms", String.class, (client, data, ackSender) -> { + if (ackSender.isAckRequested()) { + ackSender.sendAckData(client.getAllRooms()); + } + }); + + node.addConnectListener(client -> { + Map<String, List<String>> params = client.getHandshakeData().getUrlParams(); + List<String> joinParams = params.get("join"); + if (joinParams == null || joinParams.isEmpty()) { + return; + } + + Set<String> rooms = joinParams.stream() + .flatMap(v -> Arrays.stream(v.split(","))) + .map(String::trim) + .filter(s -> !s.isEmpty()) + .collect(Collectors.toSet()); + + rooms.forEach(client::joinRoom); + }); + }Then replace both occurrences with:
- node1.addEventListener("get-my-rooms", String.class, (client, data, ackSender) ->{ - if (ackSender.isAckRequested()){ - ackSender.sendAckData(client.getAllRooms()); - } - }); - node1.addConnectListener(client -> { - ... - }); + setupNodeEventListeners(node1);- node2.addEventListener("get-my-rooms", String.class, (client, data, ackSender) ->{ - if (ackSender.isAckRequested()){ - ackSender.sendAckData(client.getAllRooms()); - } - }); - node2.addConnectListener(client -> { - ... - }); + setupNodeEventListeners(node2);Also applies to: 130-153
85-89: Consider usingObject.classfor unused event data.The
get-my-roomsevent listener declaresString.classbut never uses thedataparameter. Since the event acts as a trigger rather than passing data, consider usingObject.classfor clarity, though this is a minor point.netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonReliableMultiChannelTest.java (2)
85-89: Unused parameter in event handler.The
dataparameter is declared asString.classbut never used in the handler logic. Consider usingObject.classorVoid.classif no data is expected, or document why a String type is required.Apply this diff if no data is expected:
- node1.addEventListener("get-my-rooms", String.class, (client, data, ackSender) ->{ + node1.addEventListener("get-my-rooms", Object.class, (client, data, ackSender) -> {
131-154: Consider extracting duplicated node setup logic.The event handler and connect listener for node2 (lines 131-154) are identical to node1 (lines 85-108). While duplication in test setup is common, extracting these to a helper method would improve maintainability.
Example helper method:
private void registerTestEventHandlers(SocketIOServer node) { node.addEventListener("get-my-rooms", Object.class, (client, data, ackSender) -> { if (ackSender.isAckRequested()) { ackSender.sendAckData(client.getAllRooms()); } }); node.addConnectListener(client -> { Map<String, List<String>> params = client.getHandshakeData().getUrlParams(); List<String> joinParams = params.get("join"); if (joinParams == null || joinParams.isEmpty()) { return; } Set<String> rooms = joinParams.stream() .flatMap(v -> Arrays.stream(v.split(","))) .map(String::trim) .filter(s -> !s.isEmpty()) .collect(Collectors.toSet()); rooms.forEach(client::joinRoom); }); }Then call
registerTestEventHandlers(node1)andregisterTestEventHandlers(node2)after server creation.netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferMultiChannelTest.java (2)
90-113: Consider extracting duplicated node configuration logic.The
get-my-roomsevent listener and the connect listener for auto-joining rooms are duplicated verbatim for bothnode1andnode2. This pattern is also repeated across all four Hazelcast test files in this PR.Consider extracting a helper method to reduce duplication:
private void configureNodeListeners(SocketIOServer node) { node.addEventListener("get-my-rooms", String.class, (client, data, ackSender) -> { if (ackSender.isAckRequested()) { ackSender.sendAckData(client.getAllRooms()); } }); node.addConnectListener(client -> { Map<String, List<String>> params = client.getHandshakeData().getUrlParams(); List<String> joinParams = params.get("join"); if (joinParams == null || joinParams.isEmpty()) { return; } Set<String> rooms = joinParams.stream() .flatMap(v -> Arrays.stream(v.split(","))) .map(String::trim) .filter(s -> !s.isEmpty()) .collect(Collectors.toSet()); rooms.forEach(client::joinRoom); }); }This could be placed in
DistributedCommonTestand called from each subclass'ssetup()method.Also applies to: 135-158
56-58: Outdated comment references Redis instead of Hazelcast.The comment says "Redis + Node Setup" but this test class uses Hazelcast. Consider updating for clarity.
// ------------------------------------------- - // Redis + Node Setup + // Hazelcast + Node Setup // -------------------------------------------netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedCommonTest.java (1)
479-495: Room sync check may be incorrect for distributed scenarios.The
awaitRoomSyncmethod sums the client counts from both nodes:int count = node1.getRoomOperations(room).getClients().size() + node2.getRoomOperations(room).getClients().size();In a distributed setup, each client connects to exactly one node. If 2 clients connect (one to each node), the expected count should be 2, and summing gives 2 - this works. However, if 2 clients both connect to node1, you'd get count=2 from node1 alone, which also works.
The issue is if the expectation is that both nodes should see all clients (via distributed sync), this check doesn't validate that. Consider whether you need to verify that each node's view is eventually consistent.
Also,
Thread.sleep(1)is very aggressive polling. Consider using a slightly larger interval (e.g., 10-50ms) to reduce CPU churn during tests.- Thread.sleep(1); + Thread.sleep(50);netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastPubSubSingleChannelUnreliableTest.java (1)
56-58: Outdated comment references Redis instead of Hazelcast.Same issue as in other Hazelcast test files - the comment says "Redis + Node Setup" but this uses Hazelcast.
// ------------------------------------------- - // Redis + Node Setup + // Hazelcast + Node Setup // -------------------------------------------
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedCommonTest.java(12 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastPubSubMultiChannelUnReliableTest.java(3 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastPubSubSingleChannelUnreliableTest.java(3 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferMultiChannelTest.java(3 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferSingleChannelTest.java(3 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonPubSubMultiChannelUnReliableTest.java(3 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonPubSubSingleChannelUnreliableTest.java(3 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonReliableMultiChannelTest.java(3 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonReliableSingleChannelTest.java(3 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java(3 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (21) / build
- GitHub Check: build (17) / build
- GitHub Check: build (25) / build
- GitHub Check: qodana
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (15)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java (2)
20-24: LGTM! Necessary imports for the new test infrastructure.All imports are properly utilized in the new event handlers and connect listeners.
86-109: Well-structured test event handlers with proper defensive coding.The
get-my-roomshandler correctly checks for ACK requests before responding, and the connect listener implements robust URL parameter parsing with deduplication and comma-separated value support. The logic is sound with appropriate null checks and filtering.netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonPubSubMultiChannelUnReliableTest.java (2)
20-24: LGTM! Necessary imports for the new test infrastructure.The imports are all used in the URL parameter parsing and room deduplication logic.
90-108: Excellent test infrastructure for auto-joining rooms from URL parameters.The implementation correctly handles:
- Comma-separated room names (join=a,b)
- Multiple join parameters (join=a&join=b)
- Deduplication via Set
- Whitespace trimming and empty string filtering
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonPubSubSingleChannelUnreliableTest.java (3)
20-24: LGTM!The added imports are appropriate for the new test infrastructure functionality (URL parameter parsing, room deduplication, and stream operations).
85-89: LGTM!The get-my-rooms handler correctly responds with the client's current rooms when an acknowledgment is requested, enabling tests to verify room membership.
90-108: LGTM!The connect listener correctly implements automatic room joining from handshake URL parameters. The logic handles comma-separated values, deduplicates via Set, and filters empty strings appropriately.
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java (2)
19-24: LGTM!The new imports support the URL parameter processing and stream-based room joining logic added in the connect listeners.
87-110: LGTM!The get-my-rooms event handler and connect listener for node1 are well-implemented:
- The ACK request check prevents unnecessary data transmission.
- The URL parameter parsing robustly handles null/empty cases, comma-separated values, whitespace trimming, and deduplication via Set.
- Early return pattern keeps the logic clean.
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonReliableSingleChannelTest.java (2)
20-24: LGTM!The added imports are all necessary for the new test helper functionality.
90-108: LGTM! The connect listener logic handles edge cases well.The URL parameter parsing correctly handles:
- Null/empty checks
- Comma-separated room names
- Whitespace trimming
- Empty string filtering
- Deduplication via Set
This provides solid test infrastructure for auto-joining rooms via handshake parameters.
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonReliableMultiChannelTest.java (2)
20-24: LGTM!The import additions are necessary for the new URL parameter parsing and room auto-join functionality.
90-108: LGTM with minor observation.The URL parameter-based room joining logic is well-structured with proper null checks, deduplication, and filtering. The comma-split support is a nice touch for usability.
Note: This logic is duplicated identically for node2. Consider extracting to a helper method if this pattern appears in additional test classes.
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedCommonTest.java (2)
394-477: Well-structured test with proper isolation and synchronization.The refactoring introduces:
- UUID-based room naming to prevent inter-test interference
- Clear early/late message semantics with separate tracking
- Proper use of
awaitRoomSyncbefore assertionsThis addresses non-deterministic timing issues in the original test.
811-869: Good test structure with proper listener registration order.This test correctly registers the
EVENT_CONNECTlisteners before callingconnect(), and usesCompletableFuturefor coordinating async assertions onget-my-roomsresponses.
Qodana for JVM61 new problems were found
☁️ View the detailed Qodana report Detected 123 dependenciesThird-party software listThis page lists the third-party software dependencies used in project
Contact Qodana teamContact us at qodana-support@jetbrains.com
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedCommonTest.java (3)
217-217: Clarify the 256ms timeout choice.The timeout was changed from 1000ms to 256ms. While this specific value (256 = 2^8) may be intentional, it seems arbitrary for a negative assertion. Consider documenting why 256ms is sufficient or using a rounder value like 200ms or 500ms for clarity.
485-501: Helper method improves test determinism.The
awaitRoomSyncmethod provides a reliable barrier to ensure distributed room membership is synchronized before assertions. The 5-second timeout and polling approach are appropriate for integration tests.Consider slightly longer sleep intervals
The
Thread.sleep(1)creates a very tight polling loop. ConsiderThread.sleep(10)orThread.sleep(50)to reduce CPU usage while still providing fast synchronization:- Thread.sleep(1); + Thread.sleep(10);This is a minor optimization and the current implementation is acceptable for tests.
716-718: Reconsider the Thread.sleep after awaitRoomSync.Line 718 adds a
Thread.sleep(200)afterawaitRoomSync, which seems contradictory. The comment "allow cross-node adapter propagation" suggests uncertainty about whetherawaitRoomSyncalone is sufficient.If
awaitRoomSynccorrectly waits for distributed room state, the additional sleep should be unnecessary. If it's needed, consider:
- Increasing the timeout or polling logic in
awaitRoomSyncitself- Documenting why the additional delay is required despite synchronization
Does the test pass reliably without the 200ms sleep? If so, remove it to avoid unnecessary delays and maintain consistency with other tests.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedCommonTest.java(24 hunks)pom.xml(0 hunks)
💤 Files with no reviewable changes (1)
- pom.xml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (17) / build
- GitHub Check: build (21) / build
- GitHub Check: build (25) / build
- GitHub Check: qodana
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (13)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedCommonTest.java (13)
24-30: LGTM! Imports support thread-safe refactoring.The new imports (UUID, CompletableFuture, ConcurrentHashMap, AtomicReferenceArray, fail) align with the test refactoring goals to introduce deterministic synchronization and thread-safe data structures.
Also applies to: 48-48
147-149: LGTM! Thread-safe message tracking with AtomicReferenceArray.Replacing the plain array with
AtomicReferenceArraycorrectly ensures thread-safe updates from concurrent listener callbacks.Also applies to: 189-189, 195-195, 201-201, 207-207, 218-221
180-180: Good addition of deterministic room synchronization.The
awaitRoomSynccall ensures both clients are registered in the room before broadcasting, eliminating race conditions.
250-253: LGTM! Appropriate use of thread-safe sets.Using
ConcurrentHashMap.newKeySet()is correct for thread-safe set operations from concurrent listeners. The semantic change from List to Set also better represents the test's intent to verify unique messages.
289-289: Good synchronization before broadcast.Adding
awaitRoomSync("room1", 4)ensures all clients are registered before the broadcast, improving test determinism.
318-319: LGTM! Consistent thread-safe refactoring.The conversion to
AtomicReferenceArraymaintains consistency with other tests and ensures thread-safe message storage.Also applies to: 346-346, 350-350, 356-357, 366-367, 373-373, 383-383, 385-385
398-483: Excellent refactoring for test isolation and determinism.Key improvements:
- UUID-based room naming (line 398) prevents cross-test interference
- Separate tracking structures (
earlyLatch,lateLatch,joinMsg,roomMsg) provide clear per-phase message tracking- Refactored listeners avoid
off()calls and explicitly handle "early" vs "late" messagesawaitRoomSync(line 472) ensures distributed room state is consistent before the late broadcastThese changes significantly improve test reliability and maintainability.
514-515: LGTM! Consistent thread-safe refactoring.The
AtomicReferenceArrayusage maintains consistency across the test suite.Also applies to: 528-528, 532-532, 550-551
579-582: LGTM! Thread-safety and synchronization for multi-room test.The refactoring ensures both rooms are properly synchronized before broadcasts and uses thread-safe message storage for concurrent listeners.
Also applies to: 603-604, 613-613, 621-622, 625-625, 629-629, 636-637
650-715: LGTM! Improved thread-safety and code clarity.The refactoring to
ConcurrentHashMap.newKeySet()andawaitRoomSyncimproves thread-safety and test determinism. The formatting changes enhance readability.Also applies to: 720-742
774-833: LGTM! Thread-safe two-phase broadcast test.The
AtomicReferenceArrayusage ensures thread-safe message tracking across two sequential broadcast phases.
845-901: Excellent refactoring with CompletableFuture-based assertions.Key improvements:
- UUID-based room naming (lines 845-846) prevents cross-test contamination
awaitRoomSynccalls (lines 861-862) ensure deterministic room stateCompletableFuturecoordination (lines 865-866, 896-898) provides better error propagation thanCountDownLatchalone- Exceptions in ACK callbacks are now properly captured via
completeExceptionallyThis pattern significantly improves test reliability and error visibility.
907-920: LGTM! Consistent refactoring with CompletableFuture-based assertions.The refactoring mirrors the improvements in
testConnectAndJoinDifferentRoomTest, providing UUID-based room isolation, deterministic synchronization, and robust error handling throughCompletableFuture.Also applies to: 922-959
| b.on(Socket.EVENT_CONNECT, args -> { | ||
| connectLatch.countDown(); | ||
| }); | ||
| awaitRoomSync(room, 2); |
There was a problem hiding this comment.
Fix indentation.
Line 921 is missing one space of indentation compared to surrounding lines.
Apply this diff to fix indentation:
- awaitRoomSync(room, 2);
+ awaitRoomSync(room, 2);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| awaitRoomSync(room, 2); | |
| awaitRoomSync(room, 2); |
🤖 Prompt for AI Agents
In
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedCommonTest.java
around line 921, the statement awaitRoomSync(room, 2); is indented one space
less than surrounding lines; fix by adding the missing leading space so the
line's indentation matches the block context (align with adjacent lines), then
run formatter/checkstyle to ensure consistent indentation across the file.
|
Seems qodana's condifuration is for single person, if another one open a pr, it would fail |
|
like me, like denpendabot, our PRs are all failed |
#90 - your PR works well, only bots PR problem as they cant access secret, we can manually check and approve bot PRs |
Description
Brief description of the changes in this PR.
Type of Change
Related Issue
Closes #(issue number)
Changes Made
Testing
mvn testChecklist
Additional Notes
Any additional information, screenshots, or context that reviewers should know.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.