Skip to content

Wait for conditions in tests instead of sleeping for a guess #48

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.

Sign up for GitHub

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

Merged
jochen-testingbot merged 1 commit into from
Sep 7, 2026
Merged

Wait for conditions in tests instead of sleeping for a guess #48

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 129 additions & 0 deletions src/test/java/com/testingbot/tunnel/Await.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
package com.testingbot.tunnel;

import java.io.IOException;
import java.net.InetAddress;
import java.net.InetSocketAddress;
import java.net.Socket;
import java.util.function.BooleanSupplier;
import java.util.function.Supplier;

/**
* Waits for a condition instead of sleeping for a guess.
*
* <p>Tests here used {@code Thread.sleep(500)} before asserting on something another thread had
* to produce -- a server finishing its bind, a log record being written, a statistic being
* updated. That is wrong in both directions at once. It is too long whenever the thing is
* already done, which it almost always is: {@code InsightServerTest} spent 5.5 of its 6.3
* seconds asleep. And it is too short whenever the machine is loaded, which is exactly when CI
* runs, so the failure arrives as an assertion about logging or statistics that says nothing
* about timing.
*
* <p>Polling fixes both: it returns as soon as the condition holds, and it can afford a timeout
* long enough to cover a slow runner because that cost is only paid on a genuine failure.
*
* <p>This does not suit every wait. Asserting that something did <em>not</em> happen has to give
* it time to happen first, and there is no condition to poll for -- see {@code ProxyLoopTest},
* which keeps its sleep and says why.
*/
public final class Await {

/** Long enough for a loaded CI runner; only ever paid in full when the test is failing. */
public static final long DEFAULT_TIMEOUT_MS = 10_000;

private static final long POLL_INTERVAL_MS = 20;

private Await() {
}

/**
* @param what described in the failure message, so a timeout says what never happened
* @param condition polled until true
* @throws AssertionError if it is still false when the timeout expires
*/
public static void until(String what, BooleanSupplier condition) {
until(what, DEFAULT_TIMEOUT_MS, condition);
}

public static void until(String what, long timeoutMs, BooleanSupplier condition) {
long deadline = System.currentTimeMillis() + timeoutMs;
AssertionError lastFailure = null;
while (System.currentTimeMillis() < deadline) {
try {
if (condition.getAsBoolean()) {
return;
}
lastFailure = null;
} catch (AssertionError notYet) {
// A condition expressed as an assertion is allowed to fail while we wait; only
// the last one matters, and only if we run out of time.
lastFailure = notYet;
}
sleep();
}
if (lastFailure != null) {
throw lastFailure;
}
throw new AssertionError("Timed out after " + timeoutMs + "ms waiting for: " + what);
}

/**
* As {@link #until}, for a condition that produces a value.
*
* @return the first non-null, non-empty value the supplier returns
*/
public static <T> T value(String what, Supplier<T> supplier) {
long deadline = System.currentTimeMillis() + DEFAULT_TIMEOUT_MS;
while (System.currentTimeMillis() < deadline) {
T candidate = supplier.get();
if (candidate != null
&& !(candidate instanceof CharSequence text && text.length() == 0)
&& !(candidate instanceof java.util.Collection<?> c && c.isEmpty())) {
return candidate;
}
sleep();
}
throw new AssertionError(
"Timed out after " + DEFAULT_TIMEOUT_MS + "ms waiting for: " + what);
}

/**
* Polls for up to {@code timeoutMs} and reports whether the condition held, without failing.
*
* <p>For a helper shared by tests that expect something and tests that expect nothing: the
* first kind returns as soon as it appears, the second pays the full wait it needs, and
* neither has to know which it is.
*
* @return true if the condition became true within the timeout
*/
public static boolean atMost(long timeoutMs, BooleanSupplier condition) {
long deadline = System.currentTimeMillis() + timeoutMs;
while (System.currentTimeMillis() < deadline) {
if (condition.getAsBoolean()) {
return true;
}
sleep();
}
return condition.getAsBoolean();
}

/** Waits until something accepts connections on {@code port}. */
public static void serverOn(int port) {
until("a server listening on port " + port, () -> {
try (Socket probe = new Socket()) {
probe.connect(new InetSocketAddress(InetAddress.getLoopbackAddress(), port), 200);
return true;
} catch (IOException notYet) {
return false;
}
});
}

private static void sleep() {
try {
Thread.sleep(POLL_INTERVAL_MS);
} catch (InterruptedException interrupted) {
Thread.currentThread().interrupt();
throw new AssertionError("Interrupted while waiting", interrupted);
}
}
}
34 changes: 15 additions & 19 deletions src/test/java/com/testingbot/tunnel/HealthEndpointsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ void setUp() throws Exception {
app.setClientSecret("test_secret");
app.setMetricsPort(metricsPort);
insightServer = new InsightServer(app);
waitForPort(metricsPort);
Await.serverOn(metricsPort);
}

@AfterEach
Expand All @@ -56,16 +56,6 @@ void tearDown() {
}
}

private static void waitForPort(int port) throws Exception {
for (int i = 0; i < 100; i++) {
try (java.net.Socket s = new java.net.Socket("127.0.0.1", port)) {
return;
} catch (IOException retry) {
Thread.sleep(50);
}
}
throw new IllegalStateException("Insight server did not start on port " + port);
}

private static int status(int port, String path) throws Exception {
try (CloseableHttpClient client = HttpClients.createDefault()) {
Expand Down Expand Up @@ -124,14 +114,20 @@ void healthEndpoints_areReachableWithoutMetricsAuth() throws Exception {
app.setClientSecret("test_secret");
app.setMetricsPort(port);
app.setMetricsAuth("user:password");
new InsightServer(app);
waitForPort(port);

// Probes cannot easily carry credentials, so these must not be behind auth...
assertThat(status(port, "/healthz")).isEqualTo(200);
assertThat(status(port, "/readyz")).isEqualTo(503);
// ...but /metrics still is.
assertThat(status(port, "/metrics")).isEqualTo(401);
// A second server, so it needs stopping too: tearDown only knows about the one from
// setUp. Left running, it held this port for the rest of the JVM.
InsightServer authed = new InsightServer(app);
try {
Await.serverOn(port);

// Probes cannot easily carry credentials, so these must not be behind auth...
assertThat(status(port, "/healthz")).isEqualTo(200);
assertThat(status(port, "/readyz")).isEqualTo(503);
// ...but /metrics still is.
assertThat(status(port, "/metrics")).isEqualTo(401);
} finally {
authed.stop();
}
}

@Test
Expand Down
61 changes: 37 additions & 24 deletions src/test/java/com/testingbot/tunnel/InsightServerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,19 @@ void setUp() throws Exception {
@AfterEach
void tearDown() throws Exception {
resetStatistics();
// Every test here leaked a Jetty server and a bound port. That leak is why the tests
// below each needed a *different* hardcoded port: the previous one was still held.
if (insightServer != null) {
insightServer.stop();
insightServer = null;
}
}

@Test
void constructor_shouldStartServer() throws Exception {
// Given: App with metrics port configured
app.setMetricsPort(8999);
int port = freePort();
app.setMetricsPort(port);

// When: Creating InsightServer
insightServer = new InsightServer(app);
Expand All @@ -64,19 +71,20 @@ void constructor_shouldStartServer() throws Exception {
assertThat(insightServer).isNotNull();

// Give server time to start
Thread.sleep(500);
Await.serverOn(port);
}

@Test
void metricsEndpoint_shouldReturnJson() throws Exception {
// Given: Running InsightServer
app.setMetricsPort(8998);
int port = freePort();
app.setMetricsPort(port);
insightServer = new InsightServer(app);
Thread.sleep(500); // Wait for server to start
Await.serverOn(port);

// When: Making request to metrics endpoint
try (CloseableHttpClient client = HttpClients.createDefault()) {
HttpGet request = new HttpGet("http://localhost:8998/");
HttpGet request = new HttpGet("http://localhost:" + port + "/");
client.execute(request, response -> {
// Then: Should return 200 OK
assertThat(response.getCode()).isEqualTo(200);
Expand All @@ -101,13 +109,14 @@ void metricsEndpoint_shouldReturnJson() throws Exception {
@Test
void metricsEndpoint_shouldReturnCorrectVersion() throws Exception {
// Given: Running InsightServer
app.setMetricsPort(8997);
int port = freePort();
app.setMetricsPort(port);
insightServer = new InsightServer(app);
Thread.sleep(500);
Await.serverOn(port);

// When: Getting metrics
try (CloseableHttpClient client = HttpClients.createDefault()) {
HttpGet request = new HttpGet("http://localhost:8997/");
HttpGet request = new HttpGet("http://localhost:" + port + "/");
client.execute(request, response -> {
String body = EntityUtils.toString(response.getEntity());
JsonNode json = objectMapper.readTree(body);
Expand All @@ -125,13 +134,14 @@ void metricsEndpoint_shouldReturnUptime() throws Exception {
long startTime = System.currentTimeMillis() - 5000; // 5 seconds ago
Statistics.setStartTime(startTime);

app.setMetricsPort(8996);
int port = freePort();
app.setMetricsPort(port);
insightServer = new InsightServer(app);
Thread.sleep(500);
Await.serverOn(port);

// When: Getting metrics
try (CloseableHttpClient client = HttpClients.createDefault()) {
HttpGet request = new HttpGet("http://localhost:8996/");
HttpGet request = new HttpGet("http://localhost:" + port + "/");
client.execute(request, response -> {
String body = EntityUtils.toString(response.getEntity());
JsonNode json = objectMapper.readTree(body);
Expand All @@ -152,13 +162,14 @@ void metricsEndpoint_shouldReturnNumberOfRequests() throws Exception {
Statistics.addRequest();
Statistics.addRequest();

app.setMetricsPort(8995);
int port = freePort();
app.setMetricsPort(port);
insightServer = new InsightServer(app);
Thread.sleep(500);
Await.serverOn(port);

// When: Getting metrics
try (CloseableHttpClient client = HttpClients.createDefault()) {
HttpGet request = new HttpGet("http://localhost:8995/");
HttpGet request = new HttpGet("http://localhost:" + port + "/");
client.execute(request, response -> {
String body = EntityUtils.toString(response.getEntity());
JsonNode json = objectMapper.readTree(body);
Expand All @@ -176,13 +187,14 @@ void metricsEndpoint_shouldReturnBytesTransferred() throws Exception {
resetStatistics();
Statistics.addBytesTransferred(2048);

app.setMetricsPort(8994);
int port = freePort();
app.setMetricsPort(port);
insightServer = new InsightServer(app);
Thread.sleep(500);
Await.serverOn(port);

// When: Getting metrics
try (CloseableHttpClient client = HttpClients.createDefault()) {
HttpGet request = new HttpGet("http://localhost:8994/");
HttpGet request = new HttpGet("http://localhost:" + port + "/");
client.execute(request, response -> {
String body = EntityUtils.toString(response.getEntity());
JsonNode json = objectMapper.readTree(body);
Expand All @@ -197,14 +209,15 @@ void metricsEndpoint_shouldReturnBytesTransferred() throws Exception {
@Test
void metricsEndpoint_shouldHandleMultipleRequests() throws Exception {
// Given: Running InsightServer
app.setMetricsPort(8993);
int port = freePort();
app.setMetricsPort(port);
insightServer = new InsightServer(app);
Thread.sleep(500);
Await.serverOn(port);

// When: Making multiple requests
try (CloseableHttpClient client = HttpClients.createDefault()) {
for (int i = 0; i < 5; i++) {
HttpGet request = new HttpGet("http://localhost:8993/");
HttpGet request = new HttpGet("http://localhost:" + port + "/");
client.execute(request, response -> {
// Then: Each request should succeed
assertThat(response.getCode()).isEqualTo(200);
Expand All @@ -230,7 +243,7 @@ void prometheusEndpoint_shouldReturnExpositionFormat() throws Exception {
int port = freePort();
app.setMetricsPort(port);
insightServer = new InsightServer(app);
Thread.sleep(500);
Await.serverOn(port);

try (CloseableHttpClient client = HttpClients.createDefault()) {
HttpGet request = new HttpGet("http://localhost:" + port + "/metrics");
Expand All @@ -254,7 +267,7 @@ void prometheusEndpoint_withAuth_shouldRejectAnonymous() throws Exception {
app.setMetricsPort(port);
app.setMetricsAuth("user:secret");
insightServer = new InsightServer(app);
Thread.sleep(500);
Await.serverOn(port);

try (CloseableHttpClient client = HttpClients.createDefault()) {
HttpGet request = new HttpGet("http://localhost:" + port + "/metrics");
Expand All @@ -273,7 +286,7 @@ void prometheusEndpoint_withAuth_shouldAcceptCorrectCredentials() throws Excepti
app.setMetricsPort(port);
app.setMetricsAuth("user:secret");
insightServer = new InsightServer(app);
Thread.sleep(500);
Await.serverOn(port);

String credentials = java.util.Base64.getEncoder()
.encodeToString("user:secret".getBytes(java.nio.charset.StandardCharsets.UTF_8));
Expand All @@ -296,7 +309,7 @@ void prometheusEndpoint_withAuth_shouldStillServeJsonStatusUnprotected() throws
app.setMetricsPort(port);
app.setMetricsAuth("user:secret");
insightServer = new InsightServer(app);
Thread.sleep(500);
Await.serverOn(port);

try (CloseableHttpClient client = HttpClients.createDefault()) {
HttpGet request = new HttpGet("http://localhost:" + port + "/");
Expand Down
8 changes: 6 additions & 2 deletions src/test/java/com/testingbot/tunnel/integration/ConnectLoggingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,8 @@ void aConnectProducesNoPerRequestLineWhenLoggingIsOff() throws Exception {
} catch (IOException expected) {
// the tunnel cannot be established; only the logging matters here
}
// A deliberate wait, not a guess at readiness: this asserts a line never appears, so
// there is nothing to poll for and the only way to be wrong is to look too early.
Thread.sleep(300);

assertThat(captured.messages())
Expand Down Expand Up @@ -176,7 +178,8 @@ void aConnectIsLoggedAtUrlLevel() throws Exception {
} catch (IOException expected) {
// the dial cannot succeed; the line is written on the way in regardless
}
Thread.sleep(300);
com.testingbot.tunnel.Await.until("a CONNECT line from the proxy logger",
() -> urlCaptured.messages().stream().anyMatch(m -> m.contains("CONNECT")));

assertThat(urlCaptured.messages())
.as("--log-http url must log the CONNECT")
Expand Down Expand Up @@ -221,7 +224,8 @@ void debugDumpsTheConnectHeaders() throws Exception {
} catch (IOException expected) {
// the destination does not resolve; only the logging matters here
}
Thread.sleep(300);
com.testingbot.tunnel.Await.until("the CONNECT headers to be logged",
() -> debugCaptured.messages().stream().anyMatch(m -> m.contains("X-Marker")));

String log = String.join("\n", debugCaptured.messages());
assertThat(log).contains("X-Marker: seen");
Expand Down
5 changes: 4 additions & 1 deletion src/test/java/com/testingbot/tunnel/integration/ForwarderLoggingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,10 @@ private List<String> relayOneRequest(String logHttp) throws Exception {
} catch (IOException expected) {
// only the logging matters here
}
Thread.sleep(300);
// Shared by the tests that expect a line and the ones that expect none, so it cannot
// simply poll: it returns as soon as something is logged, and otherwise waits the full
// window before reporting nothing, which is what the absence assertions need.
com.testingbot.tunnel.Await.atMost(300, () -> !captured.messages().isEmpty());
return captured.messages();
}

Expand Down
Loading
Loading