Fix test bug in ClientDisconnectionTest and remove useless testcontainers in integration tests

[NeatGuyCoding]

Description

Fix test bug in ClientDisconnectionTest and remove useless testcontainers in integration tests

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Code refactoring
• Test improvements
• Build/tooling changes

Related Issue

Closes #4

Changes Made

Testing

• All existing tests pass
• New tests added for new functionality
• Tests pass locally with mvn test
• Integration tests pass (if applicable)

Checklist

• Code follows project coding standards
• Self-review completed
• Code is commented where necessary
• Documentation updated (if needed)
• Commit messages follow conventional format
• No merge conflicts
• All CI checks pass

Additional Notes

Any additional information, screenshots, or context that reviewers should know.

Summary by CodeRabbit

• Bug Fixes

  • Improved error handling for socket operations and scheduler cancellations to prevent uncaught exceptions
  • Added debug logging for client disconnections to improve diagnostics

• Tests

  • Replaced external service dependency with dynamic port allocation and startup retry logic
  • Simplified test setup by removing external service containers
  • Added a short stabilization delay in a client disconnection test
  • Made broadcast tests more resilient by stringifying and substring-checking received payloads

[coderabbitai]

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Added defensive try/catch logging around scheduler/channel operations in ClientHead, added debug logging on client disconnect in Namespace, refactored integration tests to remove Redis/Redisson and use dynamic port allocation with startup retry, and added a short stabilization delay in ClientDisconnectionTest.

Changes

Cohort / File(s)	Summary
Defensive Error Handling netty-socketio-core/src/main/java/com/corundumstudio/socketio/handler/ClientHead.java	releasePollingChannel, cancelPing, and cancelPingTimeout now wrap scheduler cancel and channel cleanup operations in try-catch blocks and log errors on Throwable instead of propagating.
Logging Enhancement netty-socketio-core/src/main/java/com/corundumstudio/socketio/namespace/Namespace.java	Added SLF4J Logger/LoggerFactory and emits a debug log when a client disconnects (includes sessionId and namespace name).
Test Infrastructure Refactor netty-socketio-core/src/test/java/com/corundumstudio/socketio/integration/AbstractSocketIOIntegrationTest.java	Removed Redis/Redisson fields and helper methods; replaced container-based store setup with dynamic port allocation and a startup retry loop (findAvailablePort/allocatePort); removed Redis init/teardown and added getServerPort()/getServerHost() helpers.
Test Stabilization netty-socketio-core/src/test/java/com/corundumstudio/socketio/integration/ClientDisconnectionTest.java	Added a 3-second sleep after confirming a connected client to allow connection stabilization and protocol upgrade before disconnecting.
Test Assertion Robustness netty-socketio-core/src/test/java/com/corundumstudio/socketio/integration/RoomBroadcastTest.java	Switched to stringifying and joining event arguments (Arrays.stream(...).map(Object::toString).collect(...)) and replaced strict equality checks with substring containment assertions to tolerate polling vs websocket differences.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Test as Integration Test
  participant Alloc as Port Allocator
  participant Server as SocketIO Server

  rect rgb(240,248,255)
  Note over Test,Alloc: Startup with dynamic port selection & retry
  Test->>Alloc: findAvailablePort()
  alt port available
    Alloc-->>Test: port
    Test->>Server: start(port)
    alt start success
      Server-->>Test: started
    else start failed
      Server-->>Test: start failure
      Test->>Test: sleep and retry
    end
  else no port
    Alloc-->>Test: none
    Test->>Test: sleep and retry
  end
  end

Loading

sequenceDiagram
  autonumber
  participant Client as ClientHead
  participant Scheduler as Scheduler

  rect rgb(245,245,255)
  Note over Client,Scheduler: Defensive cancellation and cleanup
  Client->>Scheduler: cancel(pingTask)
  alt cancel throws
    Scheduler-->>Client: Throwable
    Client-->>Client: log error (swallow)
  else cancel ok
    Scheduler-->>Client: cancelled
  end
  Client->>Client: releasePollingChannel() wrapped in try-catch
  end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

• Inspect AbstractSocketIOIntegrationTest.java closely: port-allocation/retry correctness, race conditions, and interactions with other tests.
• Verify ClientHead.java logging does not hide critical failures and includes enough context.
• Check Namespace.java log level and message format to avoid excessive CI noise.
• Review RoomBroadcastTest and ClientDisconnectionTest for potential flakiness introduced by timing or relaxed assertions.

Poem

🐰 I nibble logs and hop through ports at night,

I catch the falls and keep the server light.
A gentle retry, a pause to mend,
Safe cancels and a debug friend.
Hops of code — all stitched up tight.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 2 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check	❓ Inconclusive	The PR description lacks specific details in the 'Changes Made' section, which is left empty despite multiple files being modified.	Fill in the 'Changes Made' section with bullet points describing the specific modifications, such as the added error handling in ClientHead, logging in Namespace, and testcontainer removal details.
Out of Scope Changes check	❓ Inconclusive	Changes to ClientHead and Namespace appear to be scope-related error handling improvements, but unclear if they are directly required by issue #4 or are incidental.	Clarify whether the error handling additions in ClientHead and Namespace logging are necessary to fix the build failures, or if they are separate enhancements.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main changes: fixing a test bug and removing unnecessary testcontainers from integration tests.
Linked Issues check	✅ Passed	The PR addresses the build failure issue #4 by fixing test bugs and removing flaky testcontainer dependencies that likely caused CI failures.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch hotfix-disconnect-is-not-called-bug-1118

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (2)

netty-socketio-core/src/test/java/com/corundumstudio/socketio/integration/AbstractSocketIOIntegrationTest.java (1)

42-47: Javadoc still mentions Redis container management

The class-level Javadoc advertises “Automatic Redis container management”, but this implementation no longer initializes or manages Redis in setUp/tearDown. It would be good to update the doc comment to reflect the current responsibilities (dynamic port allocation, server lifecycle, helper methods).

netty-socketio-core/src/test/java/com/corundumstudio/socketio/integration/ClientDisconnectionTest.java (1)

75-76: Consider a more deterministic sync point than a fixed 3s sleep

The extra 3-second sleep will help with flakiness around connection/upgrade, but it also slows the test suite and still relies on timing rather than a concrete signal.

If possible, consider waiting on a specific event (e.g., a listener that fires after the upgrade you care about) or a shorter timeout with a latch instead of a hard sleep(3), so the test stays fast while remaining stable.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c4de874 and fa895ab.

📒 Files selected for processing (4)

• netty-socketio-core/src/main/java/com/corundumstudio/socketio/handler/ClientHead.java (2 hunks)
• netty-socketio-core/src/main/java/com/corundumstudio/socketio/namespace/Namespace.java (3 hunks)
• netty-socketio-core/src/test/java/com/corundumstudio/socketio/integration/AbstractSocketIOIntegrationTest.java (2 hunks)
• netty-socketio-core/src/test/java/com/corundumstudio/socketio/integration/ClientDisconnectionTest.java (1 hunks)

🔇 Additional comments (2)

netty-socketio-core/src/main/java/com/corundumstudio/socketio/namespace/Namespace.java (2)

59-60: Logger wiring in Namespace looks good

SLF4J logger is correctly initialized for Namespace and ready for use; no behavioral impact beyond added observability.

Also applies to: 70-70

---

206-207: Helpful debug log on disconnect

The added debug log with session ID and namespace should significantly help when diagnosing disconnect-related issues, without changing behavior.

[Copilot]

Pull Request Overview

This PR addresses test stability issues and removes unnecessary testcontainers dependencies from the integration test suite.

Key Changes:

• Added a 3-second stabilization delay in ClientDisconnectionTest to ensure connection stability before disconnection
• Removed Redis/Redisson dependencies from AbstractSocketIOIntegrationTest, simplifying the test infrastructure
• Added defensive error handling with try-catch blocks in ClientHead for polling channel release and ping/timeout cancellation operations
• Enhanced logging in Namespace to track client disconnections

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.

File	Description
ClientDisconnectionTest.java	Adds 3-second sleep to ensure connection stability before testing disconnection
AbstractSocketIOIntegrationTest.java	Removes Redis container/Redisson client dependencies and implements infinite retry loop for server startup
Namespace.java	Adds debug logging for client disconnections with session ID and namespace information
ClientHead.java	Adds try-catch blocks around polling channel release and ping cancellation methods to prevent exceptions from propagating

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

netty-socketio-core/src/test/java/com/corundumstudio/socketio/integration/AbstractSocketIOIntegrationTest.java (1)

157-182: Bound the startup retry loop and preserve exception details

The while (!successful) startup loop in setUp() has no overall retry cap and swallows all exceptions:

• If findAvailablePort(), configureServer(), or server.start() repeatedly fail, tests can hang indefinitely.
• The catch block logs a generic message and discards the exception, making failures hard to debug.

Rework this to cap attempts (e.g., reuse MAX_PORT_RETRIES) and propagate the last failure after exhausting retries, while logging the exception:

-        boolean successful = false;
-        while (!successful) {
-            try {
-                // Find an available port for this test
-                serverPort = findAvailablePort();
-                serverConfig.setPort(serverPort);
-
-                // Allow subclasses to customize configuration
-                configureServer(serverConfig);
-
-                // Create and start server
-                server = new SocketIOServer(serverConfig);
-                server.start();
-
-                // Verify server started successfully
-                successful = true;
-            } catch (Exception e) {
-                log.warn("Port {} is not available, retrying...", serverPort);
-                // If server failed to start, try again with a different port
-                TimeUnit.SECONDS.sleep(1);
-            }
-        }
-
-        // Allow subclasses to do additional setup
-        additionalSetup();
+        boolean successful = false;
+        int attempts = 0;
+        Exception lastError = null;
+
+        while (!successful && attempts < MAX_PORT_RETRIES) {
+            attempts++;
+            try {
+                // Find an available port for this test
+                serverPort = findAvailablePort();
+                serverConfig.setPort(serverPort);
+
+                // Allow subclasses to customize configuration
+                configureServer(serverConfig);
+
+                // Create and start server
+                server = new SocketIOServer(serverConfig);
+                server.start();
+
+                // Verify server started successfully
+                successful = true;
+            } catch (Exception e) {
+                lastError = e;
+                log.warn(
+                    "Failed to start SocketIOServer on port {}, retrying... (attempt {}/{})",
+                    serverPort, attempts, MAX_PORT_RETRIES, e
+                );
+                // If server failed to start, try again with a different port
+                TimeUnit.SECONDS.sleep(1);
+            }
+        }
+
+        if (!successful) {
+            throw new RuntimeException(
+                "Failed to start SocketIOServer after " + attempts + " attempts",
+                lastError
+            );
+        }
+
+        // Allow subclasses to do additional setup
+        additionalSetup();

🧹 Nitpick comments (1)

netty-socketio-core/src/test/java/com/corundumstudio/socketio/integration/AbstractSocketIOIntegrationTest.java (1)

38-47: Update Javadoc to match current behavior (Redis/Testcontainers removed)

The class and setUp() Javadocs still describe “Automatic Redis container management” and initialization of Redis/Redisson components, but this base class no longer manages a Redis container or Redisson client.

To avoid confusion for future maintainers and test writers, consider updating these comments to reflect the current responsibilities (Socket.IO server setup with dynamic port allocation, no built‑in Redis/Testcontainers support).

Also applies to: 147-150

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fa895ab and 666b241.

📒 Files selected for processing (2)

• netty-socketio-core/src/main/java/com/corundumstudio/socketio/handler/ClientHead.java (2 hunks)
• netty-socketio-core/src/test/java/com/corundumstudio/socketio/integration/AbstractSocketIOIntegrationTest.java (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• netty-socketio-core/src/main/java/com/corundumstudio/socketio/handler/ClientHead.java

⏰ 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). (3)

• GitHub Check: build (17) / build
• GitHub Check: build (25) / build
• GitHub Check: build (21) / build

🔇 Additional comments (1)

netty-socketio-core/src/test/java/com/corundumstudio/socketio/integration/AbstractSocketIOIntegrationTest.java (1)

18-18: TimeUnit import is appropriate and necessary

The java.util.concurrent.TimeUnit import is required for the retry sleep call in setUp() and is correctly added.

[Copilot]

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

netty-socketio-core/src/test/java/com/corundumstudio/socketio/integration/RoomBroadcastTest.java (1)

76-78: Same fragile workaround as client1 handler.

This handler has the same issues as the client1 handler: no null checks, joining without delimiter, and potential false positives. Apply the same defensive improvements suggested for lines 70-75.

🧹 Nitpick comments (1)

netty-socketio-core/src/test/java/com/corundumstudio/socketio/integration/RoomBroadcastTest.java (1)

111-112: Weakened assertions reduce test rigor.

The change from assertEquals to substring contains checks means the test now passes if testData appears anywhere in the received data, even with extra content. This was necessary given the joining workaround, but it reduces the test's ability to catch bugs.

If the event handler is improved (as suggested in lines 70-75), consider strengthening these back to exact equality checks:

-        assertTrue(receivedData1.get().toString().contains(testData), "Client 1 should receive the correct data");
-        assertTrue(receivedData2.get().toString().contains(testData), "Client 2 should receive the correct data");
+        assertEquals(testData, receivedData1.get(), "Client 1 should receive the correct data");
+        assertEquals(testData, receivedData2.get(), "Client 2 should receive the correct data");

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 666b241 and ff3e940.

📒 Files selected for processing (1)

• netty-socketio-core/src/test/java/com/corundumstudio/socketio/integration/RoomBroadcastTest.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). (3)

• GitHub Check: build (21) / build
• GitHub Check: build (17) / build
• GitHub Check: build (25) / build

🔇 Additional comments (1)

netty-socketio-core/src/test/java/com/corundumstudio/socketio/integration/RoomBroadcastTest.java (1)

18-18: LGTM! Imports support the stream-based args handling.

The new imports are necessary for the updated event handler logic.

Also applies to: 23-23

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

netty-socketio-core/src/test/java/com/corundumstudio/socketio/integration/RoomBroadcastTest.java (1)

81-88: Same pattern as client1 handler.

This handler has the same implementation as client1's handler (lines 69-80), so the same feedback applies.

🧹 Nitpick comments (2)

netty-socketio-core/src/test/java/com/corundumstudio/socketio/integration/RoomBroadcastTest.java (2)

69-80: Good improvement from past review, but consider extracting data element.

The null check and comma delimiter address previous concerns. However, the current approach still joins all arguments, including the event name for polling transport, which makes the assertion less precise.

Since you're now using .contains() for validation (lines 121-122), the test is more flexible but also less strict—it would pass even if testData is only a substring of the actual received data.

For more robust handling, consider extracting only the data element(s) rather than joining all args:

         client1.on(testEvent, args -> {
             //there is an issue in socket.io-java-client with polling and websocket callback differences
             //for polling args contains event, for websocket does not
             //https://github.com/socketio/socket.io-client-java/issues/793
             //https://github.com/socketio4j/netty-socketio/issues/6
             if (args != null && args.length > 0) {
-                receivedData1.set(
-                        Arrays.stream(args).map(Object::toString)
-                                .collect(Collectors.joining(","))
-                );
+                // For polling, args might include event name as first element
+                // For websocket, args[0] is the data
+                // Take the last element as data to handle both cases
+                receivedData1.set(args[args.length - 1].toString());
             }
         });

This would allow you to restore the more precise equality assertion instead of .contains().

---

121-122: Assertions weakened by using substring match.

The change from direct equality to .contains() makes the test more flexible across transport modes, but it also weakens validation. The test would pass even if testData is only a substring of the actual received data, potentially missing bugs where extra data is appended or prepended.

If you adopt the data element extraction approach suggested earlier (using args[args.length - 1]), you could restore the more precise assertEquals() assertions:

-        assertTrue(receivedData1.get().toString().contains(testData), "Client 1 should receive the correct data");
-        assertTrue(receivedData2.get().toString().contains(testData), "Client 2 should receive the correct data");
+        assertEquals(testData, receivedData1.get().toString(), "Client 1 should receive the correct data");
+        assertEquals(testData, receivedData2.get().toString(), "Client 2 should receive the correct data");

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ff3e940 and c2ef57a.

📒 Files selected for processing (1)

• netty-socketio-core/src/test/java/com/corundumstudio/socketio/integration/RoomBroadcastTest.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). (5)

• GitHub Check: build (25) / build
• GitHub Check: build (17) / build
• GitHub Check: build (21) / build
• GitHub Check: Agent
• GitHub Check: CodeQL analysis (java)

🔇 Additional comments (1)

netty-socketio-core/src/test/java/com/corundumstudio/socketio/integration/RoomBroadcastTest.java (1)

18-18: LGTM: Necessary imports for stream-based transformation.

The Arrays and Collectors imports correctly support the new stream-based approach for handling event arguments.

Also applies to: 23-23

[Copilot]

Pull Request Overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

netty-socketio-core/src/test/java/com/corundumstudio/socketio/integration/AbstractSocketIOIntegrationTest.java:32

• These imports are no longer used after removing Redis/Redisson dependencies. They should be removed to keep the code clean.

import org.redisson.Redisson;
import org.redisson.api.RedissonClient;
import org.redisson.config.Config;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import com.corundumstudio.socketio.Configuration;
import com.corundumstudio.socketio.SocketIOServer;
import com.corundumstudio.socketio.store.CustomizedRedisContainer;
import com.corundumstudio.socketio.store.RedissonStoreFactory;

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.