From e8d688faf6e42399227adbe2dc54cfa9c6eae086 Mon Sep 17 00:00:00 2001 From: Samran Asif Date: Wed, 9 Sep 2026 16:04:06 +0500 Subject: [PATCH] fix(scoring): test the arithmetic this benchmark's numbers are made of Phase 6 of the plan calls for mutation-testing the scoring logic, on the grounds that a measurement tool's measurement is its product. Doing it put tooltrace/scoring/builtin.py at a 57.4% mutation score: 23 of 54 mutants survived, meaning 23 behaviours in the scorers that no test constrained. Three findings, in order of severity. **A scorer that could be inverted silently.** In `file_not_contains`, mutating `in` to `not in` survived. That scorer would have scored 1.0 for a file *containing* the forbidden text and 0.0 for a clean one -- doing exactly the opposite of its name -- with the whole suite green. **A constraint that has never worked.** Writing a boundary test for `git_diff`'s `max_changed_files` crashed it: changed = {line.split()[2] for line in ... if line.startswith("+++ b/")} A git diff header is `+++ b/path`, two whitespace-separated fields, so `[2]` raises IndexError on every real diff. The constraint could not return a score, only crash. No shipped task uses it, which is why nothing noticed. Verified the format against real `git diff HEAD` output rather than assuming. **The pytest scorer's core calculation was entirely unconstrained.** Every operator in total = passed + failed + errors ratio = passed / total if total else 0.0 score = 1.0 if ratio >= min_ratio and errors == 0 else round(ratio, 4) survived mutation: the total could be computed by subtraction, the ratio by multiplication, the threshold inverted, and the errors clause flipped from `and` to `or`. It was untested because it was unreachable -- welded inside `_tests_pass` behind a `subprocess.run` of a real pytest in a temp workspace. Extracting `score_pytest_output` is what made the assertions possible. Mutation score now 77.8% (42/54). Every comparison and arithmetic mutant is killed. The 12 survivors are boolean short-circuits and bool constants, mostly `isinstance(...) and ...` guards whose second operand is unreachable without the first -- equivalent mutants rather than gaps. Stating that rather than chasing the number: a mutation score is a diagnostic, not a target. One of my own mistakes worth recording: my first attempt to kill the JSON-path `Lt -> LtE` mutant used index 9 on a three-element array, where `9 < 3` and `9 <= 3` are both false, so the mutant survived a test written specifically to kill it. Only `idx == len` distinguishes them. Verified: ruff, ruff format, mypy (73 files), 340 tests pass, coverage 85.48% against the 80% floor. --- tests/test_pytest_scorer_arithmetic.py | 125 +++++++++++++++++++ tests/test_scorer_boundaries.py | 158 +++++++++++++++++++++++++ tooltrace/scoring/builtin.py | 63 +++++++--- 3 files changed, 329 insertions(+), 17 deletions(-) create mode 100644 tests/test_pytest_scorer_arithmetic.py create mode 100644 tests/test_scorer_boundaries.py diff --git a/tests/test_pytest_scorer_arithmetic.py b/tests/test_pytest_scorer_arithmetic.py new file mode 100644 index 0000000..83eb859 --- /dev/null +++ b/tests/test_pytest_scorer_arithmetic.py @@ -0,0 +1,125 @@ +"""The pytest scorer's arithmetic, pinned operator by operator. + +Mutation testing put `tooltrace/scoring/builtin.py` at a 57% mutation score: +23 of 54 mutants survived. The worst cluster was this scorer's core +calculation, where *every* operator survived -- + + total = passed + failed + errors # Add -> Sub survived + ratio = passed / total if total else 0 # Div -> Mult survived + score = 1.0 if ratio >= min_ratio and errors == 0 else round(ratio, 4) + # GtE -> Gt, GtE -> LtE, Eq -> NotEq, and -> or survived + +-- meaning the number this benchmark publishes could have been computed by +subtraction, or by multiplying instead of dividing, or with an inverted +threshold, and the whole suite would still have been green. + +They were untested because they were unreachable: the calculation sat inside +`_tests_pass`, welded to a `subprocess.run` of a real pytest in a temporary +workspace. Extracting `score_pytest_output` is what made these assertions +possible; each test below corresponds to a specific surviving mutant. +""" + +from __future__ import annotations + +import pytest +from tooltrace.scoring.builtin import score_pytest_output + + +def score(output: str, min_ratio: float = 0.8) -> float: + return score_pytest_output(output, min_ratio).score + + +# --------------------------------------------------------- total = a + b + c + + +def test_total_sums_the_three_counts() -> None: + """Kills `Add -> Sub` on `total = passed + failed + errors`. + + With subtraction, 6 passed / 3 failed gives total 3 and a ratio of 2.0 -- + a score above 1. The ratio has to be 6/9. + """ + assert score("6 passed, 3 failed", min_ratio=0.99) == pytest.approx(0.6667, abs=1e-4) + + +def test_errors_count_toward_the_total_not_against_it() -> None: + """The second `Add -> Sub` survivor, on the errors term.""" + assert score("6 passed, 2 error", min_ratio=0.99) == pytest.approx(0.75, abs=1e-4) + + +# ------------------------------------------------- ratio = passed / total + + +def test_ratio_divides_rather_than_multiplies() -> None: + """Kills `Div -> Mult`. + + 4 passed of 8 is 0.5. Multiplied it would be 32, which `round` would + happily return as a score. + """ + assert score("4 passed, 4 failed", min_ratio=0.99) == pytest.approx(0.5) + + +def test_no_tests_at_all_scores_zero_rather_than_dividing_by_zero() -> None: + assert score("no tests ran", min_ratio=0.5) == 0.0 + + +# ----------------------------------------- score = 1.0 if ratio >= min_ratio + + +def test_a_ratio_exactly_at_the_threshold_passes() -> None: + """Kills `GtE -> Gt`. The boundary is inclusive: 0.8 meets a 0.8 bar.""" + assert score("8 passed, 2 failed", min_ratio=0.8) == 1.0 + + +def test_a_ratio_below_the_threshold_does_not_pass() -> None: + """Kills `GtE -> LtE`, which would invert the comparison entirely.""" + assert score("7 passed, 3 failed", min_ratio=0.8) == pytest.approx(0.7) + + +def test_a_perfect_run_still_fails_a_higher_bar() -> None: + """A second angle on the inverted-comparison mutant.""" + assert score("9 passed, 1 failed", min_ratio=0.95) == pytest.approx(0.9) + + +# ------------------------------------------------------- and errors == 0 + + +def test_errors_deny_full_marks_even_when_the_ratio_is_met() -> None: + """Kills `Eq -> NotEq` and `and -> or` on the errors clause. + + A collection error means the suite did not fully execute, so a passing + ratio among the tests that *did* run must not be reported as success. + """ + assert score("18 passed, 2 error", min_ratio=0.5) == pytest.approx(0.9) + + +def test_zero_errors_with_a_met_ratio_is_full_marks() -> None: + """The other side of `Eq -> NotEq`: no errors must mean the clause holds.""" + assert score("10 passed", min_ratio=0.5) == 1.0 + + +def test_errors_alone_score_zero() -> None: + assert score("3 error", min_ratio=0.5) == 0.0 + + +# ----------------------------------------------------------------- parsing + + +@pytest.mark.parametrize( + ("output", "expected"), + [ + ("=== 5 passed in 0.12s ===", 1.0), + ("=== 5 passed, 5 failed in 1.2s ===", 0.5), + ("", 0.0), + ("collected 0 items", 0.0), + ], +) +def test_real_pytest_summary_shapes(output: str, expected: float) -> None: + assert score(output, min_ratio=0.75) == pytest.approx(expected) + + +def test_the_detail_string_reports_what_it_counted() -> None: + """A score with no visible basis is not auditable.""" + outcome = score_pytest_output("7 passed, 2 failed, 1 error", 0.9) + assert "passed=7" in outcome.detail + assert "failed=2" in outcome.detail + assert "errors=1" in outcome.detail diff --git a/tests/test_scorer_boundaries.py b/tests/test_scorer_boundaries.py new file mode 100644 index 0000000..2eccc5c --- /dev/null +++ b/tests/test_scorer_boundaries.py @@ -0,0 +1,158 @@ +"""Boundary and inversion behaviour in scorers that mutation testing left open. + +After pinning the pytest scorer's arithmetic the mutation score for +`tooltrace/scoring/builtin.py` moved from 57.4% to 70.4%. Three of the +remaining survivors were real gaps rather than equivalent mutants, and one of +them inverts a safety check outright: + + line 70 `In -> NotIn` in `file_not_contains`: a file *containing* the + forbidden text would score 1.0 and a clean file + 0.0 -- the scorer doing precisely the opposite of + its name, with the suite still green. + line 247 `Gt -> GtE` in `git_diff`: a diff exactly at `max_changed` + would be rejected instead of accepted. + line 310 `Lt -> LtE` in JSON-path indexing: the last valid index would + raise IndexError instead of resolving. + +A scorer is the thing a benchmark's numbers are made of. An inverted one does +not produce a slightly wrong score; it produces a confidently wrong verdict. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import tooltrace.scoring # noqa: F401 - importing registers the scorers +from tooltrace.core.registry import scorer_registry + + +def run(name: str, params: dict, workspace: Path): + return scorer_registry.get(name)(params, workspace) + + +# ------------------------------------------------- file_not_contains (line 70) + + +def test_forbidden_text_present_scores_zero(tmp_path: Path) -> None: + """The inversion mutant, stated directly. + + If `in` became `not in`, this file -- which *does* contain the forbidden + string -- would score 1.0. + """ + (tmp_path / "out.txt").write_text("result: TODO fix me later", encoding="utf-8") + outcome = run("file_not_contains", {"path": "out.txt", "text": "TODO"}, tmp_path) + assert outcome.score == 0.0, "a file containing the forbidden text must not pass" + + +def test_forbidden_text_absent_scores_one(tmp_path: Path) -> None: + """The other half: without the inversion, a clean file must pass.""" + (tmp_path / "out.txt").write_text("result: complete", encoding="utf-8") + outcome = run("file_not_contains", {"path": "out.txt", "text": "TODO"}, tmp_path) + assert outcome.score == 1.0 + + +def test_none_of_rejects_when_any_one_is_present(tmp_path: Path) -> None: + """`none_of` is a list; any single hit must fail the whole check.""" + (tmp_path / "out.txt").write_text("clean except FIXME", encoding="utf-8") + outcome = run( + "file_not_contains", + {"path": "out.txt", "none_of": ["TODO", "FIXME", "XXX"]}, + tmp_path, + ) + assert outcome.score == 0.0 + + +def test_none_of_passes_when_all_are_absent(tmp_path: Path) -> None: + (tmp_path / "out.txt").write_text("entirely clean", encoding="utf-8") + outcome = run( + "file_not_contains", + {"path": "out.txt", "none_of": ["TODO", "FIXME", "XXX"]}, + tmp_path, + ) + assert outcome.score == 1.0 + + +# ----------------------------------------------- api_state json-path indexing + + +def test_json_path_resolves_the_last_valid_array_index(tmp_path: Path) -> None: + """Kills `Lt -> LtE` on `idx < len(current)` in the api_state walker. + + Index 2 of a three-element array is valid. With `<=` the guard would admit + index 3 and raise IndexError on a document a user could legitimately + supply. + """ + (tmp_path / "state.json").write_text(json.dumps({"items": [1, 2, 99]}), encoding="utf-8") + outcome = run( + "api_state", + {"file": "state.json", "json_path": "items.2", "equals": 99}, + tmp_path, + ) + assert outcome.score == 1.0 + + +def test_json_path_past_the_end_scores_zero_rather_than_raising(tmp_path: Path) -> None: + """An out-of-range index is a failed assertion, not a crash.""" + (tmp_path / "state.json").write_text(json.dumps({"items": [1, 2, 3]}), encoding="utf-8") + outcome = run( + "api_state", + {"file": "state.json", "json_path": "items.9", "equals": 3}, + tmp_path, + ) + assert outcome.score == 0.0 + + +def test_json_path_at_exactly_the_array_length_is_out_of_range(tmp_path: Path) -> None: + """The precise boundary `Lt -> LtE` turns on. + + My first attempt at this used index 9 on a three-element array, where + `9 < 3` and `9 <= 3` are both false -- so the mutant survived a test + written to kill it. Only `idx == len` distinguishes them: with `<=`, + `items.3` indexes past the end and raises IndexError instead of scoring 0. + """ + (tmp_path / "state.json").write_text(json.dumps({"items": [1, 2, 3]}), encoding="utf-8") + outcome = run( + "api_state", + {"file": "state.json", "json_path": "items.3", "equals": None}, + tmp_path, + ) + assert outcome.score in (0.0, 1.0) # must not raise + assert outcome.score == 1.0, "an index past the end resolves to None, which equals None here" + + +# ------------------------------------------------- git_diff max_changed_files + + +def _git_workspace(tmp_path: Path, files: int) -> Path: + import subprocess + + subprocess.run(["git", "init", "-q"], cwd=tmp_path, check=True) + subprocess.run(["git", "config", "user.email", "t@example.com"], cwd=tmp_path, check=True) + subprocess.run(["git", "config", "user.name", "t"], cwd=tmp_path, check=True) + (tmp_path / "seed.txt").write_text("seed\n", encoding="utf-8") + subprocess.run(["git", "add", "-A"], cwd=tmp_path, check=True) + subprocess.run(["git", "commit", "-qm", "seed"], cwd=tmp_path, check=True) + for i in range(files): + (tmp_path / f"f{i}.txt").write_text(f"changed {i}\n", encoding="utf-8") + subprocess.run(["git", "add", "-A"], cwd=tmp_path, check=True) + return tmp_path + + +def test_a_diff_exactly_at_max_changed_files_is_accepted(tmp_path: Path) -> None: + """Kills `Gt -> GtE` on `len(changed) > limit`. + + The limit is inclusive: changing exactly `max_changed_files` files is + within budget. With `>=` it would be rejected -- an off-by-one that fails + a submission which met the stated constraint. + """ + ws = _git_workspace(tmp_path, files=3) + outcome = run("git_diff", {"max_changed_files": 3}, ws) + assert outcome.score == 1.0 + + +def test_a_diff_over_max_changed_files_is_rejected(tmp_path: Path) -> None: + """Kills `Gt -> Lt`, which would invert the budget entirely.""" + ws = _git_workspace(tmp_path, files=5) + outcome = run("git_diff", {"max_changed_files": 3}, ws) + assert outcome.score == 0.0 diff --git a/tooltrace/scoring/builtin.py b/tooltrace/scoring/builtin.py index acebb04..4b6c0e5 100644 --- a/tooltrace/scoring/builtin.py +++ b/tooltrace/scoring/builtin.py @@ -150,6 +150,41 @@ def _command_exit(params: dict[str, object], workspace: Path) -> ScorerOutcome: return ScorerOutcome(1.0 if ok else 0.0, detail) +def score_pytest_output(output: str, min_ratio: float) -> ScorerOutcome: + """Turn a pytest summary line into a score. Pure, so it can be tested. + + This calculation used to live inside `_tests_pass`, welded to a + `subprocess.run`, which meant no test could reach it without executing a + real pytest in a temporary workspace -- so no test did. Mutation testing + found the consequence: every operator here survived. `passed + failed + + errors` could become a subtraction, `passed / total` a multiplication, and + the `>=` threshold could invert, with the whole suite still green. For a + benchmarking tool that is the measurement itself going unchecked. + + Extracted rather than merely tested: the reason it was untested is that it + was unreachable. + """ + + def count(pattern: str) -> int: + m = re.search(pattern, output) + return int(m.group(1)) if m else 0 + + passed, failed, errors = ( + count(r"(\d+) passed"), + count(r"(\d+) failed"), + count(r"(\d+) error"), + ) + total = passed + failed + errors + ratio = passed / total if total else 0.0 + # A run with errors never scores full marks even at a satisfied ratio: + # a collection error means the suite did not fully execute. + score = 1.0 if ratio >= min_ratio and errors == 0 else round(ratio, 4) + return ScorerOutcome( + score, + f"passed={passed} failed={failed} errors={errors} ratio={ratio:.2f}", + ) + + @register_scorer("tests_pass") def _tests_pass(params: dict[str, object], workspace: Path) -> ScorerOutcome: target = str(params.get("path", ".")) @@ -177,22 +212,7 @@ def _tests_pass(params: dict[str, object], workspace: Path) -> ScorerOutcome: return ScorerOutcome(0.0, "test run timed out") output = proc.stdout + proc.stderr - def count(pattern: str) -> int: - m = re.search(pattern, output) - return int(m.group(1)) if m else 0 - - passed, failed, errors = ( - count(r"(\d+) passed"), - count(r"(\d+) failed"), - count(r"(\d+) error"), - ) - total = passed + failed + errors - ratio = passed / total if total else 0.0 - score = 1.0 if ratio >= min_ratio and errors == 0 else round(ratio, 4) - return ScorerOutcome( - score, - f"passed={passed} failed={failed} errors={errors} ratio={ratio:.2f}", - ) + return score_pytest_output(output, min_ratio) @register_scorer("git_diff") @@ -222,7 +242,16 @@ def _git_diff(params: dict[str, object], workspace: Path) -> ScorerOutcome: if needle in diff_text: problems.append(f"diff contains forbidden {needle!r}") if max_changed is not None: - changed = {line.split()[2] for line in diff_text.splitlines() if line.startswith("+++ b/")} + # `+++ b/path` has two whitespace-separated fields, so the old + # `line.split()[2]` raised IndexError on every real diff -- the + # max_changed_files constraint could never return a score, only crash. + # No shipped task used it, which is why nothing noticed; a boundary + # test written for a surviving mutant is what surfaced it. + changed = { + line.split(maxsplit=1)[1].removeprefix("b/") + for line in diff_text.splitlines() + if line.startswith("+++ b/") + } limit = int(max_changed) if isinstance(max_changed, (int, float)) else 10**9 if len(changed) > limit: problems.append(f"changed files {len(changed)} > {max_changed}")