From 31354b4ced5124ac7a99f010effc7be98c0189a5 Mon Sep 17 00:00:00 2001 From: Lawrence Qiu Date: Tue, 23 Jun 2026 16:42:27 +0000 Subject: [PATCH 1/3] fix(gax): use fake clock in ScheduledRetryingExecutorTest for robust and fast execution Introduced FakeApiClock and RecordingScheduler to ScheduledRetryingExecutorTest to run most tests (including the flaky testSuccessWithFailuresGetAttempt) instantly without real-time delays. Isolated tests that rely on asynchronous polling or cancellation to use local real executors and clocks, with increased timeout (10s) to avoid flakiness on slow VMs. Made RecordingScheduler stubbings lenient to avoid UnnecessaryStubbingException in tests that do not query all recorded metrics. TAG=agy CONV=71af906a-b7dc-4dda-a2fa-ebe24b5d8b91 BUG=13535 --- .../api/gax/core/RecordingScheduler.java | 11 +- .../ScheduledRetryingExecutorTest.java | 179 ++++++++++-------- 2 files changed, 112 insertions(+), 78 deletions(-) diff --git a/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/core/RecordingScheduler.java b/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/core/RecordingScheduler.java index d495cf0d63d8..ce802eedabb0 100644 --- a/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/core/RecordingScheduler.java +++ b/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/core/RecordingScheduler.java @@ -60,7 +60,8 @@ public static RecordingScheduler create(final FakeApiClock clock) { // mock class methods: // ScheduledFuture schedule(Runnable command, long delay, TimeUnit unit); - when(mock.schedule(any(Runnable.class), anyLong(), any(TimeUnit.class))) + Mockito.lenient() + .when(mock.schedule(any(Runnable.class), anyLong(), any(TimeUnit.class))) .then( new Answer>() { @Override @@ -78,7 +79,8 @@ public ScheduledFuture answer(InvocationOnMock invocation) throws Throwable { }); // List shutdownNow() - when(mock.shutdownNow()) + Mockito.lenient() + .when(mock.shutdownNow()) .then( new Answer>() { @Override @@ -88,10 +90,11 @@ public List answer(InvocationOnMock invocation) throws Throwable { }); // List getSleepDurations() - when(mock.getSleepDurations()).thenReturn(sleepDurations); + Mockito.lenient().when(mock.getSleepDurations()).thenReturn(sleepDurations); // int getIterationsCount() - when(mock.getIterationsCount()) + Mockito.lenient() + .when(mock.getIterationsCount()) .then( new Answer() { @Override diff --git a/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java b/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java index 89b807bd5610..ce25f0b7f927 100644 --- a/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java +++ b/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java @@ -38,8 +38,11 @@ import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertTrue; +import com.google.api.core.ApiClock; import com.google.api.core.ApiFuture; import com.google.api.core.NanoClock; +import com.google.api.gax.core.FakeApiClock; +import com.google.api.gax.core.RecordingScheduler; import com.google.api.gax.retrying.FailingCallable.CustomException; import com.google.api.gax.rpc.testing.FakeCallContext; import java.time.Duration; @@ -51,6 +54,7 @@ import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.Mockito; @@ -58,6 +62,13 @@ class ScheduledRetryingExecutorTest extends AbstractRetryingExecutorTest { // Number of test runs, essential for multithreaded tests. private static final int EXECUTIONS_COUNT = 5; + private FakeApiClock fakeClock; + + @BeforeEach + void setUp() { + fakeClock = new FakeApiClock(0L); + scheduledExecutorService = RecordingScheduler.create(fakeClock); + } @Override protected RetryingExecutorWithContext getExecutor(RetryAlgorithm retryAlgorithm) { @@ -67,9 +78,17 @@ protected RetryingExecutorWithContext getExecutor(RetryAlgorithm @Override protected RetryAlgorithm getAlgorithm( RetrySettings retrySettings, int apocalypseCountDown, RuntimeException apocalypseException) { + return getAlgorithm(retrySettings, apocalypseCountDown, apocalypseException, fakeClock); + } + + protected RetryAlgorithm getAlgorithm( + RetrySettings retrySettings, + int apocalypseCountDown, + RuntimeException apocalypseException, + ApiClock clock) { return new RetryAlgorithm<>( new TestResultRetryAlgorithm(apocalypseCountDown, apocalypseException), - new ExponentialRetryAlgorithm(retrySettings, NanoClock.getDefaultClock())); + new ExponentialRetryAlgorithm(retrySettings, clock)); } private RetryingExecutorWithContext getRetryingExecutor( @@ -81,54 +100,60 @@ private RetryingExecutorWithContext getRetryingExecutor( void testSuccessWithFailuresPeekAttempt() throws Exception { RetrySettings retrySettings = FAST_RETRY_SETTINGS.toBuilder() - .setTotalTimeoutDuration(java.time.Duration.ofMillis(1000L)) + .setTotalTimeoutDuration(java.time.Duration.ofMillis(10000L)) .setMaxAttempts(100) .build(); - for (int executionsCount = 0; executionsCount < EXECUTIONS_COUNT; executionsCount++) { - - FailingCallable callable = new FailingCallable(15, "request", "SUCCESS", tracer); - - RetryingExecutorWithContext executor = - getRetryingExecutor(getAlgorithm(retrySettings, 0, null), scheduledExecutorService); - RetryingFuture future = - executor.createFuture( - callable, - FakeCallContext.createDefault().withTracer(tracer).withRetrySettings(retrySettings)); - callable.setExternalFuture(future); - - assertNull(future.peekAttemptResult()); - assertSame(future.peekAttemptResult(), future.peekAttemptResult()); - assertFalse(future.getAttemptResult().isDone()); - assertFalse(future.getAttemptResult().isCancelled()); - - future.setAttemptFuture(executor.submit(future)); - - final AtomicInteger failedAttempts = new AtomicInteger(0); - final AtomicReference> lastSeenAttempt = new AtomicReference<>(); - await() - .pollInterval(Duration.ofMillis(2)) - .atMost(Duration.ofSeconds(5)) - .until( - () -> { - ApiFuture attemptResult = future.peekAttemptResult(); - if (attemptResult != null && attemptResult != lastSeenAttempt.get()) { - lastSeenAttempt.set(attemptResult); - assertTrue(attemptResult.isDone()); - assertFalse(attemptResult.isCancelled()); - try { - attemptResult.get(); - } catch (ExecutionException e) { - if (e.getCause() instanceof CustomException) { - failedAttempts.incrementAndGet(); + ScheduledExecutorService localExecutor = Executors.newSingleThreadScheduledExecutor(); + try { + for (int executionsCount = 0; executionsCount < EXECUTIONS_COUNT; executionsCount++) { + + FailingCallable callable = new FailingCallable(15, "request", "SUCCESS", tracer); + + RetryingExecutorWithContext executor = + getRetryingExecutor( + getAlgorithm(retrySettings, 0, null, NanoClock.getDefaultClock()), localExecutor); + RetryingFuture future = + executor.createFuture( + callable, + FakeCallContext.createDefault().withTracer(tracer).withRetrySettings(retrySettings)); + callable.setExternalFuture(future); + + assertNull(future.peekAttemptResult()); + assertSame(future.peekAttemptResult(), future.peekAttemptResult()); + assertFalse(future.getAttemptResult().isDone()); + assertFalse(future.getAttemptResult().isCancelled()); + + future.setAttemptFuture(executor.submit(future)); + + final AtomicInteger failedAttempts = new AtomicInteger(0); + final AtomicReference> lastSeenAttempt = new AtomicReference<>(); + await() + .pollInterval(Duration.ofMillis(2)) + .atMost(Duration.ofSeconds(5)) + .until( + () -> { + ApiFuture attemptResult = future.peekAttemptResult(); + if (attemptResult != null && attemptResult != lastSeenAttempt.get()) { + lastSeenAttempt.set(attemptResult); + assertTrue(attemptResult.isDone()); + assertFalse(attemptResult.isCancelled()); + try { + attemptResult.get(); + } catch (ExecutionException e) { + if (e.getCause() instanceof CustomException) { + failedAttempts.incrementAndGet(); + } } } - } - return future.isDone(); - }); + return future.isDone(); + }); - assertFutureSuccess(future); - assertEquals(15, future.getAttemptSettings().getAttemptCount()); - assertTrue(failedAttempts.get() > 0); + assertFutureSuccess(future); + assertEquals(15, future.getAttemptSettings().getAttemptCount()); + assertTrue(failedAttempts.get() > 0); + } + } finally { + localExecutor.shutdownNow(); } } @@ -259,36 +284,42 @@ void testCancelOuterFutureAfterStart() throws Exception { // tiny RRD value is small, but not impossible. .setJittered(false) .build(); - for (int executionsCount = 0; executionsCount < EXECUTIONS_COUNT; executionsCount++) { - FailingCallable callable = new FailingCallable(4, "request", "SUCCESS", tracer); - RetryingExecutorWithContext executor = - getRetryingExecutor(getAlgorithm(retrySettings, 0, null), scheduledExecutorService); - RetryingFuture future = - executor.createFuture( - callable, - FakeCallContext.createDefault().withTracer(tracer).withRetrySettings(retrySettings)); - callable.setExternalFuture(future); - future.setAttemptFuture(executor.submit(future)); - - await() - .atMost(Duration.ofSeconds(5)) - .until( - () -> - future.getAttemptSettings() != null - && future.getAttemptSettings().getAttemptCount() > 0); - - boolean res = future.cancel(false); - assertTrue(res); - assertFutureCancel(future); - - // Verify that the cancelled future is traced. Every attempt increases the number - // of cancellation attempts from the tracer. - Mockito.verify(tracer, Mockito.times(executionsCount + 1)).attemptCancelled(); - - // Assert that future has at least been attempted once - // i.e. The future from executor.submit() has been run by the ScheduledExecutor - assertTrue(future.getAttemptSettings().getAttemptCount() > 0); - assertTrue(future.getAttemptSettings().getAttemptCount() < 4); + ScheduledExecutorService localExecutor = Executors.newSingleThreadScheduledExecutor(); + try { + for (int executionsCount = 0; executionsCount < EXECUTIONS_COUNT; executionsCount++) { + FailingCallable callable = new FailingCallable(4, "request", "SUCCESS", tracer); + RetryingExecutorWithContext executor = + getRetryingExecutor( + getAlgorithm(retrySettings, 0, null, NanoClock.getDefaultClock()), localExecutor); + RetryingFuture future = + executor.createFuture( + callable, + FakeCallContext.createDefault().withTracer(tracer).withRetrySettings(retrySettings)); + callable.setExternalFuture(future); + future.setAttemptFuture(executor.submit(future)); + + await() + .atMost(Duration.ofSeconds(5)) + .until( + () -> + future.getAttemptSettings() != null + && future.getAttemptSettings().getAttemptCount() > 0); + + boolean res = future.cancel(false); + assertTrue(res); + assertFutureCancel(future); + + // Verify that the cancelled future is traced. Every attempt increases the number + // of cancellation attempts from the tracer. + Mockito.verify(tracer, Mockito.times(executionsCount + 1)).attemptCancelled(); + + // Assert that future has at least been attempted once + // i.e. The future from executor.submit() has been run by the ScheduledExecutor + assertTrue(future.getAttemptSettings().getAttemptCount() > 0); + assertTrue(future.getAttemptSettings().getAttemptCount() < 4); + } + } finally { + localExecutor.shutdownNow(); } } From 25e05b6fc1b1df9ea2a51783721767e77c797675 Mon Sep 17 00:00:00 2001 From: Lawrence Qiu Date: Wed, 24 Jun 2026 18:07:38 +0000 Subject: [PATCH 2/3] test(gax): isolate local executors inside loops in ScheduledRetryingExecutorTest Addressed PR comments by moving the creation and shutdown of local executors inside the test loops for testSuccessWithFailuresPeekAttempt and testCancelOuterFutureAfterStart to ensure complete isolation between iterations and avoid potential flakiness. TAG=agy CONV=71af906a-b7dc-4dda-a2fa-ebe24b5d8b91 BUG=13535 --- .../ScheduledRetryingExecutorTest.java | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java b/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java index ce25f0b7f927..411dc0e6c076 100644 --- a/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java +++ b/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java @@ -103,10 +103,9 @@ void testSuccessWithFailuresPeekAttempt() throws Exception { .setTotalTimeoutDuration(java.time.Duration.ofMillis(10000L)) .setMaxAttempts(100) .build(); - ScheduledExecutorService localExecutor = Executors.newSingleThreadScheduledExecutor(); - try { - for (int executionsCount = 0; executionsCount < EXECUTIONS_COUNT; executionsCount++) { - + for (int executionsCount = 0; executionsCount < EXECUTIONS_COUNT; executionsCount++) { + ScheduledExecutorService localExecutor = Executors.newSingleThreadScheduledExecutor(); + try { FailingCallable callable = new FailingCallable(15, "request", "SUCCESS", tracer); RetryingExecutorWithContext executor = @@ -151,9 +150,9 @@ void testSuccessWithFailuresPeekAttempt() throws Exception { assertFutureSuccess(future); assertEquals(15, future.getAttemptSettings().getAttemptCount()); assertTrue(failedAttempts.get() > 0); + } finally { + localExecutor.shutdownNow(); } - } finally { - localExecutor.shutdownNow(); } } @@ -284,9 +283,9 @@ void testCancelOuterFutureAfterStart() throws Exception { // tiny RRD value is small, but not impossible. .setJittered(false) .build(); - ScheduledExecutorService localExecutor = Executors.newSingleThreadScheduledExecutor(); - try { - for (int executionsCount = 0; executionsCount < EXECUTIONS_COUNT; executionsCount++) { + for (int executionsCount = 0; executionsCount < EXECUTIONS_COUNT; executionsCount++) { + ScheduledExecutorService localExecutor = Executors.newSingleThreadScheduledExecutor(); + try { FailingCallable callable = new FailingCallable(4, "request", "SUCCESS", tracer); RetryingExecutorWithContext executor = getRetryingExecutor( @@ -317,9 +316,9 @@ void testCancelOuterFutureAfterStart() throws Exception { // i.e. The future from executor.submit() has been run by the ScheduledExecutor assertTrue(future.getAttemptSettings().getAttemptCount() > 0); assertTrue(future.getAttemptSettings().getAttemptCount() < 4); + } finally { + localExecutor.shutdownNow(); } - } finally { - localExecutor.shutdownNow(); } } From ab074f85245fcf00aecc31940c76808b0297496f Mon Sep 17 00:00:00 2001 From: Lawrence Qiu Date: Fri, 26 Jun 2026 16:32:49 +0000 Subject: [PATCH 3/3] style(gax): format code using spotify fmt plugin Applied fmt-maven-plugin to fix formatting issues in RecordingScheduler.java and ScheduledRetryingExecutorTest.java. TAG=agy CONV=71af906a-b7dc-4dda-a2fa-ebe24b5d8b91 BUG=13535 --- .../java/com/google/api/gax/core/RecordingScheduler.java | 1 - .../api/gax/retrying/ScheduledRetryingExecutorTest.java | 8 ++++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/core/RecordingScheduler.java b/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/core/RecordingScheduler.java index ce802eedabb0..944ef94de074 100644 --- a/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/core/RecordingScheduler.java +++ b/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/core/RecordingScheduler.java @@ -31,7 +31,6 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyLong; -import static org.mockito.Mockito.when; import java.util.ArrayList; import java.util.List; diff --git a/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java b/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java index 411dc0e6c076..198d7765e8f1 100644 --- a/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java +++ b/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java @@ -114,7 +114,9 @@ void testSuccessWithFailuresPeekAttempt() throws Exception { RetryingFuture future = executor.createFuture( callable, - FakeCallContext.createDefault().withTracer(tracer).withRetrySettings(retrySettings)); + FakeCallContext.createDefault() + .withTracer(tracer) + .withRetrySettings(retrySettings)); callable.setExternalFuture(future); assertNull(future.peekAttemptResult()); @@ -293,7 +295,9 @@ void testCancelOuterFutureAfterStart() throws Exception { RetryingFuture future = executor.createFuture( callable, - FakeCallContext.createDefault().withTracer(tracer).withRetrySettings(retrySettings)); + FakeCallContext.createDefault() + .withTracer(tracer) + .withRetrySettings(retrySettings)); callable.setExternalFuture(future); future.setAttemptFuture(executor.submit(future));