-
Notifications
You must be signed in to change notification settings - Fork 5
Increase wait time on rerun for flakey tests #359
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
Conversation
Signal tests fail sometimes, probably due to a race condition. On slow startup on some CI systems (I've never seen locally) the signal could be sent before the signal handler is registered. If that is whats happening, the test fails, and on the next rerun the wait time inceases.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #359 +/- ##
=======================================
Coverage 90.98% 90.98%
=======================================
Files 8 8
Lines 1021 1021
Branches 135 135
=======================================
Hits 929 929
Misses 70 70
Partials 22 22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR aims to address flaky signal handling tests that occasionally fail on slow CI systems due to race conditions where signals are sent before handlers are registered. The changes add retry logic using pytest-rerunfailures and attempt to scale wait times based on retry attempts.
- Adds
@pytest.mark.flaky(reruns=5)decorator to both signal tests to enable automatic retries - Injects the
requestfixture to access attempt/execution count information - Implements dynamic wait time scaling based on retry attempts (0.2 * attempt)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Who knows, might not be necessary after all Recent runs on #351 have looked pretty stable |
|
I'll leave this open as a draft then-- race condition flakes have haunted the project for a while, so this pattern might prove to be useful elsewhere. |
|
@candleindark this will hopefully solve the flakey tests you saw. |
|
@CodyCBakerPhD maybe reviewing this so I can merge before more intel work would save you a headache :) |
|
Can't hurt? |
Hopefully Fixes: #190
Signal tests fail sometimes, probably due to a race condition. On slow startup on some CI systems (I've never seen locally) the signal could be sent before the signal handler is registered. If that is whats happening, the test fails, and on the next rerun the wait time inceases.
With this retry logic, it seems worth also dropping the wait time a little bit.
@CodyCBakerPhD this will hopefully solve a lot of the flake you've been seeing.
@candleindark tagging you too since you recently dug into signal handling :)