Solve and enforce ruff pytest ruleset #12569
Merged
+608
−588
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
* Why is this bad? `pytest.warns(Warning)` will catch any `Warning` and may catch warnings that are unrelated to the code under test. To avoid this, `pytest.warns` should be called with a `match` parameter.
* Why is this bad? Such a parameter will always have the default value during the test regardless of whether a fixture with the same name is defined.
* What it does Checks for unnecessary `request.addfinalizer` usages in `pytest` fixtures. * Why is this bad? `pytest` offers two ways to perform cleanup in fixture code. The first is sequential (via the `yield` statement), the second callback-based (via `request.addfinalizer`). The sequential approach is more readable and should be preferred, unless the fixture uses the "factory as fixture" pattern.
* What it does Checks for `assert` statements in `except` clauses. * Why is this bad? When testing for exceptions, `pytest.raises()` should be used instead of `assert` statements in `except` clauses, as it's more explicit and idiomatic. Further, `pytest.raises()` will fail if the exception is _not_ raised, unlike the `assert` statement.
* What it does Checks for `pytest.warns` context managers with multiple statements. This rule allows `pytest.warns` bodies to contain `for` loops with empty bodies (e.g., `pass` or `...` statements), to test iterator behavior. * Why is this bad? When `pytest.warns` is used as a context manager and contains multiple statements, it can lead to the test passing when it should instead fail. A `pytest.warns` context manager should only contain a single simple statement that triggers the expected warning.
* What it does Checks for incorrect import of pytest. * Why is this bad? For consistency, `pytest` should be imported as `import pytest` and its members should be accessed in the form of `pytest.xxx.yyy` for consistency
* What it does Checks for `pytest` test functions that should be decorated with `@pytest.mark.usefixtures`. * Why is this bad? In `pytest`, fixture injection is used to activate fixtures in a test function. Fixtures can be injected either by passing them as parameters to the test function, or by using the `@pytest.mark.usefixtures` decorator. If the test function depends on the fixture being activated, but does not use it in the test body or otherwise rely on its return value, prefer the `@pytest.mark.usefixtures` decorator, to make the dependency explicit and avoid the confusion caused by unused arguments.
* What it does Checks for assertions that combine multiple independent conditions. * Why is this bad? Composite assertion statements are harder to debug upon failure, as the failure message will not indicate which condition failed.
* What it does Checks for duplicate test cases in `pytest.mark.parametrize`. * Why is this bad? Duplicate test cases are redundant and should be removed.
* What it does Checks for argument-free `@pytest.fixture()` decorators with or without parentheses. * Why is this bad? If a `@pytest.fixture()` doesn't take any arguments, the parentheses are optional. Either removing those unnecessary parentheses _or_ requiring them for all fixtures is fine, but it's best to be consistent. The rule defaults to removing unnecessary parentheses, to match the documentation of the official pytest projects.
* What it does Checks for unnecessary `yield` expressions in `pytest` fixtures. * Why is this bad? In `pytest` fixtures, the `yield` expression should only be used for fixtures that include teardown code, to clean up the fixture after the test function has finished executing.
* What it does Checks for `pytest.raises` calls without a `match` parameter. * Why is this bad? `pytest.raises(Error)` will catch any `Error` and may catch errors that are unrelated to the code under test. To avoid this, `pytest.raises` should be called with a `match` parameter.
berland
added
the
release-notes:skip
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12569 +/- ##
==========================================
- Coverage 90.58% 90.56% -0.02%
==========================================
Files 435 435
Lines 29915 29916 +1
==========================================
- Hits 27098 27093 -5
- Misses 2817 2823 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
* What it does Checks for `pytest.raises` context managers with multiple statements. This rule allows `pytest.raises` bodies to contain `for` loops with empty bodies (e.g., `pass` or `...` statements), to test iterator behavior. * Why is this bad? When a `pytest.raises` is used as a context manager and contains multiple statements, it can lead to the test passing when it actually should fail. A `pytest.raises` context manager should only contain a single simple statement that raises the expected exception.
* What it does Checks for the type of parameter values passed to `pytest.mark.parametrize`. * Why is this bad? The `argvalues` argument of `pytest.mark.parametrize` takes an iterator of parameter values, which can be provided as lists or tuples. To aid in readability, it's recommended to use a consistent style for the list of values rows, and, in the case of multiple parameters, for each row of values.
Resolved using "ruff --fix --unsafe-fixes" * What it does Checks for the type of parameter names passed to `pytest.mark.parametrize`. * Why is this bad? The `argnames` argument of `pytest.mark.parametrize` takes a string or a sequence of strings. For a single parameter, it's preferable to use a string. For multiple parameters, it's preferable to use the style configured via the [`lint.flake8-pytest-style.parametrize-names-type`] setting.
berland
force-pushed
the
ruff_pytest_rule_by_rule
branch
from
a139e6b to
690a460
Compare
CodSpeed Performance ReportMerging #12569 will not alter performanceComparing Summary
|
verveerpj
approved these changes
Jan 6, 2026
Contributor
verveerpj
left a comment
verveerpj
left a comment
There was a problem hiding this comment.
Apart from one small comment, LGTM!
| ): | ||
| deserialized_workflow_job = ErtScriptWorkflow.model_validate(serialized_job) | ||
| assert deserialized_workflow_job == ertscript_workflow_job | ||
| ErtScriptWorkflow.model_validate(serialized_job) |
Contributor
There was a problem hiding this comment.
Is it valid to remove the assert?
Contributor
Author
There was a problem hiding this comment.
Good observation! This is one of the "bugs" that ruff exposes to us. The assert has never been run, since there is always a KeyError on the line above, and the assert is not even valid (deserialized_workflow_job is never bound).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Issue
Resolves
ruff check --select PTApproach
Mostly manual fixes, one commit pr. rule. Biggest commit is fixed by ruff itself with `--unsafe-fixes .
git rebase -i main --exec 'just rapid-tests')When applicable