[flake8-pytest-style] Test function parameters with default arguments (PT028) (#15449)

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): ...

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): ...

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) {

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),

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"),

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_ast::{self as ast, Decorator, Expr, ExprCall, Keyword, Stmt, StmtFunctionDef};
-use ruff_python_semantic::SemanticModel;
+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

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;

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<String> {
+        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));
+    }
+}

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

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

ruff.schema.json

@@ -3715,6 +3715,7 @@
         "PT025",
         "PT026",
         "PT027",
+        "PT028",
         "PT029",
         "PT03",
         "PT030",