From 46972263d4b4e660b106827458e8fad4fde6b3d4 Mon Sep 17 00:00:00 2001 From: npow Date: Mon, 27 Apr 2026 05:27:40 +0000 Subject: [PATCH] Add pre-commit hook and conftest guard to enforce test conventions Pre-commit hook (devtools/check_test_conventions.py) AST-parses test files and blocks commits that introduce unittest imports, TestCase subclasses, unittest.mock usage, or class-based test grouping. conftest.py guard marks any new TestCase subclass with strict xfail at test collection time. Legacy files that predate the conventions are allowlisted in both locations. Co-Authored-By: Claude Opus 4.6 --- .pre-commit-config.yaml | 8 ++ devtools/check_test_conventions.py | 122 +++++++++++++++++++++++++++++ test/unit/conftest.py | 28 +++++++ 3 files changed, 158 insertions(+) create mode 100755 devtools/check_test_conventions.py diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 35a5a46eace..8ee91f02cda 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -12,6 +12,14 @@ repos: - id: shellcheck files: ^devtools/ci/ args: [-e, SC1091] # don't follow sourced files + - repo: local + hooks: + - id: check-test-conventions + name: check test conventions + entry: python devtools/check_test_conventions.py + language: python + files: ^test/.*\.py$ + types: [python] - repo: https://github.com/ambv/black rev: 25.12.0 hooks: diff --git a/devtools/check_test_conventions.py b/devtools/check_test_conventions.py new file mode 100755 index 00000000000..457c9a02803 --- /dev/null +++ b/devtools/check_test_conventions.py @@ -0,0 +1,122 @@ +#!/usr/bin/env python +"""Pre-commit hook: enforce test conventions from CONTRIBUTING.md. + +Checks test files for: + 1. unittest imports (use pytest instead) + 2. unittest.mock usage (use pytest-mock's mocker fixture) + 3. TestCase subclasses (use plain functions) + 4. Class-based test grouping (use module-level functions) + +Legacy files that predate these conventions are allowlisted. +Remove entries as files are migrated. +""" +import ast +import sys + +LEGACY_ALLOWLIST = frozenset( + { + "test/unit/test_secrets_decorator.py", + "test/unit/test_s3_storage.py", + "test/unit/test_system_context.py", + "test/unit/inheritance/test_inheritance.py", + "test/unit/mutators/test_add_decorator_returns.py", + "test/unit/mutators/test_dual_inheritance.py", + "test/unit/mutators/test_flow_mutator_addition.py", + "test/unit/mutators/test_post_step_none_false.py", + "test/unit/mutators/test_remove_decorator_guard.py", + "test/unit/mutators/test_string_step_mutator.py", + "test/cmd/diff/test_metaflow_diff.py", + "test/cmd/develop/test_stub_generator.py", + } +) + + +def check_file(path): + violations = [] + + with open(path) as f: + source = f.read() + + try: + tree = ast.parse(source, filename=path) + except SyntaxError: + return violations + + for node in ast.walk(tree): + # 1. import unittest / from unittest import ... + if isinstance(node, ast.Import): + for alias in node.names: + if alias.name == "unittest" or alias.name.startswith("unittest."): + violations.append( + (node.lineno, f"import {alias.name}", "use pytest / pytest-mock") + ) + + if isinstance(node, ast.ImportFrom) and node.module: + if node.module == "unittest" or node.module.startswith("unittest."): + names = ", ".join(a.name for a in node.names) + violations.append( + ( + node.lineno, + f"from {node.module} import {names}", + "use pytest / pytest-mock (mocker fixture)", + ) + ) + + # 2. class Test*(unittest.TestCase) — TestCase subclass + if isinstance(node, ast.ClassDef): + for base in node.bases: + base_name = ast.dump(base) + if "TestCase" in base_name: + violations.append( + ( + node.lineno, + f"class {node.name}(…TestCase)", + "use module-level test functions, not TestCase", + ) + ) + break + + # 3. class Test* without TestCase — class-based grouping + if isinstance(node, ast.ClassDef) and node.name.startswith("Test"): + is_testcase = any("TestCase" in ast.dump(b) for b in node.bases) + if not is_testcase: + has_test_methods = any( + isinstance(item, ast.FunctionDef) and item.name.startswith("test_") + for item in node.body + ) + if has_test_methods: + violations.append( + ( + node.lineno, + f"class {node.name}", + "use module-level test functions, not test classes", + ) + ) + + return violations + + +def main(): + status = 0 + for path in sys.argv[1:]: + # Normalize to forward-slash relative path for allowlist matching + normalized = path.replace("\\", "/") + if any(normalized.endswith(a) for a in LEGACY_ALLOWLIST): + continue + + violations = check_file(path) + for lineno, what, fix in violations: + print(f"{path}:{lineno}: {what} -> {fix}") + status = 1 + + if status: + print( + "\nSee CONTRIBUTING.md § Test conventions. " + "Legacy files are allowlisted in devtools/check_test_conventions.py." + ) + + return status + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/test/unit/conftest.py b/test/unit/conftest.py index f2803f45060..f86c5771187 100644 --- a/test/unit/conftest.py +++ b/test/unit/conftest.py @@ -1,7 +1,35 @@ """Shared pytest configuration for unit tests.""" +import unittest + import pytest +_LEGACY_TESTCASE_FILES = frozenset( + { + "test_secrets_decorator.py", + "test_s3_storage.py", + "test_system_context.py", + } +) + + +def pytest_collection_modifyitems(items): + for item in items: + if not isinstance(item, pytest.Function): + continue + if item.cls and issubclass(item.cls, unittest.TestCase): + filename = item.fspath.basename + if filename not in _LEGACY_TESTCASE_FILES: + item.add_marker( + pytest.mark.xfail( + reason=( + "unittest.TestCase is not allowed in new tests. " + "See CONTRIBUTING.md Test conventions." + ), + strict=True, + ) + ) + def pytest_addoption(parser): """Add custom command line options shared across unit test suites."""