diff --git a/tests/vacuous_tests/README.md b/tests/vacuous_tests/README.md index 49060f028c7..af2062c6280 100644 --- a/tests/vacuous_tests/README.md +++ b/tests/vacuous_tests/README.md @@ -47,6 +47,8 @@ Verdicts: `vacuous` (survived every mutant), `not_vacuous` (a mutant killed it, Only a `vacuous` verdict authorizes editing a test +`not_vacuous` is a floor, not a compliment: it means the test notices when the code it covers changes behaviour or starts raising, which for an assert-by-not-raising test is all it ever claimed. Reviewing `verified_not_vacuous.json` is still worthwhile, since a cleared test can be weaker than its name suggests + ## Fixing a vacuous test Refactor first: add the assertion the test's own name and docstring imply, then re-run the probe and confirm the mutant that used to survive now dies. Delete only when the behaviour is provably covered somewhere else, and cite that test id. When neither is possible, leave it alone and record why: a human should look at it @@ -61,7 +63,9 @@ python tests/vacuous_tests/flake_gate.py "tests/x/test_y.py::test_z" ## Guardrails -`guardrails.py` runs against the diff and rejects anything that games the metric: files outside `tests/`, edits to `conftest.py` or CI config, edits to this directory's own logic, test removals without a citation, and assertion counts dropping without tests being removed +`guardrails.py` checks the daily automation's own PRs, so it fails by design on the PR that introduced this directory. It reads the committed diff (`base...HEAD`), not the working tree + +It runs against the diff and rejects anything that games the metric: files outside `tests/`, edits to `conftest.py` or CI config, edits to this directory's own logic, test removals without a citation, and assertion counts dropping without tests being removed ```bash python tests/vacuous_tests/guardrails.py --base origin/litellm_internal_staging diff --git a/tests/vacuous_tests/mutation_probe.py b/tests/vacuous_tests/mutation_probe.py index 86cdf21e233..ea2932b98d9 100644 --- a/tests/vacuous_tests/mutation_probe.py +++ b/tests/vacuous_tests/mutation_probe.py @@ -66,6 +66,9 @@ class Mutant: lineno: int description: str source: str + # A test that asserts by not raising can only be killed by a mutant that + # raises, so these go first in the budget. + swallow: bool = False @dataclass @@ -208,7 +211,7 @@ NodeKey = Tuple[str, int, int] def _position(node: ast.AST) -> Tuple[int, int]: - if isinstance(node, (ast.expr, ast.stmt)): + if isinstance(node, (ast.expr, ast.stmt, ast.ExceptHandler)): return (node.lineno, node.col_offset) return (-1, -1) @@ -288,7 +291,7 @@ def generate_mutants(path: str, lines: Iterable[int]) -> List[Mutant]: covered = set(lines) mutants: List[Mutant] = [] - def emit(node: ast.AST, description: str, mutate: Mutation) -> None: + def emit(node: ast.AST, description: str, mutate: Mutation, swallow: bool = False) -> None: lineno, _ = _position(node) if lineno not in covered: return @@ -296,9 +299,19 @@ def generate_mutants(path: str, lines: Iterable[int]) -> List[Mutant]: source = _mutant_source(original, _key(node), mutate) except Exception: return - mutants.append(Mutant(path=path, lineno=lineno, description=description, source=source)) + mutants.append( + Mutant(path=path, lineno=lineno, description=description, source=source, swallow=swallow) + ) for node in ast.walk(tree): + if isinstance(node, ast.ExceptHandler) and not _bare_reraise(node): + emit( + node, + f"stop swallowing `{_snippet(node.type) if node.type else 'except'}` and re-raise", + lambda n: ast.ExceptHandler(type=n.type, name=n.name, body=[ast.Raise()]), + swallow=True, + ) + continue if isinstance(node, ast.Compare) and len(node.ops) == 1 and type(node.ops[0]) in COMPARE_SWAPS: swap = COMPARE_SWAPS[type(node.ops[0])] emit( @@ -351,6 +364,11 @@ def _is_none(node: ast.expr) -> bool: return isinstance(node, ast.Constant) and node.value is None +def _bare_reraise(handler: ast.ExceptHandler) -> bool: + only = handler.body[0] if len(handler.body) == 1 else None + return isinstance(only, ast.Raise) and only.exc is None + + def _snippet(node: ast.AST, limit: int = 60) -> str: try: rendered = ast.unparse(node) @@ -360,7 +378,15 @@ def _snippet(node: ast.AST, limit: int = 60) -> str: def select_mutants(mutants: Sequence[Mutant], limit: int) -> List[Mutant]: - """Round-robin over distinct lines so the budget spreads across the code path.""" + """Round-robin over distinct lines so the budget spreads across the code path. + + Swallowed-exception mutants come first: a test whose only claim is that the + call does not raise cannot be killed by anything else, and getting that + wrong would call a real regression test vacuous. + """ + swallows = [mutant for mutant in mutants if mutant.swallow][:limit] + if swallows: + return swallows + select_mutants([m for m in mutants if not m.swallow], limit - len(swallows)) by_line: Dict[Tuple[str, int], List[Mutant]] = {} for mutant in mutants: by_line.setdefault((mutant.path, mutant.lineno), []).append(mutant) diff --git a/tests/vacuous_tests/test_vacuous_tooling.py b/tests/vacuous_tests/test_vacuous_tooling.py index 5ea546ff565..303438db843 100644 --- a/tests/vacuous_tests/test_vacuous_tooling.py +++ b/tests/vacuous_tests/test_vacuous_tooling.py @@ -239,6 +239,53 @@ def test_module_under_test_is_recognised_from_imports() -> None: assert not mutation_probe._is_under_test("litellm/caching/dual_cache.py", imports) +def test_swallowed_exception_becomes_a_mutant_that_a_no_assert_test_can_notice(tmp_path, monkeypatch) -> None: + root = tmp_path / "root" + (root / "litellm").mkdir(parents=True) + target = root / "litellm" / "hooks.py" + target.write_text( + textwrap.dedent( + """ + def record(value): + try: + return int(value) + except ValueError: + return 0 + """ + ).lstrip() + ) + monkeypatch.setattr(mutation_probe, "REPO_ROOT", str(root)) + mutants = mutation_probe.generate_mutants("litellm/hooks.py", range(1, 6)) + + swallows = [m for m in mutants if m.swallow] + assert len(swallows) == 1 + assert "stop swallowing" in swallows[0].description + assert "raise" in swallows[0].source + assert "return 0" not in swallows[0].source + # A test that only claims "this does not raise" dies to that mutant and to + # nothing else, so it has to be tried first. + assert mutation_probe.select_mutants(mutants, 2)[0].swallow + + +def test_already_reraising_handlers_produce_no_mutant(tmp_path, monkeypatch) -> None: + root = tmp_path / "root" + (root / "litellm").mkdir(parents=True) + (root / "litellm" / "hooks.py").write_text( + textwrap.dedent( + """ + def record(value): + try: + return int(value) + except ValueError: + raise + """ + ).lstrip() + ) + monkeypatch.setattr(mutation_probe, "REPO_ROOT", str(root)) + + assert not [m for m in mutation_probe.generate_mutants("litellm/hooks.py", range(1, 6)) if m.swallow] + + def test_area_rotation_moves_on_each_day_and_is_stable_within_one(monkeypatch) -> None: monkeypatch.setattr(inventory, "cleared_ids", lambda: frozenset()) candidates = [