diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pytest_style/PT028.py b/crates/ruff_linter/resources/test/fixtures/flake8_pytest_style/PT028.py new file mode 100644 index 0000000000..1eb5360d4c --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pytest_style/PT028.py @@ -0,0 +1,17 @@ +# Errors + +def test_foo(a=1): ... +def test_foo(a = 1): ... +def test_foo(a = (1)): ... +def test_foo(a: int=1): ... +def test_foo(a: int = 1): ... +def test_foo(a: (int) = 1): ... +def test_foo(a: int = (1)): ... +def test_foo(a: (int) = (1)): ... +def test_foo(a=1, /, b=2, *, c=3): ... + + +# No errors + +def test_foo(a): ... +def test_foo(a: int): ... diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pytest_style/is_pytest_test.py b/crates/ruff_linter/resources/test/fixtures/flake8_pytest_style/is_pytest_test.py new file mode 100644 index 0000000000..9383451c9f --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pytest_style/is_pytest_test.py @@ -0,0 +1,17 @@ +# Errors + +def test_this_is_a_test(a=1): ... +def testThisIsAlsoATest(a=1): ... + +class TestClass: + def test_this_too_is_a_test(self, a=1): ... + def testAndOfCourseThis(self, a=1): ... + + +# No errors + +def this_is_not_a_test(a=1): ... + +class TestClassLookalike: + def __init__(self): ... + def test_this_is_not_a_test(self, a=1): ... diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 72e7e3ddda..8bce8c1429 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -373,6 +373,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::PostInitDefault) { ruff::rules::post_init_default(checker, function_def); } + if checker.enabled(Rule::PytestParameterWithDefaultArgument) { + flake8_pytest_style::rules::parameter_with_default_argument(checker, function_def); + } } Stmt::Return(_) => { if checker.enabled(Rule::ReturnOutsideFunction) { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 4d19a0e04c..cc38f50b41 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -830,6 +830,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8PytestStyle, "025") => (RuleGroup::Stable, rules::flake8_pytest_style::rules::PytestErroneousUseFixturesOnFixture), (Flake8PytestStyle, "026") => (RuleGroup::Stable, rules::flake8_pytest_style::rules::PytestUseFixturesWithoutParameters), (Flake8PytestStyle, "027") => (RuleGroup::Stable, rules::flake8_pytest_style::rules::PytestUnittestRaisesAssertion), + (Flake8PytestStyle, "028") => (RuleGroup::Preview, rules::flake8_pytest_style::rules::PytestParameterWithDefaultArgument), (Flake8PytestStyle, "029") => (RuleGroup::Preview, rules::flake8_pytest_style::rules::PytestWarnsWithoutWarning), (Flake8PytestStyle, "030") => (RuleGroup::Preview, rules::flake8_pytest_style::rules::PytestWarnsTooBroad), (Flake8PytestStyle, "031") => (RuleGroup::Preview, rules::flake8_pytest_style::rules::PytestWarnsWithMultipleStatements), diff --git a/crates/ruff_linter/src/rules/flake8_pytest_style/mod.rs b/crates/ruff_linter/src/rules/flake8_pytest_style/mod.rs index de8390f619..7cb3486d01 100644 --- a/crates/ruff_linter/src/rules/flake8_pytest_style/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_pytest_style/mod.rs @@ -18,6 +18,12 @@ mod tests { use super::settings::Settings; use super::types; + #[test_case( + Rule::PytestParameterWithDefaultArgument, + Path::new("is_pytest_test.py"), + Settings::default(), + "is_pytest_test" + )] #[test_case( Rule::PytestFixtureIncorrectParenthesesStyle, Path::new("PT001.py"), @@ -275,6 +281,12 @@ mod tests { Settings::default(), "PT027_1" )] + #[test_case( + Rule::PytestParameterWithDefaultArgument, + Path::new("PT028.py"), + Settings::default(), + "PT028" + )] #[test_case( Rule::PytestWarnsWithoutWarning, Path::new("PT029.py"), diff --git a/crates/ruff_linter/src/rules/flake8_pytest_style/rules/helpers.rs b/crates/ruff_linter/src/rules/flake8_pytest_style/rules/helpers.rs index d09474023e..7b886dd5cc 100644 --- a/crates/ruff_linter/src/rules/flake8_pytest_style/rules/helpers.rs +++ b/crates/ruff_linter/src/rules/flake8_pytest_style/rules/helpers.rs @@ -1,10 +1,11 @@ -use std::fmt; - +use crate::checkers::ast::Checker; use ruff_python_ast::helpers::map_callable; use ruff_python_ast::name::UnqualifiedName; -use ruff_python_ast::{self as ast, Decorator, Expr, ExprCall, Keyword}; -use ruff_python_semantic::SemanticModel; +use ruff_python_ast::{self as ast, Decorator, Expr, ExprCall, Keyword, Stmt, StmtFunctionDef}; +use ruff_python_semantic::analyze::visibility; +use ruff_python_semantic::{ScopeKind, SemanticModel}; use ruff_python_trivia::PythonWhitespace; +use std::fmt; pub(super) fn get_mark_decorators( decorators: &[Decorator], @@ -46,6 +47,47 @@ pub(super) fn is_pytest_parametrize(call: &ExprCall, semantic: &SemanticModel) - }) } +/// Whether the currently checked `func` is likely to be a Pytest test. +/// +/// A normal Pytest test function is one whose name starts with `test` and is either: +/// +/// * Placed at module-level, or +/// * Placed within a class whose name starts with `Test` and does not have an `__init__` method. +/// +/// During test discovery, Pytest respects a few settings which we do not have access to. +/// This function is thus prone to both false positives and false negatives. +/// +/// References: +/// - [`pytest` documentation: Conventions for Python test discovery](https://docs.pytest.org/en/stable/explanation/goodpractices.html#conventions-for-python-test-discovery) +/// - [`pytest` documentation: Changing naming conventions](https://docs.pytest.org/en/stable/example/pythoncollection.html#changing-naming-conventions) +pub(crate) fn is_likely_pytest_test(func: &StmtFunctionDef, checker: &Checker) -> bool { + let semantic = checker.semantic(); + + if !func.name.starts_with("test") { + return false; + } + + if semantic.scope_id.is_global() { + return true; + } + + let ScopeKind::Class(class) = semantic.current_scope().kind else { + return false; + }; + + if !class.name.starts_with("Test") { + return false; + } + + class.body.iter().all(|stmt| { + let Stmt::FunctionDef(function) = stmt else { + return true; + }; + + !visibility::is_init(&function.name) + }) +} + pub(super) fn keyword_is_literal(keyword: &Keyword, literal: &str) -> bool { if let Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = &keyword.value { value == literal diff --git a/crates/ruff_linter/src/rules/flake8_pytest_style/rules/mod.rs b/crates/ruff_linter/src/rules/flake8_pytest_style/rules/mod.rs index 81b63314d1..128a1c5b4f 100644 --- a/crates/ruff_linter/src/rules/flake8_pytest_style/rules/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_pytest_style/rules/mod.rs @@ -6,6 +6,7 @@ pub(crate) use marks::*; pub(crate) use parametrize::*; pub(crate) use patch::*; pub(crate) use raises::*; +pub(crate) use test_functions::*; pub(crate) use warns::*; mod assertion; @@ -17,5 +18,6 @@ mod marks; mod parametrize; mod patch; mod raises; +mod test_functions; mod unittest_assert; mod warns; diff --git a/crates/ruff_linter/src/rules/flake8_pytest_style/rules/test_functions.rs b/crates/ruff_linter/src/rules/flake8_pytest_style/rules/test_functions.rs new file mode 100644 index 0000000000..6276db0c07 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_pytest_style/rules/test_functions.rs @@ -0,0 +1,82 @@ +use crate::checkers::ast::Checker; +use crate::rules::flake8_pytest_style::rules::helpers::is_likely_pytest_test; +use ruff_diagnostics::{Diagnostic, Edit, Fix, Violation}; +use ruff_macros::{derive_message_formats, ViolationMetadata}; +use ruff_python_ast::{ParameterWithDefault, StmtFunctionDef}; +use ruff_text_size::Ranged; + +/// ## What it does +/// Checks for parameters of test functions with default arguments. +/// +/// ## 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. +/// +/// ## Example +/// +/// ```python +/// def test_foo(a=1): ... +/// ``` +/// +/// Use instead: +/// +/// ```python +/// def test_foo(a): ... +/// ``` +/// +/// ## Fix safety +/// This rule's fix is marked as unsafe, as modifying a function signature can +/// change the behavior of the code. +/// +/// ## References +/// - [Original Pytest issue](https://github.com/pytest-dev/pytest/issues/12693) +#[derive(ViolationMetadata)] +pub(crate) struct PytestParameterWithDefaultArgument { + parameter_name: String, +} + +impl Violation for PytestParameterWithDefaultArgument { + #[derive_message_formats] + fn message(&self) -> String { + format!( + "Test function parameter `{}` has default argument", + self.parameter_name + ) + } + + fn fix_title(&self) -> Option { + Some("Remove default argument".to_string()) + } +} + +/// PT028 +pub(crate) fn parameter_with_default_argument( + checker: &mut Checker, + function_def: &StmtFunctionDef, +) { + if !is_likely_pytest_test(function_def, checker) { + return; + } + + let parameters = function_def.parameters.as_ref(); + + for ParameterWithDefault { + parameter, + default, + range: pwd_range, + } in parameters.iter_non_variadic_params() + { + let Some(default) = default else { + continue; + }; + + let parameter_name = parameter.name.to_string(); + let kind = PytestParameterWithDefaultArgument { parameter_name }; + let diagnostic = Diagnostic::new(kind, default.range()); + + let edit = Edit::deletion(parameter.end(), pwd_range.end()); + let fix = Fix::display_only_edit(edit); + + checker.diagnostics.push(diagnostic.with_fix(fix)); + } +} diff --git a/crates/ruff_linter/src/rules/flake8_pytest_style/snapshots/ruff_linter__rules__flake8_pytest_style__tests__PT028.snap b/crates/ruff_linter/src/rules/flake8_pytest_style/snapshots/ruff_linter__rules__flake8_pytest_style__tests__PT028.snap new file mode 100644 index 0000000000..118bcac45c --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_pytest_style/snapshots/ruff_linter__rules__flake8_pytest_style__tests__PT028.snap @@ -0,0 +1,224 @@ +--- +source: crates/ruff_linter/src/rules/flake8_pytest_style/mod.rs +--- +PT028.py:3:16: PT028 Test function parameter `a` has default argument + | +1 | # Errors +2 | +3 | def test_foo(a=1): ... + | ^ PT028 +4 | def test_foo(a = 1): ... +5 | def test_foo(a = (1)): ... + | + = help: Remove default argument + +ℹ Display-only fix +1 1 | # Errors +2 2 | +3 |-def test_foo(a=1): ... + 3 |+def test_foo(a): ... +4 4 | def test_foo(a = 1): ... +5 5 | def test_foo(a = (1)): ... +6 6 | def test_foo(a: int=1): ... + +PT028.py:4:18: PT028 Test function parameter `a` has default argument + | +3 | def test_foo(a=1): ... +4 | def test_foo(a = 1): ... + | ^ PT028 +5 | def test_foo(a = (1)): ... +6 | def test_foo(a: int=1): ... + | + = help: Remove default argument + +ℹ Display-only fix +1 1 | # Errors +2 2 | +3 3 | def test_foo(a=1): ... +4 |-def test_foo(a = 1): ... + 4 |+def test_foo(a): ... +5 5 | def test_foo(a = (1)): ... +6 6 | def test_foo(a: int=1): ... +7 7 | def test_foo(a: int = 1): ... + +PT028.py:5:19: PT028 Test function parameter `a` has default argument + | +3 | def test_foo(a=1): ... +4 | def test_foo(a = 1): ... +5 | def test_foo(a = (1)): ... + | ^ PT028 +6 | def test_foo(a: int=1): ... +7 | def test_foo(a: int = 1): ... + | + = help: Remove default argument + +ℹ Display-only fix +2 2 | +3 3 | def test_foo(a=1): ... +4 4 | def test_foo(a = 1): ... +5 |-def test_foo(a = (1)): ... + 5 |+def test_foo(a): ... +6 6 | def test_foo(a: int=1): ... +7 7 | def test_foo(a: int = 1): ... +8 8 | def test_foo(a: (int) = 1): ... + +PT028.py:6:21: PT028 Test function parameter `a` has default argument + | +4 | def test_foo(a = 1): ... +5 | def test_foo(a = (1)): ... +6 | def test_foo(a: int=1): ... + | ^ PT028 +7 | def test_foo(a: int = 1): ... +8 | def test_foo(a: (int) = 1): ... + | + = help: Remove default argument + +ℹ Display-only fix +3 3 | def test_foo(a=1): ... +4 4 | def test_foo(a = 1): ... +5 5 | def test_foo(a = (1)): ... +6 |-def test_foo(a: int=1): ... + 6 |+def test_foo(a: int): ... +7 7 | def test_foo(a: int = 1): ... +8 8 | def test_foo(a: (int) = 1): ... +9 9 | def test_foo(a: int = (1)): ... + +PT028.py:7:23: PT028 Test function parameter `a` has default argument + | +5 | def test_foo(a = (1)): ... +6 | def test_foo(a: int=1): ... +7 | def test_foo(a: int = 1): ... + | ^ PT028 +8 | def test_foo(a: (int) = 1): ... +9 | def test_foo(a: int = (1)): ... + | + = help: Remove default argument + +ℹ Display-only fix +4 4 | def test_foo(a = 1): ... +5 5 | def test_foo(a = (1)): ... +6 6 | def test_foo(a: int=1): ... +7 |-def test_foo(a: int = 1): ... + 7 |+def test_foo(a: int): ... +8 8 | def test_foo(a: (int) = 1): ... +9 9 | def test_foo(a: int = (1)): ... +10 10 | def test_foo(a: (int) = (1)): ... + +PT028.py:8:25: PT028 Test function parameter `a` has default argument + | + 6 | def test_foo(a: int=1): ... + 7 | def test_foo(a: int = 1): ... + 8 | def test_foo(a: (int) = 1): ... + | ^ PT028 + 9 | def test_foo(a: int = (1)): ... +10 | def test_foo(a: (int) = (1)): ... + | + = help: Remove default argument + +ℹ Display-only fix +5 5 | def test_foo(a = (1)): ... +6 6 | def test_foo(a: int=1): ... +7 7 | def test_foo(a: int = 1): ... +8 |-def test_foo(a: (int) = 1): ... + 8 |+def test_foo(a: (int)): ... +9 9 | def test_foo(a: int = (1)): ... +10 10 | def test_foo(a: (int) = (1)): ... +11 11 | def test_foo(a=1, /, b=2, *, c=3): ... + +PT028.py:9:24: PT028 Test function parameter `a` has default argument + | + 7 | def test_foo(a: int = 1): ... + 8 | def test_foo(a: (int) = 1): ... + 9 | def test_foo(a: int = (1)): ... + | ^ PT028 +10 | def test_foo(a: (int) = (1)): ... +11 | def test_foo(a=1, /, b=2, *, c=3): ... + | + = help: Remove default argument + +ℹ Display-only fix +6 6 | def test_foo(a: int=1): ... +7 7 | def test_foo(a: int = 1): ... +8 8 | def test_foo(a: (int) = 1): ... +9 |-def test_foo(a: int = (1)): ... + 9 |+def test_foo(a: int): ... +10 10 | def test_foo(a: (int) = (1)): ... +11 11 | def test_foo(a=1, /, b=2, *, c=3): ... +12 12 | + +PT028.py:10:26: PT028 Test function parameter `a` has default argument + | + 8 | def test_foo(a: (int) = 1): ... + 9 | def test_foo(a: int = (1)): ... +10 | def test_foo(a: (int) = (1)): ... + | ^ PT028 +11 | def test_foo(a=1, /, b=2, *, c=3): ... + | + = help: Remove default argument + +ℹ Display-only fix +7 7 | def test_foo(a: int = 1): ... +8 8 | def test_foo(a: (int) = 1): ... +9 9 | def test_foo(a: int = (1)): ... +10 |-def test_foo(a: (int) = (1)): ... + 10 |+def test_foo(a: (int)): ... +11 11 | def test_foo(a=1, /, b=2, *, c=3): ... +12 12 | +13 13 | + +PT028.py:11:16: PT028 Test function parameter `a` has default argument + | + 9 | def test_foo(a: int = (1)): ... +10 | def test_foo(a: (int) = (1)): ... +11 | def test_foo(a=1, /, b=2, *, c=3): ... + | ^ PT028 + | + = help: Remove default argument + +ℹ Display-only fix +8 8 | def test_foo(a: (int) = 1): ... +9 9 | def test_foo(a: int = (1)): ... +10 10 | def test_foo(a: (int) = (1)): ... +11 |-def test_foo(a=1, /, b=2, *, c=3): ... + 11 |+def test_foo(a, /, b=2, *, c=3): ... +12 12 | +13 13 | +14 14 | # No errors + +PT028.py:11:24: PT028 Test function parameter `b` has default argument + | + 9 | def test_foo(a: int = (1)): ... +10 | def test_foo(a: (int) = (1)): ... +11 | def test_foo(a=1, /, b=2, *, c=3): ... + | ^ PT028 + | + = help: Remove default argument + +ℹ Display-only fix +8 8 | def test_foo(a: (int) = 1): ... +9 9 | def test_foo(a: int = (1)): ... +10 10 | def test_foo(a: (int) = (1)): ... +11 |-def test_foo(a=1, /, b=2, *, c=3): ... + 11 |+def test_foo(a=1, /, b, *, c=3): ... +12 12 | +13 13 | +14 14 | # No errors + +PT028.py:11:32: PT028 Test function parameter `c` has default argument + | + 9 | def test_foo(a: int = (1)): ... +10 | def test_foo(a: (int) = (1)): ... +11 | def test_foo(a=1, /, b=2, *, c=3): ... + | ^ PT028 + | + = help: Remove default argument + +ℹ Display-only fix +8 8 | def test_foo(a: (int) = 1): ... +9 9 | def test_foo(a: int = (1)): ... +10 10 | def test_foo(a: (int) = (1)): ... +11 |-def test_foo(a=1, /, b=2, *, c=3): ... + 11 |+def test_foo(a=1, /, b=2, *, c): ... +12 12 | +13 13 | +14 14 | # No errors diff --git a/crates/ruff_linter/src/rules/flake8_pytest_style/snapshots/ruff_linter__rules__flake8_pytest_style__tests__is_pytest_test.snap b/crates/ruff_linter/src/rules/flake8_pytest_style/snapshots/ruff_linter__rules__flake8_pytest_style__tests__is_pytest_test.snap new file mode 100644 index 0000000000..df63491d7e --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_pytest_style/snapshots/ruff_linter__rules__flake8_pytest_style__tests__is_pytest_test.snap @@ -0,0 +1,79 @@ +--- +source: crates/ruff_linter/src/rules/flake8_pytest_style/mod.rs +--- +is_pytest_test.py:3:27: PT028 Test function parameter `a` has default argument + | +1 | # Errors +2 | +3 | def test_this_is_a_test(a=1): ... + | ^ PT028 +4 | def testThisIsAlsoATest(a=1): ... + | + = help: Remove default argument + +ℹ Display-only fix +1 1 | # Errors +2 2 | +3 |-def test_this_is_a_test(a=1): ... + 3 |+def test_this_is_a_test(a): ... +4 4 | def testThisIsAlsoATest(a=1): ... +5 5 | +6 6 | class TestClass: + +is_pytest_test.py:4:27: PT028 Test function parameter `a` has default argument + | +3 | def test_this_is_a_test(a=1): ... +4 | def testThisIsAlsoATest(a=1): ... + | ^ PT028 +5 | +6 | class TestClass: + | + = help: Remove default argument + +ℹ Display-only fix +1 1 | # Errors +2 2 | +3 3 | def test_this_is_a_test(a=1): ... +4 |-def testThisIsAlsoATest(a=1): ... + 4 |+def testThisIsAlsoATest(a): ... +5 5 | +6 6 | class TestClass: +7 7 | def test_this_too_is_a_test(self, a=1): ... + +is_pytest_test.py:7:41: PT028 Test function parameter `a` has default argument + | +6 | class TestClass: +7 | def test_this_too_is_a_test(self, a=1): ... + | ^ PT028 +8 | def testAndOfCourseThis(self, a=1): ... + | + = help: Remove default argument + +ℹ Display-only fix +4 4 | def testThisIsAlsoATest(a=1): ... +5 5 | +6 6 | class TestClass: +7 |- def test_this_too_is_a_test(self, a=1): ... + 7 |+ def test_this_too_is_a_test(self, a): ... +8 8 | def testAndOfCourseThis(self, a=1): ... +9 9 | +10 10 | + +is_pytest_test.py:8:37: PT028 Test function parameter `a` has default argument + | +6 | class TestClass: +7 | def test_this_too_is_a_test(self, a=1): ... +8 | def testAndOfCourseThis(self, a=1): ... + | ^ PT028 + | + = help: Remove default argument + +ℹ Display-only fix +5 5 | +6 6 | class TestClass: +7 7 | def test_this_too_is_a_test(self, a=1): ... +8 |- def testAndOfCourseThis(self, a=1): ... + 8 |+ def testAndOfCourseThis(self, a): ... +9 9 | +10 10 | +11 11 | # No errors diff --git a/ruff.schema.json b/ruff.schema.json index 321ac8f5e3..f302053296 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3715,6 +3715,7 @@ "PT025", "PT026", "PT027", + "PT028", "PT029", "PT03", "PT030",