diff --git a/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT014.py b/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT014.py new file mode 100644 index 0000000000000..e81cbc15f5aab --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT014.py @@ -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, 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): + ... diff --git a/crates/ruff/src/checkers/ast/analyze/statement.rs b/crates/ruff/src/checkers/ast/analyze/statement.rs index aeb82605fd63e..7518c07523b0b 100644 --- a/crates/ruff/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff/src/checkers/ast/analyze/statement.rs @@ -300,6 +300,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.any_enabled(&[ Rule::PytestParametrizeNamesWrongType, Rule::PytestParametrizeValuesWrongType, + Rule::PytestDuplicateParametrizeTestCases, ]) { flake8_pytest_style::rules::parametrize(checker, decorator_list); } diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 79add843bc25f..8d011aafa196e 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -685,6 +685,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8PytestStyle, "011") => (RuleGroup::Unspecified, rules::flake8_pytest_style::rules::PytestRaisesTooBroad), (Flake8PytestStyle, "012") => (RuleGroup::Unspecified, rules::flake8_pytest_style::rules::PytestRaisesWithMultipleStatements), (Flake8PytestStyle, "013") => (RuleGroup::Unspecified, rules::flake8_pytest_style::rules::PytestIncorrectPytestImport), + (Flake8PytestStyle, "014") => (RuleGroup::Unspecified, rules::flake8_pytest_style::rules::PytestDuplicateParametrizeTestCases), (Flake8PytestStyle, "015") => (RuleGroup::Unspecified, rules::flake8_pytest_style::rules::PytestAssertAlwaysFalse), (Flake8PytestStyle, "016") => (RuleGroup::Unspecified, rules::flake8_pytest_style::rules::PytestFailWithoutMessage), (Flake8PytestStyle, "017") => (RuleGroup::Unspecified, rules::flake8_pytest_style::rules::PytestAssertInExcept), diff --git a/crates/ruff/src/rules/flake8_pytest_style/mod.rs b/crates/ruff/src/rules/flake8_pytest_style/mod.rs index 023ba3d23eef7..faa28606eba96 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/mod.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/mod.rs @@ -169,6 +169,12 @@ mod tests { Settings::default(), "PT013" )] + #[test_case( + Rule::PytestDuplicateParametrizeTestCases, + Path::new("PT014.py"), + Settings::default(), + "PT014" + )] #[test_case( Rule::PytestAssertAlwaysFalse, Path::new("PT015.py"), diff --git a/crates/ruff/src/rules/flake8_pytest_style/rules/parametrize.rs b/crates/ruff/src/rules/flake8_pytest_style/rules/parametrize.rs index 931e8e6107833..9165aef160b97 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/parametrize.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/parametrize.rs @@ -1,3 +1,6 @@ +use rustc_hash::FxHashMap; +use std::hash::BuildHasherDefault; + use ruff_python_ast::{ self as ast, Arguments, Constant, Decorator, Expr, ExprContext, PySourceType, Ranged, }; @@ -6,6 +9,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 +170,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 { + index: usize, +} + +impl Violation for PytestDuplicateParametrizeTestCases { + #[derive_message_formats] + fn message(&self) -> String { + let PytestDuplicateParametrizeTestCases { index } = self; + format!("Duplicate of test case at index {index} in `@pytest_mark.parametrize`") + } +} + fn elts_to_csv(elts: &[Expr], generator: Generator) -> Option { let all_literals = elts.iter().all(|expr| { matches!( @@ -472,6 +528,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 +551,29 @@ fn check_values(checker: &mut Checker, names: &Expr, values: &Expr) { } } +/// PT014 +fn check_duplicates(checker: &mut Checker, values: &Expr) { + let (Expr::List(ast::ExprList { elts, .. }) | Expr::Tuple(ast::ExprTuple { elts, .. })) = + values + else { + return; + }; + + let mut seen: FxHashMap = + FxHashMap::with_capacity_and_hasher(elts.len(), BuildHasherDefault::default()); + for (index, elt) in elts.iter().enumerate() { + let expr = ComparableExpr::from(elt); + seen.entry(expr) + .and_modify(|index| { + checker.diagnostics.push(Diagnostic::new( + PytestDuplicateParametrizeTestCases { index: *index }, + elt.range(), + )); + }) + .or_insert(index); + } +} + fn handle_single_name(checker: &mut Checker, expr: &Expr, value: &Expr) { let mut diagnostic = Diagnostic::new( PytestParametrizeNamesWrongType { @@ -567,6 +647,11 @@ pub(crate) fn parametrize(checker: &mut Checker, decorators: &[Decorator]) { } } } + if checker.enabled(Rule::PytestDuplicateParametrizeTestCases) { + if let [_, values, ..] = &args[..] { + check_duplicates(checker, values); + } + } } } } diff --git a/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT014.snap b/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT014.snap new file mode 100644 index 0000000000000..45369f5551b12 --- /dev/null +++ b/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT014.snap @@ -0,0 +1,44 @@ +--- +source: crates/ruff/src/rules/flake8_pytest_style/mod.rs +--- +PT014.py:4:35: PT014 Duplicate of test case at index 0 in `@pytest_mark.parametrize` + | +4 | @pytest.mark.parametrize("x", [1, 1, 2]) + | ^ PT014 +5 | def test_error_literal(x): +6 | ... + | + +PT014.py:14:35: PT014 Duplicate of test case at index 0 in `@pytest_mark.parametrize` + | +14 | @pytest.mark.parametrize("x", [a, a, b, b, b, c]) + | ^ PT014 +15 | def test_error_expr_simple(x): +16 | ... + | + +PT014.py:14:41: PT014 Duplicate of test case at index 2 in `@pytest_mark.parametrize` + | +14 | @pytest.mark.parametrize("x", [a, a, b, b, b, c]) + | ^ PT014 +15 | def test_error_expr_simple(x): +16 | ... + | + +PT014.py:14:44: PT014 Duplicate of test case at index 2 in `@pytest_mark.parametrize` + | +14 | @pytest.mark.parametrize("x", [a, a, b, b, b, c]) + | ^ PT014 +15 | def test_error_expr_simple(x): +16 | ... + | + +PT014.py:19:40: PT014 Duplicate of test case at index 0 in `@pytest_mark.parametrize` + | +19 | @pytest.mark.parametrize("x", [(a, b), (a, b), (b, c)]) + | ^^^^^^ PT014 +20 | def test_error_expr_complex(x): +21 | ... + | + + diff --git a/ruff.schema.json b/ruff.schema.json index ed2098be3e155..04ccd6b10fb99 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2322,6 +2322,7 @@ "PT011", "PT012", "PT013", + "PT014", "PT015", "PT016", "PT017",