refactor(test): migrate remaining wait loops to Awaitility in GAX tests#13484
refactor(test): migrate remaining wait loops to Awaitility in GAX tests#13484blakeli0 wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request replaces arbitrary Thread.sleep() calls with Awaitility.await() across several test files to improve test reliability and execution speed. The review feedback highlights opportunities to clean up unused local variables and redundant assertions left over from the migration. It also recommends using Awaitility's built-in .during() modifier instead of a manual AtomicInteger counter, and addresses a potential double-counting bug in ScheduledRetryingExecutorTest where the same failed attempt future could be processed multiple times during polling.
| final long DELAY_TIME = 30L; | ||
| int actualRemaining = 0; | ||
| for (int retry = 0; retry < 3; retry++) { | ||
| System.gc(); | ||
| System.runFinalization(); | ||
| actualRemaining = BatcherReference.cleanQueue(); | ||
| if (actualRemaining == 0) { | ||
| break; | ||
| } | ||
| Thread.sleep(DELAY_TIME * (1L << retry)); | ||
| } | ||
| await() | ||
| .atMost(Duration.ofSeconds(5)) | ||
| .until( | ||
| () -> { | ||
| System.gc(); | ||
| System.runFinalization(); | ||
| return BatcherReference.cleanQueue() == 0; | ||
| }); | ||
| assertThat(actualRemaining).isAtMost(0); |
There was a problem hiding this comment.
The local variables DELAY_TIME and actualRemaining are no longer used after migrating to Awaitility, and the assertion assertThat(actualRemaining).isAtMost(0) is redundant because await() already guarantees that cleanQueue() == 0 succeeded. We can clean this up by removing these unused variables and the redundant assertion.
await()
.atMost(Duration.ofSeconds(5))
.until(
() -> {
System.gc();
System.runFinalization();
return BatcherReference.cleanQueue() == 0;
});| final long DELAY_TIME = 30L; | ||
| int actualRemaining = 0; | ||
| for (int retry = 0; retry < 3; retry++) { | ||
| System.gc(); | ||
| System.runFinalization(); | ||
| actualRemaining = BatcherReference.cleanQueue(); | ||
| if (actualRemaining == 0) { | ||
| break; | ||
| } | ||
| Thread.sleep(DELAY_TIME * (1L << retry)); | ||
| } | ||
| await() | ||
| .atMost(Duration.ofSeconds(5)) | ||
| .until( | ||
| () -> { | ||
| System.gc(); | ||
| System.runFinalization(); | ||
| return BatcherReference.cleanQueue() == 0; | ||
| }); | ||
| assertThat(actualRemaining).isAtMost(0); |
There was a problem hiding this comment.
The local variables DELAY_TIME and actualRemaining are no longer used after migrating to Awaitility, and the assertion assertThat(actualRemaining).isAtMost(0) is redundant because await() already guarantees that cleanQueue() == 0 succeeded. We can clean this up by removing these unused variables and the redundant assertion.
await()
.atMost(Duration.ofSeconds(5))
.until(
() -> {
System.gc();
System.runFinalization();
return BatcherReference.cleanQueue() == 0;
});| final AtomicInteger runs = new AtomicInteger(0); | ||
| await() | ||
| .pollInterval(Duration.ofMillis(10)) | ||
| .atMost(Duration.ofSeconds(2)) | ||
| .until( | ||
| () -> { | ||
| System.gc(); | ||
| System.runFinalization(); | ||
| BatcherReference.cleanQueue(); | ||
| return runs.incrementAndGet() >= 20; | ||
| }); |
There was a problem hiding this comment.
Using a dummy AtomicInteger counter to simulate a sleep loop inside Awaitility is an anti-pattern. Instead, we can use Awaitility's built-in .during() modifier to elegantly assert that the log records remain empty for a specified duration while repeatedly running GC.
await()
.pollInterval(Duration.ofMillis(10))
.during(Duration.ofMillis(200))
.atMost(Duration.ofSeconds(2))
.until(
() -> {
System.gc();
System.runFinalization();
BatcherReference.cleanQueue();
synchronized (records) {
return records.isEmpty();
}
});| final int[] failedAttempts = {0}; | ||
| await() | ||
| .pollInterval(Duration.ofMillis(2)) | ||
| .atMost(Duration.ofSeconds(5)) | ||
| .until( | ||
| () -> { | ||
| ApiFuture<String> attemptResult = future.peekAttemptResult(); | ||
| if (attemptResult != null) { | ||
| assertTrue(attemptResult.isDone()); | ||
| assertFalse(attemptResult.isCancelled()); | ||
| try { | ||
| attemptResult.get(); | ||
| } catch (ExecutionException e) { | ||
| if (e.getCause() instanceof CustomException) { | ||
| failedAttempts[0]++; | ||
| } | ||
| } | ||
| } | ||
| return future.isDone(); | ||
| }); |
There was a problem hiding this comment.
Since peekAttemptResult() returns the current attempt's future, and Awaitility polls the condition repeatedly, the same failed attempt future might be peeked and processed multiple times. This leads to double-counting of failedAttempts. We can avoid this by keeping track of the last seen attempt future and only processing it when a new attempt is returned.
final ApiFuture<String>[] lastSeenAttempt = new ApiFuture[1];
final int[] failedAttempts = {0};
await()
.pollInterval(Duration.ofMillis(2))
.atMost(Duration.ofSeconds(5))
.until(
() -> {
ApiFuture<String> attemptResult = future.peekAttemptResult();
if (attemptResult != null && attemptResult != lastSeenAttempt[0]) {
lastSeenAttempt[0] = attemptResult;
assertTrue(attemptResult.isDone());
assertFalse(attemptResult.isCancelled());
try {
attemptResult.get();
} catch (ExecutionException e) {
if (e.getCause() instanceof CustomException) {
failedAttempts[0]++;
}
}
}
return future.isDone();
});… failed attempts in ScheduledRetryingExecutorTest
…erence in GAX tests
… maximum in BatcherImplTest
This PR migrates all remaining wait loops and polling blocks in GAX tests to use Awaitility. This is the second part of b/505479343, following the initial introduction of Awaitility in #13458.
Specifically, this migrates: