Activate nightly workflow for random and stress tests#3500
Draft
kdeldycke wants to merge 1 commit into
Draft
Conversation
kdeldycke
added
f:test runner
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
In #3139 we added stress tests and in #3151 we added randomization and parallel tests.
We decided to not activate them by default in #3151 (comment) and rely on contributors diligence to run them by hand to catch regressions. In practice nobody does. I even often forget to run them on my own during my sessions, only focused on the task at hand.
I'd like to re-assess this decision in light of regressions that could have been caught if these test were active. See the following PRs:
Regression introduced in 8.4.0, could have been caught by
tests/test_stream_lifecycle.py.echo_via_pager#3499New stress test covering the root cause of fix: Skip flaky pager test on macOS with free-threaded Python 3.14t #3470.
The bug exists because CI doesn't run tests in parallel.
click.testing.StreamMixer's finalization, seen in a multi-threaded context #2991The reason why stress test exist: I originally only hit the race about once in 10k tries, so this seems difficult to test systematically..
NamedTextIOWrapper: stop closing buffer #3139Fix the regression from fix a race in
click.testing.StreamMixer's finalization, seen in a multi-threaded context #2991, where only with the introduction of stress test keeps from recurring.The random test already caught a leak nobody noticed via the default suite.
Also why even support parallel tests if we don't exercice them regularly? (ref: #2899)
Currently these tests adds ~15 minutes run-time. If you find this too long and invasive, I propose a compromise and run them nightly. But my preference would be to run them as part of the standard CI to incentivize contributors. And reduce surprises and and follow-up fixes.
Given the reversal in policy this change imply, I'm aiming for the next 8.5.0 minor release. I'm opening this PR as a way to discuss the best strategy forward.