From 469e3aff4c59010094c4b309b32016010cd73cb3 Mon Sep 17 00:00:00 2001 From: Mike Koltsov Date: Mon, 23 Nov 2020 12:09:19 +0300 Subject: [PATCH 1/2] add fixture usage validator --- README.md | 18 +++++++ flake8_fine_pytest/checker.py | 2 + flake8_fine_pytest/watchers/assert_count.py | 5 +- flake8_fine_pytest/watchers/base.py | 3 ++ .../watchers/signature_complexity.py | 5 +- flake8_fine_pytest/watchers/usefixtures.py | 48 +++++++++++++++++++ .../watchers/xfail_until_argument_watcher.py | 4 +- .../test_complex_signature_tests.py | 1 - tests/test_files/test_with_fixtures.py | 10 ++++ .../test_signature_complexity.py | 8 +++- tests/test_integration/test_usefixtures.py | 10 ++++ 11 files changed, 101 insertions(+), 13 deletions(-) create mode 100644 flake8_fine_pytest/watchers/usefixtures.py create mode 100644 tests/test_files/test_with_fixtures.py create mode 100644 tests/test_integration/test_usefixtures.py diff --git a/README.md b/README.md index 88016e7..b6bf40e 100644 --- a/README.md +++ b/README.md @@ -89,6 +89,24 @@ tests/test_unit/test_utils.py:128:1: FP008 stale xfail mark in case you have too old `xfail` mark +6) validates that test function uses `pytest.mark.usefixtures` +for those fixtures, which are not directly referenced in test body + +For example, checking this function + +```python3 +# file: test_something.py +def test_something(fixture_one, fixture_two): + assert fixture_two.some_attribute is not None +``` + +would raise: + +```shell +tests/test_unit/test_something.py:2:0: FP009 test_something should use fixtures +as follows: @pytest.mark.usefixtures('fixture_one') +``` + ## Installation ```terminal diff --git a/flake8_fine_pytest/checker.py b/flake8_fine_pytest/checker.py index 78e8774..c81c8f4 100644 --- a/flake8_fine_pytest/checker.py +++ b/flake8_fine_pytest/checker.py @@ -10,6 +10,7 @@ ) from flake8_fine_pytest.watchers.signature_complexity import SignatureComplexityWatcher from flake8_fine_pytest.watchers.assert_count import AssertCountWatcher +from flake8_fine_pytest.watchers.usefixtures import UsefixturesWatcher from flake8_fine_pytest.common_types import CheckResult @@ -24,6 +25,7 @@ class FinePytestChecker: SignatureComplexityWatcher, AssertCountWatcher, XfailUntilArgumentWatcher, + UsefixturesWatcher, ) def __init__(self, tree: ast.AST, filename: str): diff --git a/flake8_fine_pytest/watchers/assert_count.py b/flake8_fine_pytest/watchers/assert_count.py index a7a139a..290b2ee 100644 --- a/flake8_fine_pytest/watchers/assert_count.py +++ b/flake8_fine_pytest/watchers/assert_count.py @@ -20,15 +20,12 @@ def _should_check(self) -> bool: def _validate_assert_count(self, tree: ast.AST) -> None: for node in ast.walk(tree): - if self._is_properly_node(node) is False: + if self._is_test(node) is False: continue if self._is_valid(node) is False: # type: ignore self._note_an_error(node) - def _is_properly_node(self, node: ast.AST) -> bool: - return isinstance(node, ast.FunctionDef) and node.name.startswith('test_') - def _is_valid(self, node: ast.FunctionDef) -> bool: asserts_count = self._get_actual_asserts_count(node) diff --git a/flake8_fine_pytest/watchers/base.py b/flake8_fine_pytest/watchers/base.py index d9353ca..6f11f64 100644 --- a/flake8_fine_pytest/watchers/base.py +++ b/flake8_fine_pytest/watchers/base.py @@ -24,3 +24,6 @@ def run(self) -> None: def _is_test_file(self, filename: str) -> bool: stem = get_stem(self.filename) return stem.startswith('test_') + + def _is_test(self, node: ast.AST) -> bool: + return isinstance(node, ast.FunctionDef) and node.name.startswith('test_') diff --git a/flake8_fine_pytest/watchers/signature_complexity.py b/flake8_fine_pytest/watchers/signature_complexity.py index b4e7d19..52cfbc5 100644 --- a/flake8_fine_pytest/watchers/signature_complexity.py +++ b/flake8_fine_pytest/watchers/signature_complexity.py @@ -20,15 +20,12 @@ def _should_check(self) -> bool: def _validate_signature_arguments_count(self, tree: ast.AST) -> None: for node in ast.walk(tree): - if self._is_properly_node(node) is False: + if self._is_test(node) is False: continue if self._is_invalid_signature(node) is True: # type: ignore self._note_an_error(node) - def _is_properly_node(self, node: ast.AST) -> bool: - return isinstance(node, ast.FunctionDef) and node.name.startswith('test_') - def _is_invalid_signature(self, ast_node: ast.FunctionDef) -> bool: signature_arguments = ast_node.args.args diff --git a/flake8_fine_pytest/watchers/usefixtures.py b/flake8_fine_pytest/watchers/usefixtures.py new file mode 100644 index 0000000..ac9f25f --- /dev/null +++ b/flake8_fine_pytest/watchers/usefixtures.py @@ -0,0 +1,48 @@ +import ast +import typing + +from flake8_fine_pytest.watchers.base import BaseWatcher + + +class UsefixturesWatcher(BaseWatcher): + error_template = ( + 'FP009 {test_name} should use fixtures as follows: ' + '@pytest.mark.usefixtures({fixtures_list_as_str})' + ) + + def run(self) -> None: + if self._is_test_file(self.filename): + self._validate_usefixtures_used_where_possible(self.tree) + + def _validate_usefixtures_used_where_possible(self, tree: ast.AST) -> None: + for node in ast.walk(tree): + if not self._is_test(node): + continue + + fixture_names = self._get_unreferenced_fixture_names(node) # type: ignore + if fixture_names: + self._add_usefixtures_error(node, fixture_names) # type: ignore + + def _get_unreferenced_fixture_names( + self, + function_node: ast.FunctionDef, + ) -> typing.List[str]: + referenced_variable_names = { + node.id + for node in ast.walk(function_node) + if isinstance(node, ast.Name) + } + test_fixture_names = {arg.arg for arg in function_node.args.args} + + return sorted(test_fixture_names - referenced_variable_names) + + def _add_usefixtures_error( + self, + function_node: ast.FunctionDef, + fixture_names: typing.List[str], + ) -> None: + error_message = self.error_template.format( + test_name=function_node.name, + fixtures_list_as_str=', '.join(repr(name) for name in fixture_names), + ) + self.add_error((function_node.lineno, 0, error_message)) diff --git a/flake8_fine_pytest/watchers/xfail_until_argument_watcher.py b/flake8_fine_pytest/watchers/xfail_until_argument_watcher.py index dc3103e..5a0766b 100644 --- a/flake8_fine_pytest/watchers/xfail_until_argument_watcher.py +++ b/flake8_fine_pytest/watchers/xfail_until_argument_watcher.py @@ -69,7 +69,7 @@ def run(self) -> None: def _should_check(self) -> bool: return self._is_test_file(self.filename) - def _is_properly_node(self, decorator: ast.Call) -> bool: + def _is_xfail_call(self, decorator: ast.Call) -> bool: return ( hasattr(decorator, 'func') and hasattr(decorator.func, 'attr') @@ -85,7 +85,7 @@ def _find_decorators_node_to_validate(self) -> None: def _validate_decorators(self, decorators: List[ast.Call]) -> None: for decorator in decorators: - if self._is_properly_node(decorator) is False: + if self._is_xfail_call(decorator) is False: continue self._validate_xfail_decorator_args(decorator) diff --git a/tests/test_files/test_complex_signature_tests.py b/tests/test_files/test_complex_signature_tests.py index e62cfd8..c8c2808 100644 --- a/tests/test_files/test_complex_signature_tests.py +++ b/tests/test_files/test_complex_signature_tests.py @@ -16,7 +16,6 @@ def three(): pass - def test_with_too_complex_signature(one, two, three): assert (2 + 2) == 4 diff --git a/tests/test_files/test_with_fixtures.py b/tests/test_files/test_with_fixtures.py new file mode 100644 index 0000000..fcd72ca --- /dev/null +++ b/tests/test_files/test_with_fixtures.py @@ -0,0 +1,10 @@ +import pytest + + +def test_with_no_usefixtures_where_needed(caplog, capsys, tmp_path): + assert capsys + + +@pytest.mark.usefixtures('caplog', 'tmp_path') +def test_with_usefixtures_where_needed(capsys): + assert capsys diff --git a/tests/test_integration/test_signature_complexity.py b/tests/test_integration/test_signature_complexity.py index 5857c57..bb1b799 100644 --- a/tests/test_integration/test_signature_complexity.py +++ b/tests/test_integration/test_signature_complexity.py @@ -1,4 +1,8 @@ def test_signature_complexity(run_validator_for_test_files): - errors = run_validator_for_test_files('test_complex_signature_tests.py', allowed_test_arguments_count=2) + errors = run_validator_for_test_files( + 'test_complex_signature_tests.py', + allowed_test_arguments_count=2, + ) + FP004s = [error for error in errors if 'FP004' in error[2]] - assert len(errors) == 1 + assert len(FP004s) == 1 diff --git a/tests/test_integration/test_usefixtures.py b/tests/test_integration/test_usefixtures.py new file mode 100644 index 0000000..f87e5db --- /dev/null +++ b/tests/test_integration/test_usefixtures.py @@ -0,0 +1,10 @@ +def test_fixtures_should_be_in_usefixtures(run_validator_for_test_files): + expected_error_message = ( + 'FP009 test_with_no_usefixtures_where_needed should use ' + "fixtures as follows: @pytest.mark.usefixtures('caplog', 'tmp_path')" + ) + + errors = run_validator_for_test_files('test_with_fixtures.py') + + assert len(errors) == 1 + assert errors[0][2] == expected_error_message From 8bb69813c56829ecf2d4d0d5d4996dd3ebea307a Mon Sep 17 00:00:00 2001 From: Mike Koltsov Date: Tue, 24 Nov 2020 14:26:23 +0300 Subject: [PATCH 2/2] make base methods for selecting checkable ast nodes. --- flake8_fine_pytest/watchers/assert_count.py | 8 ++--- flake8_fine_pytest/watchers/base.py | 8 ++++- .../watchers/modules_structure.py | 30 +++++++++++-------- .../watchers/signature_complexity.py | 8 ++--- flake8_fine_pytest/watchers/usefixtures.py | 4 +-- .../watchers/xfail_until_argument_watcher.py | 13 +++----- 6 files changed, 38 insertions(+), 33 deletions(-) diff --git a/flake8_fine_pytest/watchers/assert_count.py b/flake8_fine_pytest/watchers/assert_count.py index 290b2ee..1eb7706 100644 --- a/flake8_fine_pytest/watchers/assert_count.py +++ b/flake8_fine_pytest/watchers/assert_count.py @@ -12,15 +12,15 @@ class AssertCountWatcher(BaseWatcher): def run(self) -> None: self.allowed_assert_count = self.options.allowed_assert_count - if self._should_check(): + if self._should_run(): self._validate_assert_count(self.tree) - def _should_check(self) -> bool: - return self._is_test_file(self.filename) and self.allowed_assert_count is not None + def _should_run(self) -> bool: + return super()._should_run() and self.allowed_assert_count is not None def _validate_assert_count(self, tree: ast.AST) -> None: for node in ast.walk(tree): - if self._is_test(node) is False: + if self._should_check_node(node) is False: continue if self._is_valid(node) is False: # type: ignore diff --git a/flake8_fine_pytest/watchers/base.py b/flake8_fine_pytest/watchers/base.py index 6f11f64..6dcfc8d 100644 --- a/flake8_fine_pytest/watchers/base.py +++ b/flake8_fine_pytest/watchers/base.py @@ -25,5 +25,11 @@ def _is_test_file(self, filename: str) -> bool: stem = get_stem(self.filename) return stem.startswith('test_') - def _is_test(self, node: ast.AST) -> bool: + def _is_test_function(self, node: ast.AST) -> bool: return isinstance(node, ast.FunctionDef) and node.name.startswith('test_') + + def _should_run(self) -> bool: + return self._is_test_file(self.filename) + + def _should_check_node(self, node: ast.AST) -> bool: + return self._is_test_function(node) diff --git a/flake8_fine_pytest/watchers/modules_structure.py b/flake8_fine_pytest/watchers/modules_structure.py index ac317ec..8146f48 100644 --- a/flake8_fine_pytest/watchers/modules_structure.py +++ b/flake8_fine_pytest/watchers/modules_structure.py @@ -1,5 +1,5 @@ import pathlib -from typing import List, Optional +from typing import List from flake8_fine_pytest.watchers.base import BaseWatcher from flake8_fine_pytest.utils import get_stem @@ -33,15 +33,19 @@ class ModulesStructureWatcher(BaseWatcher): def run(self) -> None: allowed_test_directories = self.options.allowed_test_directories - if self._should_check(allowed_test_directories) is False: - return None - - file_directory = get_file_directory(self.filename) - - if file_directory not in allowed_test_directories: - error_message = get_error_message(self.error_template, allowed_test_directories, self.filename) - - self.add_error((0, 0, error_message)) - - def _should_check(self, allowed_test_directories: Optional[List[str]]) -> bool: - return self._is_test_file(self.filename) and allowed_test_directories is not None + if self._should_run(): + file_directory = get_file_directory(self.filename) + + if file_directory not in allowed_test_directories: + error_message = get_error_message( + self.error_template, + allowed_test_directories, + self.filename, + ) + self.add_error((0, 0, error_message)) + + def _should_run(self) -> bool: + return ( + super()._should_run() + and self.options.allowed_test_directories is not None + ) diff --git a/flake8_fine_pytest/watchers/signature_complexity.py b/flake8_fine_pytest/watchers/signature_complexity.py index 52cfbc5..69a787e 100644 --- a/flake8_fine_pytest/watchers/signature_complexity.py +++ b/flake8_fine_pytest/watchers/signature_complexity.py @@ -12,15 +12,15 @@ class SignatureComplexityWatcher(BaseWatcher): def run(self) -> None: self.allowed_test_arguments_count = self.options.allowed_test_arguments_count - if self._should_check(): + if self._should_run(): self._validate_signature_arguments_count(self.tree) - def _should_check(self) -> bool: - return self._is_test_file(self.filename) and self.allowed_test_arguments_count is not None + def _should_run(self) -> bool: + return super()._should_run() and self.allowed_test_arguments_count is not None def _validate_signature_arguments_count(self, tree: ast.AST) -> None: for node in ast.walk(tree): - if self._is_test(node) is False: + if self._should_check_node(node) is False: continue if self._is_invalid_signature(node) is True: # type: ignore diff --git a/flake8_fine_pytest/watchers/usefixtures.py b/flake8_fine_pytest/watchers/usefixtures.py index ac9f25f..5b5b2c5 100644 --- a/flake8_fine_pytest/watchers/usefixtures.py +++ b/flake8_fine_pytest/watchers/usefixtures.py @@ -11,12 +11,12 @@ class UsefixturesWatcher(BaseWatcher): ) def run(self) -> None: - if self._is_test_file(self.filename): + if self._should_run(): self._validate_usefixtures_used_where_possible(self.tree) def _validate_usefixtures_used_where_possible(self, tree: ast.AST) -> None: for node in ast.walk(tree): - if not self._is_test(node): + if not self._should_check_node(node): continue fixture_names = self._get_unreferenced_fixture_names(node) # type: ignore diff --git a/flake8_fine_pytest/watchers/xfail_until_argument_watcher.py b/flake8_fine_pytest/watchers/xfail_until_argument_watcher.py index 5a0766b..da530ec 100644 --- a/flake8_fine_pytest/watchers/xfail_until_argument_watcher.py +++ b/flake8_fine_pytest/watchers/xfail_until_argument_watcher.py @@ -63,13 +63,10 @@ class XfailUntilArgumentWatcher(BaseWatcher): ] def run(self) -> None: - if self._should_check(): + if self._should_run(): self._find_decorators_node_to_validate() - def _should_check(self) -> bool: - return self._is_test_file(self.filename) - - def _is_xfail_call(self, decorator: ast.Call) -> bool: + def _should_check_node(self, decorator: ast.Call) -> bool: # type: ignore return ( hasattr(decorator, 'func') and hasattr(decorator.func, 'attr') @@ -85,10 +82,8 @@ def _find_decorators_node_to_validate(self) -> None: def _validate_decorators(self, decorators: List[ast.Call]) -> None: for decorator in decorators: - if self._is_xfail_call(decorator) is False: - continue - - self._validate_xfail_decorator_args(decorator) + if self._should_check_node(decorator): + self._validate_xfail_decorator_args(decorator) def _validate_xfail_decorator_args(self, decorator: ast.Call) -> None: xfail_args = [