From c894530e7d9fff1a890312b204b1c3eecd13fae2 Mon Sep 17 00:00:00 2001 From: Mikhail Burshteyn Date: Fri, 10 Jan 2025 18:23:32 +0400 Subject: [PATCH] Add PT028 (checks for default arguments in test functions) (#324) Fixes #316 --- README.md | 2 + docs/rules/PT028.md | 25 +++++ flake8_pytest_style/errors.py | 7 ++ flake8_pytest_style/plugin.py | 2 + flake8_pytest_style/visitors/__init__.py | 2 + flake8_pytest_style/visitors/fixtures.py | 13 --- flake8_pytest_style/visitors/t_functions.py | 44 ++++++++ .../test_PT019_fixture_param_without_value.py | 10 +- ...028_test_function_argument_with_default.py | 101 ++++++++++++++++++ 9 files changed, 188 insertions(+), 18 deletions(-) create mode 100644 docs/rules/PT028.md create mode 100644 flake8_pytest_style/visitors/t_functions.py create mode 100644 tests/test_PT028_test_function_argument_with_default.py diff --git a/README.md b/README.md index c758a79..52857d3 100644 --- a/README.md +++ b/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 diff --git a/docs/rules/PT028.md b/docs/rules/PT028.md new file mode 100644 index 0000000..2c00a4a --- /dev/null +++ b/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 diff --git a/flake8_pytest_style/errors.py b/flake8_pytest_style/errors.py index 854ad7d..7427586 100644 --- a/flake8_pytest_style/errors.py +++ b/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' diff --git a/flake8_pytest_style/plugin.py b/flake8_pytest_style/plugin.py index 4b9db50..9c37761 100644 --- a/flake8_pytest_style/plugin.py +++ b/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, ] diff --git a/flake8_pytest_style/visitors/__init__.py b/flake8_pytest_style/visitors/__init__.py index 7a2c551..350099f 100644 --- a/flake8_pytest_style/visitors/__init__.py +++ b/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', ) diff --git a/flake8_pytest_style/visitors/fixtures.py b/flake8_pytest_style/visitors/fixtures.py index abc4a4f..3d66eac 100644 --- a/flake8_pytest_style/visitors/fixtures.py +++ b/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 diff --git a/flake8_pytest_style/visitors/t_functions.py b/flake8_pytest_style/visitors/t_functions.py new file mode 100644 index 0000000..f332d50 --- /dev/null +++ b/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 diff --git a/tests/test_PT019_fixture_param_without_value.py b/tests/test_PT019_fixture_param_without_value.py index 5b19b8d..5613b7c 100644 --- a/tests/test_PT019_fixture_param_without_value.py +++ b/tests/test_PT019_fixture_param_without_value.py @@ -1,7 +1,7 @@ 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(): @@ -9,7 +9,7 @@ def test_ok_good_param_name(): def test_xxx(fixture): pass """ - assert_not_error(FixturesVisitor, code) + assert_not_error(TFunctionsVisitor, code) def test_ok_non_test_function(): @@ -17,7 +17,7 @@ def test_ok_non_test_function(): def xxx(_param): pass """ - assert_not_error(FixturesVisitor, code) + assert_not_error(TFunctionsVisitor, code) def test_error_arg(): @@ -25,7 +25,7 @@ def test_error_arg(): def test_xxx(_fixture): pass """ - assert_error(FixturesVisitor, code, FixtureParamWithoutValue, name='_fixture') + assert_error(TFunctionsVisitor, code, FixtureParamWithoutValue, name='_fixture') def test_error_kwonly(): @@ -33,4 +33,4 @@ def test_error_kwonly(): def test_xxx(*, _fixture): pass """ - assert_error(FixturesVisitor, code, FixtureParamWithoutValue, name='_fixture') + assert_error(TFunctionsVisitor, code, FixtureParamWithoutValue, name='_fixture') diff --git a/tests/test_PT028_test_function_argument_with_default.py b/tests/test_PT028_test_function_argument_with_default.py new file mode 100644 index 0000000..788843a --- /dev/null +++ b/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