From baab4836cf3fb3cb0768f1d5a148b17394742204 Mon Sep 17 00:00:00 2001 From: sverhoeven Date: Wed, 7 Oct 2026 16:07:43 +0200 Subject: [PATCH 1/2] Add no if in tests hook --- .pre-commit-config.yaml | 4 +- .pre-commit-hooks.yaml | 9 + README.md | 31 ++- pyproject.toml | 1 + sgconfig.yml | 1 + src/python_mock_hooks/__init__.py | 14 +- .../if-rules/no-if-in-tests.yml | 15 ++ src/python_mock_hooks/ifs-sgconfig.yml | 2 + .../__snapshots__/no-if-in-tests-snapshot.yml | 220 ++++++++++++++++++ tests/no-if-in-tests-test.yml | 96 ++++++++ tests/test_hook.py | 20 +- 11 files changed, 398 insertions(+), 15 deletions(-) create mode 100644 src/python_mock_hooks/if-rules/no-if-in-tests.yml create mode 100644 src/python_mock_hooks/ifs-sgconfig.yml create mode 100644 tests/__snapshots__/no-if-in-tests-snapshot.yml create mode 100644 tests/no-if-in-tests-test.yml diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 6a594a8..523f783 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -14,9 +14,9 @@ repos: - repo: local hooks: - id: ast-grep-rule-tests - name: Test monkeypatch rule + name: Test testing policy rules language: python entry: ast-grep test --config sgconfig.yml additional_dependencies: ["ast-grep-cli==0.45.2"] pass_filenames: false - files: ^(src/python_mock_hooks/rules/|tests/|sgconfig\.yml$) + files: ^(src/python_mock_hooks/(rules|if-rules)/|tests/|sgconfig\.yml$) diff --git a/.pre-commit-hooks.yaml b/.pre-commit-hooks.yaml index dc4f0db..4305a35 100644 --- a/.pre-commit-hooks.yaml +++ b/.pre-commit-hooks.yaml @@ -8,3 +8,12 @@ files: ^tests/.*\.py$ require_serial: true stages: [pre-commit, pre-merge-commit, pre-push, manual] +- id: tests-without-ifs + name: Check for if statements in tests + description: Reject if statements in test bodies; use separate tests or parametrization. + entry: tests-without-ifs + language: python + types: [python] + files: ^tests/.*\.py$ + require_serial: true + stages: [pre-commit, pre-merge-commit, pre-push, manual] diff --git a/README.md b/README.md index 7fa210e..ecaf466 100644 --- a/README.md +++ b/README.md @@ -3,7 +3,9 @@ [![DOI](https://zenodo.org/badge/DOI/10.5281/zenodo.23183666.svg)](https://doi.org/10.5281/zenodo.23183666) [![Research Software Directory Badge](https://img.shields.io/badge/rsd-00a3e3.svg)](https://research-software-directory.org/software/python-mock-precommit-hook) -A pre-commit / prek hook that restricts mocking in Python tests: +Two independently selectable pre-commit / prek hooks for Python tests. + +### `python-mock-hooks` - **Blocks**: - `unittest.mock` imports and references. @@ -19,10 +21,21 @@ A pre-commit / prek hook that restricts mocking in Python tests: For HTTP tests, prefer recording requests with [pytest-recording](https://github.com/kiwicom/pytest-recording). +### `tests-without-ifs` + +Blocks `if` / `elif` / `else` branching statements inside `test_*` functions +and methods, including async tests. Use separate tests or +`pytest.mark.parametrize` for each case instead. + +Permits `if` statements in fixtures, helpers (including nested helpers), and +at module or class scope, as well as conditional expressions +(`a if condition else b`) and comprehension filters. This hook does not +restrict mocking; `python-mock-hooks` does not restrict branching. + Check failures explain the policy to developers and LLM coding agents. Ruff [does not support custom lint plugins](https://docs.astral.sh/ruff/faq/#can-i-write-my-own-linter-plugins-for-ruff), -so this hook adds these testing policies as a separate check alongside Ruff. +so these hooks add testing policies as separate checks alongside Ruff. ## Usage @@ -35,9 +48,10 @@ For `.pre-commit-config.yaml` (pre-commit or prek): ```yaml repos: - repo: https://github.com/i-VRESSE/python-mock-hooks - rev: v0.2.0 + rev: v0.3.0 hooks: - id: python-mock-hooks + - id: tests-without-ifs ``` For `prek.toml` (prek): @@ -45,14 +59,15 @@ For `prek.toml` (prek): ```toml [[repos]] repo = "https://github.com/i-VRESSE/python-mock-hooks" -rev = "v0.2.0" -hooks = [{ id = "python-mock-hooks" }] +rev = "v0.3.0" +hooks = [{ id = "python-mock-hooks" }, { id = "tests-without-ifs" }] ``` -Run `prek run python-mock-hooks --all-files` (or use `pre-commit` instead of `prek`). +Enable either hook or both. Run `prek run --all-files` to run all enabled hooks +(or use `pre-commit` instead of `prek`). -Checks Python files under `tests/` by default. For another layout, set -`files` on the hook: +Both hooks check Python files under `tests/` by default. For another layout, +set `files` on each enabled hook: - YAML: `files: ^(tests|test)/.*\.py$` - TOML: `files = '^(tests|test)/.*\.py$'` diff --git a/pyproject.toml b/pyproject.toml index dbd3b7f..fc0113d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -13,6 +13,7 @@ dependencies = ["ast-grep-cli==0.45.2"] [project.scripts] python-mock-hooks = "python_mock_hooks:main" +tests-without-ifs = "python_mock_hooks:tests_without_ifs" [tool.hatch.build.targets.wheel] packages = ["src/python_mock_hooks"] diff --git a/sgconfig.yml b/sgconfig.yml index 717d670..c6e8e02 100644 --- a/sgconfig.yml +++ b/sgconfig.yml @@ -1,4 +1,5 @@ ruleDirs: - src/python_mock_hooks/rules + - src/python_mock_hooks/if-rules testConfigs: - testDir: tests diff --git a/src/python_mock_hooks/__init__.py b/src/python_mock_hooks/__init__.py index 897f809..b3d7ef3 100644 --- a/src/python_mock_hooks/__init__.py +++ b/src/python_mock_hooks/__init__.py @@ -1,4 +1,4 @@ -"""Run the bundled monkeypatch policy against explicit filenames.""" +"""Run the bundled testing policies against explicit filenames.""" import subprocess import sys @@ -6,12 +6,22 @@ def main() -> int: + """Check mocking policies.""" + return _scan("sgconfig.yml") + + +def tests_without_ifs() -> int: + """Check for if statements in test bodies.""" + return _scan("ifs-sgconfig.yml") + + +def _scan(config_name: str) -> int: """Forward filenames to ast-grep and preserve its exit status.""" filenames = sys.argv[1:] if not filenames: return 0 - config = Path(__file__).with_name("sgconfig.yml") + config = Path(__file__).with_name(config_name) return subprocess.run( ["ast-grep", "scan", "--config", str(config), "--", *filenames], check=False, diff --git a/src/python_mock_hooks/if-rules/no-if-in-tests.yml b/src/python_mock_hooks/if-rules/no-if-in-tests.yml new file mode 100644 index 0000000..1471185 --- /dev/null +++ b/src/python_mock_hooks/if-rules/no-if-in-tests.yml @@ -0,0 +1,15 @@ +id: no-if-in-tests +language: Python +severity: error +message: Do not use if statements in test bodies; use separate tests or pytest.mark.parametrize for each case +rule: + kind: if_statement + inside: + kind: function_definition + has: + field: name + regex: '^test_' + stopBy: + any: + - kind: function_definition + - kind: class_definition diff --git a/src/python_mock_hooks/ifs-sgconfig.yml b/src/python_mock_hooks/ifs-sgconfig.yml new file mode 100644 index 0000000..c7ecc20 --- /dev/null +++ b/src/python_mock_hooks/ifs-sgconfig.yml @@ -0,0 +1,2 @@ +ruleDirs: + - if-rules diff --git a/tests/__snapshots__/no-if-in-tests-snapshot.yml b/tests/__snapshots__/no-if-in-tests-snapshot.yml new file mode 100644 index 0000000..06bb091 --- /dev/null +++ b/tests/__snapshots__/no-if-in-tests-snapshot.yml @@ -0,0 +1,220 @@ +id: no-if-in-tests +snapshots: + ? |- + @pytest.mark.parametrize("enabled", [True, False]) + def test_result(enabled): + if enabled: + assert result == 1 + : labels: + - source: |- + if enabled: + assert result == 1 + style: primary + start: 81 + end: 119 + - source: test_result + style: secondary + start: 55 + end: 66 + - source: |- + def test_result(enabled): + if enabled: + assert result == 1 + style: secondary + start: 51 + end: 119 + ? |- + async def test_result(): + if enabled: + assert await calculate() == 1 + : labels: + - source: |- + if enabled: + assert await calculate() == 1 + style: primary + start: 29 + end: 78 + - source: test_result + style: secondary + start: 10 + end: 21 + - source: |- + async def test_result(): + if enabled: + assert await calculate() == 1 + style: secondary + start: 0 + end: 78 + ? |- + class TestResult: + def test_result(self): + if enabled: + assert result == 1 + : labels: + - source: |- + if enabled: + assert result == 1 + style: primary + start: 53 + end: 95 + - source: test_result + style: secondary + start: 26 + end: 37 + - source: |- + def test_result(self): + if enabled: + assert result == 1 + style: secondary + start: 22 + end: 95 + ? |- + def test_result(): + for result in results: + if result: + assert result == 1 + : labels: + - source: |- + if result: + assert result == 1 + style: primary + start: 54 + end: 95 + - source: test_result + style: secondary + start: 4 + end: 15 + - source: |- + def test_result(): + for result in results: + if result: + assert result == 1 + style: secondary + start: 0 + end: 95 + ? |- + def test_result(): + if enabled: + assert result == 1 + : labels: + - source: |- + if enabled: + assert result == 1 + style: primary + start: 23 + end: 61 + - source: test_result + style: secondary + start: 4 + end: 15 + - source: |- + def test_result(): + if enabled: + assert result == 1 + style: secondary + start: 0 + end: 61 + ? |- + def test_result(): + if enabled: + assert result == 1 + elif fallback: + assert result == 2 + else: + assert result == 3 + : labels: + - source: |- + if enabled: + assert result == 1 + elif fallback: + assert result == 2 + else: + assert result == 3 + style: primary + start: 23 + end: 144 + - source: test_result + style: secondary + start: 4 + end: 15 + - source: |- + def test_result(): + if enabled: + assert result == 1 + elif fallback: + assert result == 2 + else: + assert result == 3 + style: secondary + start: 0 + end: 144 + ? |- + def test_result(): + if enabled: assert result == 1 + : labels: + - source: 'if enabled: assert result == 1' + style: primary + start: 23 + end: 53 + - source: test_result + style: secondary + start: 4 + end: 15 + - source: |- + def test_result(): + if enabled: assert result == 1 + style: secondary + start: 0 + end: 53 + ? |- + def test_result(): + try: + calculate() + except ValueError: + if enabled: + pytest.fail("unexpected error") + : labels: + - source: |- + if enabled: + pytest.fail("unexpected error") + style: primary + start: 79 + end: 134 + - source: test_result + style: secondary + start: 4 + end: 15 + - source: |- + def test_result(): + try: + calculate() + except ValueError: + if enabled: + pytest.fail("unexpected error") + style: secondary + start: 0 + end: 134 + ? |- + def test_result(): + with context(): + if enabled: + assert result == 1 + : labels: + - source: |- + if enabled: + assert result == 1 + style: primary + start: 47 + end: 89 + - source: test_result + style: secondary + start: 4 + end: 15 + - source: |- + def test_result(): + with context(): + if enabled: + assert result == 1 + style: secondary + start: 0 + end: 89 diff --git a/tests/no-if-in-tests-test.yml b/tests/no-if-in-tests-test.yml new file mode 100644 index 0000000..eca3473 --- /dev/null +++ b/tests/no-if-in-tests-test.yml @@ -0,0 +1,96 @@ +id: no-if-in-tests +valid: + - |- + def test_result(): + assert result == expected + - |- + @pytest.mark.parametrize("value, expected", [(1, 2), (2, 3)]) + def test_result(value, expected): + assert calculate(value) == expected + - |- + if TYPE_CHECKING: + from example import Result + - |- + @pytest.fixture + def resource(): + if available: + return create_resource() + - |- + def helper(value): + if value: + return value + - |- + def test_result(): + def helper(value): + if value: + return value + assert helper(1) == 1 + - |- + def test_result(): + class Result: + if enabled: + value = 1 + assert Result.value == 1 + - |- + class TestResult: + def helper(self): + if enabled: + return 1 + - |- + def test_result(): + text = "if enabled: pass" + # if enabled: pass + assert text + - |- + def test_result(): + assert [value for value in values if value] == [1] + - |- + def test_result(): + assert (1 if enabled else 2) == expected +invalid: + - |- + def test_result(): + if enabled: + assert result == 1 + - |- + def test_result(): + if enabled: + assert result == 1 + elif fallback: + assert result == 2 + else: + assert result == 3 + - |- + async def test_result(): + if enabled: + assert await calculate() == 1 + - |- + class TestResult: + def test_result(self): + if enabled: + assert result == 1 + - |- + @pytest.mark.parametrize("enabled", [True, False]) + def test_result(enabled): + if enabled: + assert result == 1 + - |- + def test_result(): + for result in results: + if result: + assert result == 1 + - |- + def test_result(): + with context(): + if enabled: + assert result == 1 + - |- + def test_result(): + try: + calculate() + except ValueError: + if enabled: + pytest.fail("unexpected error") + - |- + def test_result(): + if enabled: assert result == 1 diff --git a/tests/test_hook.py b/tests/test_hook.py index 4a7d588..5e86c9b 100644 --- a/tests/test_hook.py +++ b/tests/test_hook.py @@ -55,7 +55,7 @@ def test_installed_hook( checked("git", "init", "-q", cwd=tmp_path) (tmp_path / ".pre-commit-config.yaml").write_text( f"repos:\n - repo: {json.dumps(str(repo))}\n rev: {revision}\n" - " hooks:\n - id: python-mock-hooks\n" + " hooks:\n - id: python-mock-hooks\n - id: tests-without-ifs\n" ) (tmp_path / "tests").mkdir() (tmp_path / "src").mkdir() @@ -74,6 +74,7 @@ def test_installed_hook( (tmp_path / "sgconfig.yml").write_text("ruleDirs: [does-not-exist]\n") checked("git", "add", ".", cwd=tmp_path) checked(runner, "run", "python-mock-hooks", "--all-files", cwd=tmp_path) + checked(runner, "run", "tests-without-ifs", "--all-files", cwd=tmp_path) (tmp_path / "tests/test forbidden.py").write_text(forbidden) checked("git", "add", ".", cwd=tmp_path) result = run(runner, "run", "python-mock-hooks", "--all-files", cwd=tmp_path) @@ -96,11 +97,24 @@ def test_installed_hook( assert result.returncode != 0 assert "no-mocker-patch" in result.stdout + result.stderr assert "test mocker.py" in result.stdout + result.stderr + checked(runner, "run", "tests-without-ifs", "--all-files", cwd=tmp_path) + + (tmp_path / "tests/test mocker.py").write_text("pass\n") + (tmp_path / "tests/test branching.py").write_text( + "def test_result():\n if enabled:\n assert result == 1\n" + ) + checked("git", "add", ".", cwd=tmp_path) + checked(runner, "run", "python-mock-hooks", "--all-files", cwd=tmp_path) + result = run(runner, "run", "tests-without-ifs", "--all-files", cwd=tmp_path) + assert result.returncode != 0 + assert "no-if-in-tests" in result.stdout + result.stderr + assert "test branching.py" in result.stdout + result.stderr -def test_no_filenames_does_not_scan(tmp_path: Path) -> None: +@pytest.mark.parametrize("hook", ["python-mock-hooks", "tests-without-ifs"]) +def test_no_filenames_does_not_scan(hook: str, tmp_path: Path) -> None: (tmp_path / "bad.py").write_text('monkeypatch.setattr(obj, "name", value)\n') - checked("python-mock-hooks", cwd=tmp_path) + checked(hook, cwd=tmp_path) def test_no_uv_dependency(tmp_path: Path) -> None: From 838eb26db0d6c666df07f2c05bfd63e6a2c896e7 Mon Sep 17 00:00:00 2001 From: sverhoeven Date: Wed, 7 Oct 2026 16:08:11 +0200 Subject: [PATCH 2/2] With new hook, rename should be done, include suggestion here --- rename.md | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) create mode 100644 rename.md diff --git a/rename.md b/rename.md new file mode 100644 index 0000000..2aa3df0 --- /dev/null +++ b/rename.md @@ -0,0 +1,89 @@ +# Naming options + +The repository contains two independently selectable Python test policies: + +- Restrict `unittest.mock`, `mocker.patch`, and most `monkeypatch` methods. +- Reject `if` statements inside `test_*` functions and methods. + +Compatibility is not a requirement. The options below assume a clean rename, +without legacy hook IDs or command aliases. + +## Examples from established hooks + +Existing projects use several naming patterns rather than one uniform convention: + +- Actions: `check-ast`, `detect-private-key`, and `check-builtin-literals` in + [pre-commit-hooks](https://github.com/pre-commit/pre-commit-hooks/blob/main/.pre-commit-hooks.yaml). +- Prohibitions with a language prefix: `python-no-eval` and + `python-no-log-warn` in + [pygrep-hooks](https://github.com/pre-commit/pygrep-hooks/blob/main/.pre-commit-hooks.yaml). +- Tool and action: `ruff-check` and `ruff-format` in + [ruff-pre-commit](https://github.com/astral-sh/ruff-pre-commit/blob/main/.pre-commit-hooks.yaml). + +These examples support short, descriptive hook IDs with hyphens. A collection's +repository name can describe its overall purpose while each hook ID describes +one policy. + +## Repository names + +| Name | Pros | Cons | +| --- | --- | --- | +| **`python-test-hooks`** | Clearly identifies the language and purpose; accommodates additional test policies. | Broad; does not emphasize that the rules are opinionated. | +| `python-test-policy-hooks` | Explicitly describes selectable testing policies. | Longer and less convenient to type. | +| `python-test-pre-commit` | Makes the integration immediately obvious, following the `ruff-pre-commit` pattern. | Less natural if the commands also become standalone lint tools. | +| `python-test-lint` | Concise; describes static checks and suits standalone use. | Does not advertise the pre-commit integration. | +| `python-mock-hooks` | Short and accurately describes the original mocking policy. | Does not describe the new conditional-statement hook or future unrelated policies. | + +## Mocking hook alternatives to `python-mock-hooks` + +The policy permits `mocker.Mock`, `mocker.spy`, and several `monkeypatch` methods. +Its name should therefore describe restrictions rather than promise a complete +ban on mocks. + +| Hook ID | Pros | Cons | +| --- | --- | --- | +| **`check-test-mocking`** | Uses the established `check-*` pattern; covers all three mocking APIs without claiming a total ban. | The exact restrictions require a description. | +| `restrict-test-mocking` | Clearly communicates selective restrictions and test scope. | `restrict-*` is less familiar than the patterns in the examples above. | +| `check-mocking-policy` | Accurately communicates an opinionated policy with exceptions. | Omits test scope and is somewhat abstract. | +| `python-check-test-mocking` | Explicit language and test scope; follows the language-prefix pattern used by pygrep-hooks. | Longer; Python is already present in the recommended repository name. | +| `no-patching-in-tests` | Directly communicates the main discouraged behavior. | Overstates the policy because environment and working-directory monkeypatching are allowed; also understates the ban on `unittest.mock` imports. | +| `tests-with-restricted-mocking` | Reads naturally and pairs with `tests-without-ifs`. | Long and less consistent with common action-based hook IDs. | + +Avoid `no-mocks` and `tests-without-mocks`: both imply that all mocking is +forbidden, which does not match the implementation. + +## Conditional-statement hook names + +| Hook ID | Pros | Cons | +| --- | --- | --- | +| **`no-if-in-tests`** | Direct; follows the prohibition pattern; matches the existing rule ID. | Its description should clarify that conditional expressions remain allowed. | +| `python-no-if-in-tests` | Explicit language scope; follows pygrep-hooks naming. | Longer and repeats the language in the recommended repository name. | +| `tests-without-ifs` | Readable and already used by the new hook. | Less consistent with the action and prohibition patterns above; `ifs` is informal. | +| `check-test-if-statements` | Uses `check-*` and precisely identifies the syntax being checked. | Does not immediately communicate that every matching statement is forbidden. | +| `check-test-conditionals` | Concise and uses the established `check-*` pattern. | Suggests coverage of conditional expressions and comprehension filters, which are allowed. | +| `no-test-branching` | Communicates the policy's motivation. | Overstates coverage: `match`, conditional expressions, and other control flow remain allowed. | + +## Suggested combinations + +| Style | Repository | Mocking hook | Conditional-statement hook | Tradeoff | +| --- | --- | --- | --- | --- | +| **Recommended** | `python-test-hooks` | `check-test-mocking` | `no-if-in-tests` | Short, descriptive names; each hook uses the wording that best fits its actual policy. | +| Explicit language | `python-test-hooks` | `python-check-test-mocking` | `python-no-if-in-tests` | Hook IDs make sense outside the repository context, at the cost of length. | +| Consistent actions | `python-test-policy-hooks` | `check-test-mocking` | `check-test-if-statements` | Both IDs follow `check-*`; the prohibition is expressed in the hook description. | +| Natural phrasing | `python-test-hooks` | `tests-with-restricted-mocking` | `tests-without-ifs` | Both names describe the desired tests; the mocking ID is lengthy. | + +With the recommended names, the consumer configuration would be: + +```yaml +repos: + - repo: https://github.com/i-VRESSE/python-test-hooks + rev: v0.3.0 + hooks: + - id: check-test-mocking + - id: no-if-in-tests +``` + +These are proposals; repository, package, command, and hook names have not been +changed. Once a combination is selected, the rename should update the GitHub +repository, package metadata and import path, console commands, hook manifest, +documentation, and integration tests together.