-
Notifications
You must be signed in to change notification settings - Fork 2.3k
[flake8-pytest-style] Implement duplicate parameterized fixture detection (PT014)
#6598
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
Changes from 7 commits
5073847
17a8bdc
4b8529e
e5cd579
0d96a04
c6eb566
2efb740
ba46c21
9d5dce4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import pytest | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("x", [1, 1, 2]) | ||
| def test_error_literal(x): | ||
| ... | ||
|
|
||
|
|
||
| a = 1 | ||
| b = 2 | ||
| c = 3 | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("x", [a, a, b, b, c]) | ||
| def test_error_expr_simple(x): | ||
| ... | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("x", [(a, b), (a, b), (b, c)]) | ||
| def test_error_expr_complex(x): | ||
| ... | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("x", [1, 2]) | ||
| def test_ok(x): | ||
| ... |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| use itertools::Itertools; | ||
|
|
||
| use ruff_python_ast::{ | ||
| self as ast, Arguments, Constant, Decorator, Expr, ExprContext, PySourceType, Ranged, | ||
| }; | ||
|
|
@@ -6,6 +8,7 @@ use ruff_text_size::TextRange; | |
|
|
||
| use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; | ||
| use ruff_macros::{derive_message_formats, violation}; | ||
| use ruff_python_ast::comparable::ComparableExpr; | ||
| use ruff_python_codegen::Generator; | ||
| use ruff_source_file::Locator; | ||
|
|
||
|
|
@@ -166,6 +169,58 @@ impl Violation for PytestParametrizeValuesWrongType { | |
| } | ||
| } | ||
|
|
||
| /// ## 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. | ||
| /// | ||
| /// ## Example | ||
| /// ```python | ||
| /// import pytest | ||
| /// | ||
| /// | ||
| /// @pytest.mark.parametrize( | ||
| /// ("param1", "param2"), | ||
| /// [ | ||
| /// (1, 2), | ||
| /// (1, 2), | ||
| /// ], | ||
| /// ) | ||
| /// def test_foo(param1, param2): | ||
| /// ... | ||
| /// ``` | ||
| /// | ||
| /// Use instead: | ||
| /// ```python | ||
| /// import pytest | ||
| /// | ||
| /// | ||
| /// @pytest.mark.parametrize( | ||
| /// ("param1", "param2"), | ||
| /// [ | ||
| /// (1, 2), | ||
| /// ], | ||
| /// ) | ||
| /// def test_foo(param1, param2): | ||
| /// ... | ||
| /// ``` | ||
| /// | ||
| /// ## References | ||
| /// - [`pytest` documentation: How to parametrize fixtures and test functions](https://docs.pytest.org/en/latest/how-to/parametrize.html#pytest-mark-parametrize) | ||
| #[violation] | ||
| pub struct PytestDuplicateParametrizeTestCases { | ||
| pub indices: (usize, usize), | ||
| } | ||
|
|
||
| impl Violation for PytestDuplicateParametrizeTestCases { | ||
| #[derive_message_formats] | ||
| fn message(&self) -> String { | ||
| let PytestDuplicateParametrizeTestCases { indices } = self; | ||
| format!("Found duplicate test cases {indices:?} in `@pytest.mark.parametrize`") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we say "at indices ..."? I wasn't sure what the message meant at first.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we raise PT014 against items that should be removed? This allows us to remove indices.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it makes sense to use the range of the duplicated item, so that we underline the duplicated item specifically
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we have multiple duplicated items, we underline them?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we support highlighting multiple ranges. In that case, they'd each need to be a new violation which seems okay.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think in the duplicate value rule, we just highlight the second value (i.e., the one that is a duplicate). That seems reasonable to me. (We could also mention the index of which it's a duplicate in the message.)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so it should look like this?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, and the same for the next 100 -- one violation for each duplicate (but no violation for the first |
||
| } | ||
| } | ||
|
|
||
| fn elts_to_csv(elts: &[Expr], generator: Generator) -> Option<String> { | ||
| let all_literals = elts.iter().all(|expr| { | ||
| matches!( | ||
|
|
@@ -472,6 +527,7 @@ fn check_values(checker: &mut Checker, names: &Expr, values: &Expr) { | |
| values.range(), | ||
| )); | ||
| } | ||
|
|
||
| if is_multi_named { | ||
| handle_value_rows(checker, elts, values_type, values_row_type); | ||
| } | ||
|
|
@@ -494,6 +550,31 @@ fn check_values(checker: &mut Checker, names: &Expr, values: &Expr) { | |
| } | ||
| } | ||
|
|
||
| fn find_duplicates(elts: &[Expr]) -> Vec<(usize, usize)> { | ||
| let mut duplicates: Vec<(usize, usize)> = Vec::new(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we use a
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tend to prefer |
||
| for ((idx1, elt1), (idx2, elt2)) in elts.iter().enumerate().tuple_combinations() { | ||
| if ComparableExpr::from(elt1) == ComparableExpr::from(elt2) { | ||
| duplicates.push((idx1 + 1, idx2 + 1)); | ||
| } | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'd suggest using an |
||
| duplicates | ||
| } | ||
|
|
||
| /// PT014 | ||
| fn check_duplicates(checker: &mut Checker, values: &Expr) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it feasible to factor this so the type of |
||
| match values { | ||
| Expr::List(ast::ExprList { elts, .. }) | Expr::Tuple(ast::ExprTuple { elts, .. }) => { | ||
| for indices in find_duplicates(elts) { | ||
| checker.diagnostics.push(Diagnostic::new( | ||
| PytestDuplicateParametrizeTestCases { indices }, | ||
| values.range(), | ||
| )); | ||
| } | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
|
|
||
| fn handle_single_name(checker: &mut Checker, expr: &Expr, value: &Expr) { | ||
| let mut diagnostic = Diagnostic::new( | ||
| PytestParametrizeNamesWrongType { | ||
|
|
@@ -567,6 +648,11 @@ pub(crate) fn parametrize(checker: &mut Checker, decorators: &[Decorator]) { | |
| } | ||
| } | ||
| } | ||
| if checker.enabled(Rule::PytestDuplicateParametrizeTestCases) { | ||
| if let [_, values, ..] = &args[..] { | ||
| check_duplicates(checker, values); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| --- | ||
| source: crates/ruff/src/rules/flake8_pytest_style/mod.rs | ||
| --- | ||
| PT014.py:4:31: PT014 Found duplicate test cases (1, 2) in `@pytest.mark.parametrize` | ||
| | | ||
| 4 | @pytest.mark.parametrize("x", [1, 1, 2]) | ||
| | ^^^^^^^^^ PT014 | ||
| 5 | def test_error_literal(x): | ||
| 6 | ... | ||
| | | ||
|
|
||
| PT014.py:14:31: PT014 Found duplicate test cases (1, 2) in `@pytest.mark.parametrize` | ||
| | | ||
| 14 | @pytest.mark.parametrize("x", [a, a, b, b, c]) | ||
| | ^^^^^^^^^^^^^^^ PT014 | ||
| 15 | def test_error_expr_simple(x): | ||
| 16 | ... | ||
| | | ||
|
|
||
| PT014.py:14:31: PT014 Found duplicate test cases (3, 4) in `@pytest.mark.parametrize` | ||
| | | ||
| 14 | @pytest.mark.parametrize("x", [a, a, b, b, c]) | ||
| | ^^^^^^^^^^^^^^^ PT014 | ||
| 15 | def test_error_expr_simple(x): | ||
| 16 | ... | ||
| | | ||
|
|
||
| PT014.py:19:31: PT014 Found duplicate test cases (1, 2) in `@pytest.mark.parametrize` | ||
| | | ||
| 19 | @pytest.mark.parametrize("x", [(a, b), (a, b), (b, c)]) | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^ PT014 | ||
| 20 | def test_error_expr_complex(x): | ||
| 21 | ... | ||
| | | ||
|
|
||
|
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.
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.
flake8_pytest_style result: