mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-12 23:01:41 +00:00
fix(tests): mutate swallowed exceptions so assert-by-not-raising tests are not called vacuous
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This commit is contained in:
parent
a72b356f57
commit
37eea9c6e8
3 changed files with 82 additions and 5 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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 = [
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue