Add PT028 (checks for default arguments in test functions) (#324)

Fixes #316

README.md

@@ -43,6 +43,7 @@ Currently the following errors are reported:
 | [PT025] | pytest.mark.usefixtures has no effect on fixtures |
 | [PT026] | useless pytest.mark.usefixtures without parameters | 
 | [PT027] | use pytest.raises() instead of unittest-style '{assertion}' |
+| [PT028] | test function {name} has default value for argument {arg}, remove it |
 
 ## Installation
 
@@ -256,3 +257,4 @@ MIT
 [PT025]: /docs/rules/PT025.md
 [PT026]: /docs/rules/PT026.md
 [PT027]: /docs/rules/PT027.md
+[PT028]: /docs/rules/PT028.md

docs/rules/PT028.md

@@ -0,0 +1,25 @@
+# PT028
+
+`test function {name} has default value for argument {arg}, remove it`
+
+## Examples
+
+Bad code:
+
+```python
+def test_foo(bar=42):
+    pass
+```
+
+Good code:
+
+```python
+def test_foo(bar):
+    pass
+```
+
+## Rationale
+
+* even if the corresponding fixture is defined, current behavior of pytest is 
+   to use the default value instead of injecting the fixture
+* see original pytest issue: https://github.com/pytest-dev/pytest#12693

flake8_pytest_style/errors.py

@@ -148,3 +148,10 @@ class UseFixturesWithoutParameters(Error):
 class UnittestRaisesAssertion(Error):
     code = 'PT027'
     message = "use pytest.raises() instead of unittest-style '{assertion}'"
+
+
+# This should be named `TestFunctionArgumentWithDefault`,
+# but this way it is easier to avoid confusion with the `Test` prefix in pytest.
+class TFunctionArgumentWithDefault(Error):
+    code = 'PT028'
+    message = 'test function {name} has default value for argument {arg}, remove it'

flake8_pytest_style/plugin.py

@@ -21,6 +21,7 @@
     ParametrizeVisitor,
     PatchVisitor,
     RaisesVisitor,
+    TFunctionsVisitor,
     UnittestAssertionVisitor,
 )
 
@@ -39,6 +40,7 @@ class PytestStylePlugin(Plugin[Config]):
         PatchVisitor,
         ParametrizeVisitor,
         RaisesVisitor,
+        TFunctionsVisitor,
         UnittestAssertionVisitor,
     ]
 

flake8_pytest_style/visitors/__init__.py

@@ -6,6 +6,7 @@
 from .parametrize import ParametrizeVisitor
 from .patch import PatchVisitor
 from .raises import RaisesVisitor
+from .t_functions import TFunctionsVisitor
 
 __all__ = (
     'AssertionVisitor',
@@ -16,5 +17,6 @@
     'ParametrizeVisitor',
     'PatchVisitor',
     'RaisesVisitor',
+    'TFunctionsVisitor',
     'UnittestAssertionVisitor',
 )

flake8_pytest_style/visitors/fixtures.py

@@ -9,7 +9,6 @@
     ErroneousUseFixturesOnFixture,
     ExtraneousScopeFunction,
     FixtureFinalizerCallback,
-    FixtureParamWithoutValue,
     FixturePositionalArgs,
     IncorrectFixtureNameUnderscore,
     IncorrectFixtureParenthesesStyle,
@@ -26,7 +25,6 @@
     get_qualname,
     is_abstract_method,
     is_pytest_yield_fixture,
-    is_test_function,
     walk_without_nested_functions,
 )
 
@@ -152,14 +150,6 @@ def _check_fixture_marks(self, node: AnyFunctionDef) -> None:
                 self.error_from_node(ErroneousUseFixturesOnFixture, mark)
                 reported_errors.add(ErroneousUseFixturesOnFixture)
 
-    def _check_test_function_args(self, node: AnyFunctionDef) -> None:
-        """Checks for PT019."""
-        # intentionally not looking at posonlyargs because pytest passes everything
-        # as kwargs, so declaring fixture args as positional-only will fail anyway
-        for arg in node.args.args + node.args.kwonlyargs:
-            if arg.arg.startswith('_'):
-                self.error_from_node(FixtureParamWithoutValue, node, name=arg.arg)
-
     def visit_FunctionDef(self, node: AnyFunctionDef) -> None:
         fixture_decorator = get_fixture_decorator(node)
         if fixture_decorator:
@@ -169,9 +159,6 @@ def visit_FunctionDef(self, node: AnyFunctionDef) -> None:
             self._check_fixture_addfinalizer(node)
             self._check_fixture_marks(node)
 
-        if is_test_function(node):
-            self._check_test_function_args(node)
-
         self.generic_visit(node)
 
     visit_AsyncFunctionDef = visit_FunctionDef  # noqa: N815

flake8_pytest_style/visitors/t_functions.py

@@ -0,0 +1,44 @@
+from flake8_plugin_utils import Visitor
+
+from flake8_pytest_style.config import Config
+from flake8_pytest_style.errors import (
+    FixtureParamWithoutValue,
+    TFunctionArgumentWithDefault,
+)
+from flake8_pytest_style.utils import AnyFunctionDef, is_test_function
+
+
+# This should be named `TestFunctionsVisitor` (and the module `test_functions`),
+# but this way it is easier to avoid confusion with the `Test` prefix in pytest.
+class TFunctionsVisitor(Visitor[Config]):
+    def _check_test_function_args(self, node: AnyFunctionDef) -> None:
+        """Checks for PT019, P028."""
+        # intentionally not looking at posonlyargs because pytest passes everything
+        # as kwargs, so declaring fixture args as positional-only will fail anyway
+        for arg in node.args.args + node.args.kwonlyargs:
+            if arg.arg.startswith('_'):
+                # The error is raised at the position of `node` (function call),
+                # not `arg`, to preserve backwards compatibility.
+                self.error_from_node(FixtureParamWithoutValue, node, name=arg.arg)
+
+        if node.args.defaults:
+            pos_args = node.args.posonlyargs + node.args.args
+            pos_args_with_defaults = pos_args[-len(node.args.defaults) :]  # noqa: E203
+            for arg in pos_args_with_defaults:
+                self.error_from_node(
+                    TFunctionArgumentWithDefault, arg, name=node.name, arg=arg.arg
+                )
+
+        for arg, default in zip(node.args.kwonlyargs, node.args.kw_defaults):
+            if default is not None:
+                self.error_from_node(
+                    TFunctionArgumentWithDefault, arg, name=node.name, arg=arg.arg
+                )
+
+    def visit_FunctionDef(self, node: AnyFunctionDef) -> None:
+        if is_test_function(node):
+            self._check_test_function_args(node)
+
+        self.generic_visit(node)
+
+    visit_AsyncFunctionDef = visit_FunctionDef  # noqa: N815

tests/test_PT019_fixture_param_without_value.py

@@ -1,36 +1,36 @@
 from flake8_plugin_utils.utils import assert_error, assert_not_error
 
 from flake8_pytest_style.errors import FixtureParamWithoutValue
-from flake8_pytest_style.visitors import FixturesVisitor
+from flake8_pytest_style.visitors import TFunctionsVisitor
 
 
 def test_ok_good_param_name():
     code = """
         def test_xxx(fixture):
             pass
     """
-    assert_not_error(FixturesVisitor, code)
+    assert_not_error(TFunctionsVisitor, code)
 
 
 def test_ok_non_test_function():
     code = """
         def xxx(_param):
             pass
     """
-    assert_not_error(FixturesVisitor, code)
+    assert_not_error(TFunctionsVisitor, code)
 
 
 def test_error_arg():
     code = """
         def test_xxx(_fixture):
             pass
     """
-    assert_error(FixturesVisitor, code, FixtureParamWithoutValue, name='_fixture')
+    assert_error(TFunctionsVisitor, code, FixtureParamWithoutValue, name='_fixture')
 
 
 def test_error_kwonly():
     code = """
         def test_xxx(*, _fixture):
             pass
     """
-    assert_error(FixturesVisitor, code, FixtureParamWithoutValue, name='_fixture')
+    assert_error(TFunctionsVisitor, code, FixtureParamWithoutValue, name='_fixture')

tests/test_PT028_test_function_argument_with_default.py

@@ -0,0 +1,101 @@
+import ast
+import textwrap
+
+from flake8_plugin_utils.utils import assert_error, assert_not_error
+
+from flake8_pytest_style.errors import TFunctionArgumentWithDefault
+from flake8_pytest_style.visitors import TFunctionsVisitor
+
+
+def test_ok():
+    code = """
+        def test_xxx(fixture1, /, fixture2, *, fixture3):
+            pass
+    """
+    assert_not_error(TFunctionsVisitor, code)
+
+
+def test_ok_non_test_function():
+    code = """
+        def xxx(posonly1, posonly2=2, /, arg=3, *, kwonly=4):
+            pass
+    """
+    assert_not_error(TFunctionsVisitor, code)
+
+
+def test_error_posonly():
+    code = """
+        def test_xxx(posonly1, posonly2=2, /):
+            pass
+    """
+    assert_error(
+        TFunctionsVisitor,
+        code,
+        TFunctionArgumentWithDefault,
+        name='test_xxx',
+        arg='posonly2',
+    )
+
+
+def test_error_arg():
+    code = """
+        def test_xxx(arg1, arg2=2):
+            pass
+    """
+    assert_error(
+        TFunctionsVisitor,
+        code,
+        TFunctionArgumentWithDefault,
+        name='test_xxx',
+        arg='arg2',
+    )
+
+
+def test_error_kwonly():
+    code = """
+        def test_xxx(*, kwonly1, kwonly2=2):
+            pass
+    """
+    assert_error(
+        TFunctionsVisitor,
+        code,
+        TFunctionArgumentWithDefault,
+        name='test_xxx',
+        arg='kwonly2',
+    )
+
+
+def test_error_multiple():
+    code = """
+        def test_xxx(
+            posonly=1,
+            /,
+            arg=2,
+            *,
+            kwonly=3,
+        ):
+            pass
+    """
+    # flake8-plugin-utils does not allow multiple errors in a single test
+    visitor = TFunctionsVisitor()
+    visitor.visit(ast.parse(textwrap.dedent(code)))
+    assert len(visitor.errors) == 3
+    posonly_error, arg_error, kwonly_error = visitor.errors
+
+    assert isinstance(posonly_error, TFunctionArgumentWithDefault)
+    assert posonly_error.message == TFunctionArgumentWithDefault.formatted_message(
+        name='test_xxx', arg='posonly'
+    )
+    assert posonly_error.lineno == 3
+
+    assert isinstance(arg_error, TFunctionArgumentWithDefault)
+    assert arg_error.message == TFunctionArgumentWithDefault.formatted_message(
+        name='test_xxx', arg='arg'
+    )
+    assert arg_error.lineno == 5
+
+    assert isinstance(kwonly_error, TFunctionArgumentWithDefault)
+    assert kwonly_error.message == TFunctionArgumentWithDefault.formatted_message(
+        name='test_xxx', arg='kwonly'
+    )
+    assert kwonly_error.lineno == 7