Add --max-warnings option to fail test runs#14372
Add --max-warnings option to fail test runs#14372miketheman wants to merge 6 commits intopytest-dev:mainfrom
--max-warnings option to fail test runs#14372Conversation
| :Exit code 3: Internal error happened while executing tests | ||
| :Exit code 4: pytest command line usage error | ||
| :Exit code 5: No tests were collected | ||
| :Exit code 6: Maximum number of warnings exceeded (see :option:`--max-warnings`) |
There was a problem hiding this comment.
I debated on whether to add an explicit code, and since this doesn't match ExitCode 1, I chose to add another.
|
This looks like a very useful addition, especially for teams trying to gradually reduce warnings without immediately failing builds. One thought from a testing perspective: it might be helpful to include test coverage for boundary conditions around the threshold (e.g., exactly at the limit vs exceeding it) to ensure consistent behavior. Also, since this introduces a new exit code, a brief note or example in the documentation about how this interacts with CI/CD pipelines (e.g., build failure handling) could improve usability. Overall, this seems like a clean and practical enhancement. |
webknjaz
left a comment
There was a problem hiding this comment.
I tend to use filterwarnings for addressing warnings gradually. With the default set to error. It's probably a good idea to document how these two approaches are supposed to co-exist. And maybe even add tests for the respective cases.
nicoddemus
left a comment
There was a problem hiding this comment.
Thanks, this does look like a good addition.
The implementation is great and thorough, please take a look at my suggestions.
Allow users to set a maximum number of allowed warnings via --max-warnings CLI option or max_warnings config option. When the warning count exceeds the threshold and all tests pass, pytest exits with a new WARNINGS_ERROR exit code (6). This supports gradually ratcheting down warnings in a codebase without converting them all to errors. Based on the pytest-max-warnings plugin by @miketheman, with improvements: a dedicated ExitCode instead of an arbitrary exit code, and INI config support. Closes pytest-dev#14371 Signed-off-by: Mike Fiedler <miketheman@gmail.com>
- documentation improvements - test cases with filterwarnings Signed-off-by: Mike Fiedler <miketheman@gmail.com>
80864bd to
60e336c
Compare
for more information, see https://pre-commit.ci
nicoddemus
left a comment
There was a problem hiding this comment.
Thanks, great contribution!
webknjaz
left a comment
There was a problem hiding this comment.
Looks good, but I have one final docs suggestion to consider.
There was a problem hiding this comment.
So the filterwarnings examples show the best practice of starting with the error value from the beginning. And I'm thinking that considering that docs examples tend to end up in people's projects unchanged, it'd be good to show the new setting set to 0. Personally, I'll build that into my configs once this is shipped.
There was a problem hiding this comment.
I disagree a bit here. I think that folks that have warnings may explore this and use to ratchet down. If you're already a warnings expert 😉 you're likely already using filterwarnings: error:* or such, then this feature does not really apply to those folks.
Supplying a default of 0 (or even 1) has the potential to confuse the reader, as 0/1 values are also often used to denote true/false in other configurations (not pytest as far as I know), so showing a "real" number makes it clearer to me.
There was a problem hiding this comment.
I mean.. We could have a code comment in the config examples:
# Zero allowed warnings errors out on any warning leaks unlike the unset default which is a no-op
I think you raise an interesting UX point regarding disabling the feature, though — it's not documented how to disable a pre-enabled setting. Imagine having this set in the config file but wanting to override one for a one-time CLI invocation. For an external plugin, you'd just do a -p no:plugin_importable (except for when it's set via adopts that would break the parser). But how does an end-user reset the plugin to the default/unset value? Some plugins introduce additional options (pytest-cov has --no-cov and --cov-reset, for example). It's probably a good idea to special-case an empty string so people could do a --max-warnings= w/o a numeric value that would disable the check. Or use some kind of a sentinel value.
Allow users to set a maximum number of allowed warnings via --max-warnings CLI option or max_warnings config option. When the warning count exceeds the threshold and all tests pass, pytest exits with a new WARNINGS_ERROR exit code (6). This supports gradually ratcheting down warnings in a codebase without converting them all to errors.
Based on the pytest-max-warnings plugin by @miketheman, with improvements: a dedicated ExitCode instead of an arbitrary exit code, and INI config support.
Closes #14371