From 9b52ae8991b5ad67cd3624f12ed839002e3ff438 Mon Sep 17 00:00:00 2001 From: Victor Hugo Gomes Date: Wed, 14 May 2025 11:31:42 -0300 Subject: [PATCH] [`flake8-pytest-style`] Don't recommend `usefixtures` for parametrize values in `PT019` (#17650) ## Summary Fixes #17599. ## Test Plan Snapshot tests. --------- Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com> --- .../fixtures/flake8_pytest_style/PT019.py | 35 +++++++++++++ .../flake8_pytest_style/rules/fixture.rs | 49 +++++++++++++++++-- ...es__flake8_pytest_style__tests__PT019.snap | 17 ++++++- 3 files changed, 97 insertions(+), 4 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pytest_style/PT019.py b/crates/ruff_linter/resources/test/fixtures/flake8_pytest_style/PT019.py index 1df67fd7f8..2bdbe93116 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_pytest_style/PT019.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pytest_style/PT019.py @@ -12,3 +12,38 @@ def test_xxx(_fixture): # Error arg def test_xxx(*, _fixture): # Error kwonly pass + +# https://github.com/astral-sh/ruff/issues/17599 + +@pytest.mark.parametrize("_foo", [1, 2, 3]) +def test_thingy(_foo): # Ok defined in parametrize + pass + +@pytest.mark.parametrize( + "_test_input,_expected", + [("3+5", 8), ("2+4", 6), pytest.param("6*9", 42, marks=pytest.mark.xfail)], +) +def test_eval(_test_input, _expected): # OK defined in parametrize + pass + + +@pytest.mark.parametrize("_foo", [1, 2, 3]) +def test_thingy2(_foo, _bar): # Error _bar is not defined in parametrize + pass + +@pytest.mark.parametrize(["_foo", "_bar"], [1, 2, 3]) +def test_thingy3(_foo, _bar): # OK defined in parametrize + pass + +@pytest.mark.parametrize(("_foo"), [1, 2, 3]) +def test_thingy4(_foo, _bar): # Error _bar is not defined in parametrize + pass + +@pytest.mark.parametrize([" _foo", " _bar "], [1, 2, 3]) +def test_thingy5(_foo, _bar): # OK defined in parametrize + pass + +x = "other" +@pytest.mark.parametrize(x, [1, 2, 3]) +def test_thingy5(_foo, _bar): # known false negative, we don't try to resolve variables + pass diff --git a/crates/ruff_linter/src/rules/flake8_pytest_style/rules/fixture.rs b/crates/ruff_linter/src/rules/flake8_pytest_style/rules/fixture.rs index ff34a8a6c0..7e0d08faed 100644 --- a/crates/ruff_linter/src/rules/flake8_pytest_style/rules/fixture.rs +++ b/crates/ruff_linter/src/rules/flake8_pytest_style/rules/fixture.rs @@ -1,6 +1,7 @@ use ruff_diagnostics::{AlwaysFixableViolation, Violation}; use ruff_diagnostics::{Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, ViolationMetadata}; +use ruff_python_ast::helpers::map_callable; use ruff_python_ast::name::UnqualifiedName; use ruff_python_ast::visitor; use ruff_python_ast::visitor::Visitor; @@ -11,6 +12,7 @@ use ruff_python_semantic::SemanticModel; use ruff_source_file::LineRanges; use ruff_text_size::Ranged; use ruff_text_size::{TextLen, TextRange}; +use rustc_hash::FxHashSet; use crate::checkers::ast::Checker; use crate::fix::edits; @@ -807,10 +809,51 @@ fn check_fixture_returns(checker: &Checker, name: &str, body: &[Stmt], returns: } /// PT019 -fn check_test_function_args(checker: &Checker, parameters: &Parameters) { +fn check_test_function_args(checker: &Checker, parameters: &Parameters, decorators: &[Decorator]) { + let mut named_parametrize = FxHashSet::default(); + for decorator in decorators.iter().filter(|decorator| { + UnqualifiedName::from_expr(map_callable(&decorator.expression)) + .is_some_and(|name| matches!(name.segments(), ["pytest", "mark", "parametrize"])) + }) { + let Some(call_expr) = decorator.expression.as_call_expr() else { + continue; + }; + let Some(first_arg) = call_expr.arguments.find_argument_value("argnames", 0) else { + continue; + }; + + match first_arg { + Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => { + named_parametrize.extend( + value + .to_str() + .split(',') + .map(str::trim) + .filter(|param| !param.is_empty() && param.starts_with('_')), + ); + } + + Expr::Name(_) => return, + Expr::List(ast::ExprList { elts, .. }) | Expr::Tuple(ast::ExprTuple { elts, .. }) + if elts.iter().any(Expr::is_name_expr) => + { + return + } + Expr::List(ast::ExprList { elts, .. }) | Expr::Tuple(ast::ExprTuple { elts, .. }) => { + named_parametrize.extend( + elts.iter() + .filter_map(Expr::as_string_literal_expr) + .map(|param| param.value.to_str().trim()) + .filter(|param| !param.is_empty() && param.starts_with('_')), + ); + } + _ => {} + } + } + for parameter in parameters.iter_non_variadic_params() { let name = parameter.name(); - if name.starts_with('_') { + if name.starts_with('_') && !named_parametrize.contains(name.as_str()) { checker.report_diagnostic(Diagnostic::new( PytestFixtureParamWithoutValue { name: name.to_string(), @@ -915,6 +958,6 @@ pub(crate) fn fixture( } if checker.enabled(Rule::PytestFixtureParamWithoutValue) && name.starts_with("test_") { - check_test_function_args(checker, parameters); + check_test_function_args(checker, parameters, decorators); } } diff --git a/crates/ruff_linter/src/rules/flake8_pytest_style/snapshots/ruff_linter__rules__flake8_pytest_style__tests__PT019.snap b/crates/ruff_linter/src/rules/flake8_pytest_style/snapshots/ruff_linter__rules__flake8_pytest_style__tests__PT019.snap index 309e4a19ec..2c2abc05e4 100644 --- a/crates/ruff_linter/src/rules/flake8_pytest_style/snapshots/ruff_linter__rules__flake8_pytest_style__tests__PT019.snap +++ b/crates/ruff_linter/src/rules/flake8_pytest_style/snapshots/ruff_linter__rules__flake8_pytest_style__tests__PT019.snap @@ -1,6 +1,5 @@ --- source: crates/ruff_linter/src/rules/flake8_pytest_style/mod.rs -snapshot_kind: text --- PT019.py:9:14: PT019 Fixture `_fixture` without value is injected as parameter, use `@pytest.mark.usefixtures` instead | @@ -15,3 +14,19 @@ PT019.py:13:17: PT019 Fixture `_fixture` without value is injected as parameter, | ^^^^^^^^ PT019 14 | pass | + +PT019.py:31:24: PT019 Fixture `_bar` without value is injected as parameter, use `@pytest.mark.usefixtures` instead + | +30 | @pytest.mark.parametrize("_foo", [1, 2, 3]) +31 | def test_thingy2(_foo, _bar): # Error _bar is not defined in parametrize + | ^^^^ PT019 +32 | pass + | + +PT019.py:39:24: PT019 Fixture `_bar` without value is injected as parameter, use `@pytest.mark.usefixtures` instead + | +38 | @pytest.mark.parametrize(("_foo"), [1, 2, 3]) +39 | def test_thingy4(_foo, _bar): # Error _bar is not defined in parametrize + | ^^^^ PT019 +40 | pass + |