Skip to content

Commit aa3bd2a

Browse files
msrathore-dbclaude
andauthored
Fix flaky ClientConfiguratorTest.testFindAvailablePort test (databricks#1058)
## Description Fixed flaky test `ClientConfiguratorTest.testFindAvailablePort` that was failing intermittently in CI when sequential ports were unavailable. The test incorrectly assumed `availablePort + 1` would always be available, causing it to fail when other processes occupied sequential ports in busy CI environments (expected port 46512 but received 46513). **Changes made:** - Updated test assertions to validate incremental search behavior rather than expecting specific port values - Fixed incorrect `setReuseAddress()` usage - was called after binding, now properly called before binding - Applied consistent `ServerSocket` creation pattern across all test cases in the file This eliminates the race condition that caused intermittent test failures. ## Testing - Verified no compilation errors using IDE diagnostics - Test now validates behavior (incremental port search works) rather than assuming specific port availability - Changes follow Java best practices for socket option configuration ## Additional Notes to the Reviewer The key insight is that the test was testing **implementation details** (exact port numbers) rather than **behavior** (finds next available port). The fix makes it robust to different CI/local environments. Also fixed a subtle bug where `ServerSocket.setReuseAddress(true)` was being called after the socket was already bound (via the constructor), making it ineffective. Now properly set before binding. NO_CHANGELOG=true --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 84a28c9 commit aa3bd2a

File tree

2 files changed

+25
-9
lines changed

2 files changed

+25
-9
lines changed

src/main/java/com/databricks/jdbc/dbclient/impl/common/ClientConfigurator.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import com.databricks.sdk.core.oauth.TokenCache;
2424
import com.databricks.sdk.core.utils.Cloud;
2525
import java.io.IOException;
26+
import java.net.InetSocketAddress;
2627
import java.net.ServerSocket;
2728
import java.nio.file.Path;
2829
import java.nio.file.Paths;
@@ -282,8 +283,9 @@ public int findAvailablePort(List<Integer> initialPorts) {
282283
* @return true if the port is available, false otherwise
283284
*/
284285
boolean isPortAvailable(int port) {
285-
try (ServerSocket serverSocket = new ServerSocket(port)) {
286+
try (ServerSocket serverSocket = new ServerSocket()) {
286287
serverSocket.setReuseAddress(true);
288+
serverSocket.bind(new InetSocketAddress(port));
287289
return true;
288290
} catch (IOException e) {
289291
return false;

src/test/java/com/databricks/jdbc/dbclient/impl/common/ClientConfiguratorTest.java

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import com.databricks.sdk.core.oauth.ExternalBrowserCredentialsProvider;
2626
import com.databricks.sdk.core.utils.Cloud;
2727
import java.io.IOException;
28+
import java.net.InetSocketAddress;
2829
import java.net.ServerSocket;
2930
import java.time.Duration;
3031
import java.util.Collections;
@@ -360,19 +361,29 @@ void testFindAvailablePort() throws Exception {
360361

361362
// Test with multiple ports, first unavailable
362363
int secondAvailablePort = findFreePort();
363-
try (ServerSocket serverSocket = new ServerSocket(availablePort)) {
364+
try (ServerSocket serverSocket = new ServerSocket()) {
364365
serverSocket.setReuseAddress(true);
366+
serverSocket.bind(new InetSocketAddress(availablePort));
365367
ports = List.of(availablePort, secondAvailablePort);
366368
result = configurator.findAvailablePort(ports);
367369
assertEquals(secondAvailablePort, result);
368370
}
369371

370-
// Test incremental search - first port unavailable, second available
371-
try (ServerSocket serverSocket = new ServerSocket(availablePort)) {
372-
serverSocket.setReuseAddress(true);
372+
// Test incremental search - first port unavailable, finds next available in sequence
373+
try (ServerSocket serverSocket2 = new ServerSocket()) {
374+
serverSocket2.setReuseAddress(true);
375+
serverSocket2.bind(new InetSocketAddress(availablePort));
373376
ports = List.of(availablePort);
374377
result = configurator.findAvailablePort(ports);
375-
assertEquals(availablePort + 1, result);
378+
assertTrue(
379+
result > availablePort,
380+
String.format("Expected port > %d, but got %d", availablePort, result));
381+
// 3. The returned port should actually be available
382+
try (ServerSocket testSocket = new ServerSocket()) {
383+
testSocket.setReuseAddress(true);
384+
testSocket.bind(new InetSocketAddress(result));
385+
assertNotNull(testSocket, "Returned port should be available for binding");
386+
}
376387
}
377388
}
378389

@@ -394,10 +405,12 @@ void testFindAvailablePortThrowsExceptionWhenNoPortsAvailable() throws Exception
394405
}
395406

396407
// Occupy the ports to make them unavailable
397-
try (ServerSocket socket1 = new ServerSocket(port1);
398-
ServerSocket socket2 = new ServerSocket(port2)) {
408+
try (ServerSocket socket1 = new ServerSocket();
409+
ServerSocket socket2 = new ServerSocket()) {
399410
socket1.setReuseAddress(true);
411+
socket1.bind(new InetSocketAddress(port1));
400412
socket2.setReuseAddress(true);
413+
socket2.bind(new InetSocketAddress(port2));
401414

402415
// First test with multiple specified ports
403416
List<Integer> unavailablePorts = List.of(port1, port2);
@@ -425,8 +438,9 @@ protected boolean isPortAvailable(int port) {
425438

426439
/** Utility method to find a free port */
427440
private int findFreePort() {
428-
try (ServerSocket socket = new ServerSocket(0)) {
441+
try (ServerSocket socket = new ServerSocket()) {
429442
socket.setReuseAddress(true);
443+
socket.bind(new InetSocketAddress(0));
430444
return socket.getLocalPort();
431445
} catch (IOException e) {
432446
throw new RuntimeException("Failed to find free port", e);

0 commit comments

Comments
 (0)