From bffb72dfd2e72d04cfa0eeb21fedd7933eec900b Mon Sep 17 00:00:00 2001 From: Diego Marquez Date: Tue, 16 Aug 2022 18:19:39 +0000 Subject: [PATCH 1/2] fix(test): testThrottlingBlocking flakyness fix When BatcherImpl.sendOutstanding (method that calls callContext.withOption) is called from PushCurrentBatchRunnable, it uses the batcher's own executor, causing the mockito capture to fail due to be in a different thread from the one that originated add(). Other times, sendOutstanding will be called from the parent thread (successful test, in this case from our newFixedThreadPool). ALthough this is expected behavior, the argument captors do not work when used in different threads. Mockito.verify() is the recommended way of dealing with argument captors and happened to be the fix Internally, verify() relies on thread safe(r) notifiers for when the captors have been interacted with. When verifying flakiness, 100 runs in two threads were used with bazel test //gax:com.google.api.gax.batching.BatcherImplTest --runs_per_test=100 --test_filter Throttling --jobs 2 Mockito.verify() implementation: https://github.com/mockito/mockito/blob/2ded10ec704f2c93648d976031c2520a5a9b84aa/src/main/java/org/mockito/internal/MockitoCore.java#L183 --- .../java/com/google/api/gax/batching/BatcherImplTest.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/gax/src/test/java/com/google/api/gax/batching/BatcherImplTest.java b/gax/src/test/java/com/google/api/gax/batching/BatcherImplTest.java index 390dc60d5..105cd7628 100644 --- a/gax/src/test/java/com/google/api/gax/batching/BatcherImplTest.java +++ b/gax/src/test/java/com/google/api/gax/batching/BatcherImplTest.java @@ -887,10 +887,15 @@ public void run() { } try { - future.get(100, TimeUnit.MILLISECONDS); + future.get(3, TimeUnit.SECONDS); } catch (TimeoutException e) { assertWithMessage("adding elements to batcher should not be blocked").fail(); } + + // Mockito recommends using verify() as the ONLY recommended way to interact with Argument + // captors - otherwise it may incur in unexpected behaviour + Mockito.verify(callContext, Mockito.timeout(0)).withOption(key.capture(), value.capture()); + // Verify that throttled time is recorded in ApiCallContext assertThat(key.getValue()).isSameInstanceAs(Batcher.THROTTLED_TIME_KEY); assertThat(value.getValue()).isAtLeast(throttledTime); From 8f34c53afbfa2a517b0683c1c8a34113d0fa175e Mon Sep 17 00:00:00 2001 From: Diego Marquez Date: Tue, 16 Aug 2022 20:13:59 +0000 Subject: [PATCH 2/2] fix(test): remove unnecessary timeout in verify --- .../java/com/google/api/gax/batching/BatcherImplTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gax/src/test/java/com/google/api/gax/batching/BatcherImplTest.java b/gax/src/test/java/com/google/api/gax/batching/BatcherImplTest.java index 105cd7628..b503e28da 100644 --- a/gax/src/test/java/com/google/api/gax/batching/BatcherImplTest.java +++ b/gax/src/test/java/com/google/api/gax/batching/BatcherImplTest.java @@ -892,9 +892,9 @@ public void run() { assertWithMessage("adding elements to batcher should not be blocked").fail(); } - // Mockito recommends using verify() as the ONLY recommended way to interact with Argument + // Mockito recommends using verify() as the ONLY way to interact with Argument // captors - otherwise it may incur in unexpected behaviour - Mockito.verify(callContext, Mockito.timeout(0)).withOption(key.capture(), value.capture()); + Mockito.verify(callContext).withOption(key.capture(), value.capture()); // Verify that throttled time is recorded in ApiCallContext assertThat(key.getValue()).isSameInstanceAs(Batcher.THROTTLED_TIME_KEY);