fix(bucket-retriever): Fix flaky tests by removing dependency on sleeping in tests #7223
Conversation
Generate changelog in
|
| // Exists to facilitate testing in unit tests, rather than needing to mock out ThreadLocalRandom. | ||
| private final Supplier<Long> backoffMillisGenerator; | ||
| // Exists to facilitate testing in unit tests, rather than needing to mock out ThreadLocalRandom and Thread#sleep. | ||
| private final RunnableCheckedException<InterruptedException> sleeper; |
There was a problem hiding this comment.
open to another name
| private final TestShardedRetrievalStrategy strategy = new TestShardedRetrievalStrategy(); | ||
| private final TestParallelTaskExecutor parallelTaskExecutor = new TestParallelTaskExecutor(); | ||
| private final ExecutorService executorService = PTExecutors.newSingleThreadScheduledExecutor(); | ||
| private final AtomicBoolean wasSleepCalled = new AtomicBoolean(false); |
There was a problem hiding this comment.
Copying concern:
wasSleepCalled in tests - it's only read once in the tests, didn't want to mock unnecessarily, but figured I might as well inject it for all rather than have two separate functions. If you feel strongly, I can just switch the default for tests to be () -> {} and recreate the retriever as needed in the new test.
| // Even though the backoff in 10s, interrupting the task should make it finish much faster. | ||
| Awaitility.await().atMost(Duration.ofMillis(200)).untilAsserted(() -> assertThat( | ||
| // Even though the backoff is up to 10 minutes, interrupting the task should make it finish much faster. | ||
| Awaitility.await().atMost(Duration.ofSeconds(1)).untilAsserted(() -> assertThat( |
There was a problem hiding this comment.
defaultSleeperCanBeInterrupted will involve randomness once again, as we're not pinning the backoff value (it's back to being controlled by TLR) - and so it could spuriously pass. It's a 0.16% chance for that to happen ((1000 / 600000) * 100) - I can reduce the odds even further by bumping the maxBackoff, but in cases where it fails legitimately, CI will just spin.
bulldozer-bot
bot
deleted the
mdaudali/08-06-fix_bucket-retriever_fix_flaky_tests_by_removing_dependency_on_sleeping_in_tests
branch
General
Before this PR:
Test flaked, because the timing requirement was too tight: https://app.circleci.com/pipelines/github/palantir/atlasdb/18418/workflows/867c6fe8-a6e6-4de0-b87e-2fec4f2ee56c/jobs/114507/tests
After this PR:
Removes dependency on testing the code that uses thread.sleep by injecting a dependency that sleeps.
I didn't want to expand the bound on the existing await code because that would make test practically useless (a bound of 100ms to >1 second!)
==COMMIT_MSG==
==COMMIT_MSG==
Priority: P2
Concerns / possible downsides (what feedback would you like?):
It's a little jank injecting the function that sleeps - we don't do it elsewhere (but I guess it is a dependency!)
Naming - I went with sleeper, open to anything else.
wasSleepCalled in tests - it's only read once in the tests, didn't want to mock unnecessarily, but figured I might as well inject it for all rather than have two separate functions. Can just switch the default for tests to be () -> {} and recreate the retriever as needed in the new test.
defaultSleeperCanBeInterruptedwill involve randomness once again, as we're not pinning the backoff value (it's back to being controlled by TLR) - and so it could spuriously pass. It's a 0.16% chance for that to happen ((1000 / 600000) * 100) - I can reduce the odds even further by bumping the maxBackoff, but in cases where it fails legitimately, CI will justspin.
Testing and Correctness
What, if any, assumptions are made about the current state of the world? If they change over time, how will we find out?:
That flakes are bad
What was existing testing like? What have you done to improve it?:
Flaky, now less flaky!
Development Process
Where should we start reviewing?:
SSBR