Fix flaky test in SemaphoreSpec #9492
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hearnadam
approved these changes
Jan 23, 2025
| _ <- promise.succeed(()) | ||
| waitingEnd <- semaphore.awaiting.repeatUntil(_ == 0) | ||
| } yield assertTrue(waitingStart == 10, waitingEnd == 0) | ||
| } @@ timeout(10.seconds) |
There was a problem hiding this comment.
Can we lower the timeout if we lower the number of waiters?
There was a problem hiding this comment.
We can although I'm a bit reluctant lowering timeouts too much as the CI server runs with limited CPU/threads and the tests run in parallel, so it's possible that adding another CPU-intensive test might trigger a timeout in this one if we set it too low.
Also note that 10 seconds should be the total time of the test (100 iterations due to nonFlaky)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm actually surprised this test hasn't been flaky more often. Since the first invocation of
semaphore.withPermit(promise.await)was forked, if the forking in the line below won the race then the test would never complete.Fixed it by making all of semaphore.await on the promise, so that we can guarantee none of them wins the race and completes. Also annotated the entire suite with
nonFlakyjust in case