Add --max-failures <n> option in test harness #25569
Merged
+36
−24
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
sbc100
reviewed
Oct 16, 2025
| if options.failfast: | ||
| if options.max_failures != 0: | ||
| utils.exit_with_error('--failfast and --max-failures are mutually exclusive!') | ||
| options.max_failures = 0 |
There was a problem hiding this comment.
I took a convention of max_failures to mean "maximum allowed failures before aborting the test run".
So max_failures = 0 means that zero failures are allowed, after that test run is aborted. (first failure will abort)
max_failures = 1 means that one failure is allowed, and the second one after that aborts the run.
…failfast option (which is effectively a '--max-failures 0' mode).
sbc100
reviewed
Oct 16, 2025
| help='Use the default CI browser configuration.') | ||
| parser.add_argument('tests', nargs='*') | ||
| parser.add_argument('--failfast', action='store_true') | ||
| parser.add_argument('--failfast', action='store_true', help='If true, test run will abort on first failed test.') |
There was a problem hiding this comment.
Actually I wonder if you can you do something action=store_constant + target=max_failures here? Maybe too clever.
lgtm either way.
There was a problem hiding this comment.
Hmm there are still a couple of uses of the options.failfast variable that this PR does not change, so keeping that around for now helps.
|
Anything more here? |
sbc100
approved these changes
Oct 16, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Add --max-failures option in test harness to generalize on the --failfast option (which is effectively a '--max-failures 0' mode).
This allows running the harness while allowing a couple of failures. I find that when using
--failfast, aborting on the first error is sometimes a bit too eager, i.e. there might be a flaky test that triggers the abort, or there might be an error that causes more than one test to fail.Examining test harness results, that interpreting a small handful of failures (1,2,3...5) is still doable one-by-one, but when the number of failures grows to, say, > 5, the typically I want to deal with the first couple of errors only, and re-run the harness after that, assuming the rest of the errors would be from the same root cause.
So this enables running with, say,
--max-failures 5, and get info on the first handful of errors, and ignore if any more errors occur.This helps retain the failfast-like behavior while producing a bit more info, and being a bit more tolerant to instabilities.