chore: Fix testCancelGetAttempt flaky test #3592
Conversation
| assertFutureCancel(future); | ||
| localExecutor.shutdownNow(); | ||
| } | ||
| @Test |
There was a problem hiding this comment.
Remove parameterized tests since it just chooses different retry settings. Only need to run this for a single set of retry settings.
|
|
| @MethodSource("data") | ||
| void testCancelGetAttempt(boolean withCustomRetrySettings) throws Exception { | ||
| setUp(withCustomRetrySettings); | ||
| for (int executionsCount = 0; executionsCount < EXECUTIONS_COUNT; executionsCount++) { |
There was a problem hiding this comment.
can you explain a bit why we are removing this loop?
There was a problem hiding this comment.
I'm not sure why this even needs a loop to begin with. IIUC, the test should care that the future was cancelled after the first attempt. Running multiple invocations doesn't seem to be necessary.
| .setInitialRpcTimeoutDuration(java.time.Duration.ofMillis(50L)) | ||
| .setMaxRpcTimeoutDuration(java.time.Duration.ofMillis(50L)) |
There was a problem hiding this comment.
these are explicitly set instead of defaults to have quicker cancellations?
There was a problem hiding this comment.
No, these were increased to give additional buffer time. IIRC, the original values were 8ms.
| // this is a heavy test, which takes a lot of time, so only few executions. | ||
| for (int executionsCount = 0; executionsCount < 2; executionsCount++) { | ||
| // Use a test local executor for this test case due to the reasons listed below |
There was a problem hiding this comment.
Do you mean above?
| // Use a test local executor for this test case due to the reasons listed below | |
| // Use a test local executor for this test case due to the reasons listed above |
There was a problem hiding this comment.
No, is to reference the comments below in L312-316. I kept this the same since I don't fully understand the impacts of changing it.
| // future.cancel(true) may return false sometimes, which is ok. Also, the every cancellation | ||
| // of | ||
| // an already cancelled future should return false (this is what -1 means here) | ||
| assertEquals(2, checks - (failedCancelations - 1)); |
There was a problem hiding this comment.
Is this the failing point for #2851? Do you think it is redundant?
There was a problem hiding this comment.
I'm not sure what you're referring to. The test is to ensure that the future was cancelled during the retry attempt.
Future should be reported as done and that one attempt was made.
Fixes #2851