Add pre-commit hook and conftest guard for test conventions

[npow]

Summary

• Add devtools/check_test_conventions.py — AST-based pre-commit hook that blocks commits introducing unittest imports, TestCase subclasses, unittest.mock usage, or class-based test grouping in test files.
• Wire it into .pre-commit-config.yaml as a repo: local hook scoped to test/**/*.py.
• Add a pytest_collection_modifyitems guard in test/unit/conftest.py that marks any new TestCase subclass with strict xfail.
• Legacy files that predate the conventions are allowlisted in both locations.

Context / Motivation

PR #3150 codifies test conventions in CONTRIBUTING.md and AGENTS.md. This PR adds programmatic enforcement so the conventions aren't just documentation — they block violations at commit time (pre-commit hook) and test time (conftest guard).

Changes Made

File	What
devtools/check_test_conventions.py	New AST checker: parses test files for 4 violation types, allowlists 12 legacy files
.pre-commit-config.yaml	New check-test-conventions local hook, runs on test/**/*.py
test/unit/conftest.py	pytest_collection_modifyitems hook that xfails non-allowlisted TestCase subclasses

Testing

• Pre-commit hook correctly passes allowlisted legacy files and catches all 4 violation types in new files.
• Existing unit tests unaffected: 55 passed for the 3 allowlisted unit test files, 151 passed, 19 skipped overall (pre-existing importlib.metadata errors in mutators/inheritance tests are unrelated).

Trade-offs

• The hook enforces mechanical rules only (no unittest, no classes, no unittest.mock). Judgment-call conventions (when to parametrize, factory fixtures, data placement) remain review-time guidance.
• The allowlist is deliberately generous — 12 legacy files. Entries should be removed as files are migrated.

🤖 Generated with Claude Code

[greptile-apps]

Greptile Summary

This PR adds programmatic enforcement of test conventions via an AST-based pre-commit hook (devtools/check_test_conventions.py) and a pytest_collection_modifyitems guard in test/unit/conftest.py. The approach is sound, but there is one concrete self-defeating issue:

• test/unit/conftest.py is not in LEGACY_ALLOWLIST, yet it now contains import unittest. The hook's files: ^test/.*\.py$ pattern matches conftest.py, so the next commit that touches this file will be rejected by the hook it helped introduce. Fix: add "test/unit/conftest.py" to LEGACY_ALLOWLIST or add exclude: conftest\.py$ to the hook config.

Confidence Score: 3/5

Not safe to merge as-is — the hook will self-block on future edits to test/unit/conftest.py.

One P1 defect: import unittest in test/unit/conftest.py is not covered by LEGACY_ALLOWLIST, so the newly-added hook will reject future staged changes to that file. Remaining findings are P2 (ast.dump false positives, missing async detection, fragile endswith matching).

devtools/check_test_conventions.py — LEGACY_ALLOWLIST must include test/unit/conftest.py and the ast.dump TestCase detection should be tightened.

Important Files Changed

Filename	Overview
devtools/check_test_conventions.py	New AST-based pre-commit hook with a P1 bug: test/unit/conftest.py (which now imports unittest) is missing from LEGACY_ALLOWLIST, causing the hook to self-block on future edits to that file. Also has minor false-positive risks in ast.dump-based TestCase detection and missing async method coverage.
.pre-commit-config.yaml	Adds the check-test-conventions local hook; files: ^test/.*\.py$ is intentionally broad and catches conftest.py files, which compounds the missing-allowlist bug in the checker.
test/unit/conftest.py	Adds pytest_collection_modifyitems guard that applies xfail(strict=True) to non-allowlisted TestCase subclasses; logic is sound, but the import unittest here is the root cause of the allowlist gap flagged above.

_{Reviews (1): Last reviewed commit: "Add pre-commit hook and conftest guard t..." | Re-trigger Greptile}

[greptile-apps]

conftest.py missing from allowlist causes self-defeating hook failure

test/unit/conftest.py now contains import unittest (line 3), but it is not in LEGACY_ALLOWLIST. The pre-commit hook's files pattern ^test/.*\.py$ matches test/unit/conftest.py, so the next time anyone modifies that file and stages it, the hook will reject the commit with import unittest -> use pytest / pytest-mock. The enforcement tool would block edits to the file that implements part of that enforcement.

Either add "test/unit/conftest.py" to LEGACY_ALLOWLIST, or add an exclude key to the hook in .pre-commit-config.yaml to skip conftest.py files (e.g. exclude: conftest\.py$).

[greptile-apps]

ast.dump string-match for TestCase can produce false positives

ast.dump(base) serialises the entire AST node to a string and then checks if "TestCase" in base_name. This would match unrelated identifiers that happen to contain the substring "TestCase", such as class TestMyFoo(SomeLib.TestCaseLike). Prefer checking the actual attribute/name directly:

def _is_testcase_base(base):
    if isinstance(base, ast.Name):
        return base.id == "TestCase"
    if isinstance(base, ast.Attribute):
        return base.attr == "TestCase"
    return False

[greptile-apps]

Async test methods not detected in class-based grouping check

The check only scans for ast.FunctionDef nodes, so async def test_* methods (ast.AsyncFunctionDef) are silently missed. A class like class TestFoo: async def test_bar(self): ... would not trigger the violation.

has_test_methods = any(
    isinstance(item, (ast.FunctionDef, ast.AsyncFunctionDef))
    and item.name.startswith("test_")
    for item in node.body
)

[greptile-apps]

endswith allowlist matching is fragile

normalized.endswith(a) would match a path like other_project/test/unit/test_secrets_decorator.py against the allowlist entry test/unit/test_secrets_decorator.py. Pre-commit passes repo-relative paths, so an exact in check after normalisation is safer and more self-documenting:

normalized = path.replace("\\", "/").lstrip("./")
if normalized in LEGACY_ALLOWLIST:
    continue