-
Notifications
You must be signed in to change notification settings - Fork 2.1k
[Closing] Replace Thread.sleep with synchronization primitives in tests #4426
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Copilot The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…Thread.yield Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com>
|
@copilot this just makes the sleeps shorter, the goal is to replace the sleeps with synchronization primitives which explicitly wait for the conditions which the sleep is passively waiting for. fwiw, this may be outside of your skill set, if so, please close this PR. |
You're right. Looking at PR #1219, the proper pattern is to use |
As noted by the reviewer, this PR just makes the sleeps shorter or wraps them in polling loops - it doesn't actually replace them with proper synchronization primitives.
The correct approach (as shown in PR #1219) is to:
Semaphorewith permits equal to the number of expected eventsrelease()a permit when the condition is metacquire()again to block until all expected events occurThis requires understanding what each sleep is actually waiting for and modifying the production or test code to signal when those conditions are met - which is beyond what I was able to accomplish correctly.
Original description (incorrect approach):
Tests using
Thread.sleep()are inherently flaky on overloaded CI/CD systems. When machines are under heavy load, fixed sleep durations become insufficient, causing intermittent failures.Replaced 8 occurrences of(This was not done correctly - changes just shortened sleeps or wrapped them in polling loops)Thread.sleep()across 7 test files with proper synchronizationOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.