From 13182d5a6e8ca8e322fe7cac6d3f4bd002884e87 Mon Sep 17 00:00:00 2001 From: Eric Minish Date: Thu, 10 Sep 2026 07:07:23 -0400 Subject: [PATCH] fix(verify): one summary shape, so a NONE answer still names its repo `run` had three exits -- refused, nothing-selected, and the full run -- each building its dict independently, and the two short ones omitted `repo` and `commit`. So a NONE answer rendered `testgraph verify[?]`. Cosmetic in the text; not cosmetic in --json, where a consumer reading summary["repo"] gets a KeyError on exactly the quiet runs it is most likely to be aggregating, and where the key set silently changes depending on whether the selector found anything. Fixed by routing all three through one `_summary` builder rather than adding the missing key to one dict, which would have left the shapes free to drift again on the next exit added. The refused renderer now names the repo like every other line does. 420 tests (+4), pinning that every exit carries the same keys and that a NONE answer renders the repo name rather than `[?]`. --- testgraph/verify.py | 63 +++++++++++++++++++++++++++++++------------- tests/test_verify.py | 36 +++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 19 deletions(-) diff --git a/testgraph/verify.py b/testgraph/verify.py index 2e2e86e..97d792b 100644 --- a/testgraph/verify.py +++ b/testgraph/verify.py @@ -113,6 +113,33 @@ def validate(conn, registry, trace, verdicts): return credited, unvalidated, per_journey +def _summary(repo, head, selection, **extra): + """Every exit from `run` goes through here. + + The three returns — refused, nothing-selected, and the full run — used to + build their dicts independently, and the short two omitted `repo`, so the + report rendered `testgraph verify[?]` for a NONE answer. Cosmetic in the + text; not cosmetic in `--json`, where a consumer reading `summary["repo"]` + gets a KeyError on exactly the quiet runs it is most likely to be + aggregating. A shared shape is the fix, not a fourth copy of the key.""" + base = { + "repo": reg.repo_name(repo), + "commit": ledger.resolve_commit(repo, head), + "selection": selection, + "journeys": [], + "marker_expression": None, + "collected": 0, + "credited": {}, + "unvalidated": [], + "per_journey": {}, + "uncovered": [], + "rows_written": 0, + "pytest_exit": None, + } + base.update(extra) + return base + + def run(repo, base, head="HEAD", registry_path=None, db_path=None, pytest_args=(), append=None, _runner=None): """Select, run the declared tests, validate the declarations, report. @@ -137,7 +164,7 @@ def run(repo, base, head="HEAD", registry_path=None, db_path=None, if selection.get("status") == "BLOCKED" or selection.get("blocking"): # The index is not trustworthy. Running tests off it would produce a # verdict about a journey set chosen from a graph the guard refused. - return {"selection": selection, "refused": True}, None + return _summary(repo, head, selection, refused=True), None # `select` returns journey dicts keyed `id` (with name/rank/confidence); # the plain-string form is what a caller passing a hand-built selection uses. @@ -147,9 +174,7 @@ def run(repo, base, head="HEAD", registry_path=None, db_path=None, conn = dbmod.connect(db_path) if not journeys: - return {"selection": selection, "journeys": [], "collected": 0, - "credited": {}, "unvalidated": [], "per_journey": {}, - "uncovered": [], "rows_written": 0, "pytest_exit": None}, None + return _summary(repo, head, selection), None expr = marker_expression(journeys) with tempfile.TemporaryDirectory() as tmp: @@ -191,20 +216,19 @@ def run(repo, base, head="HEAD", registry_path=None, db_path=None, if append(row): written.append(row) - return { - "selection": selection, - "journeys": journeys, - "marker_expression": expr, - "collected": len(verdicts), - "credited": credited, - "unvalidated": unvalidated, - "per_journey": {k: sorted(v) for k, v in per_journey.items()}, - "uncovered": uncovered, - "rows_written": len(written), - "pytest_exit": getattr(proc, "returncode", None), - "commit": sha, - "repo": reg.repo_name(repo), - }, None + return _summary( + repo, head, selection, + journeys=journeys, + marker_expression=expr, + collected=len(verdicts), + credited=credited, + unvalidated=unvalidated, + per_journey={k: sorted(v) for k, v in per_journey.items()}, + uncovered=uncovered, + rows_written=len(written), + pytest_exit=getattr(proc, "returncode", None), + commit=sha, + ), None def exit_code(summary): @@ -232,7 +256,8 @@ def exit_code(summary): def render(summary, registry=None): if summary.get("refused"): - lines = ["testgraph verify: REFUSED — the index is not trustworthy"] + lines = [f"testgraph verify[{summary.get('repo', '?')}]: REFUSED — " + f"the index is not trustworthy"] lines += [f" BLOCKED: {b}" for b in summary["selection"].get("blocking", [])] lines.append(" No tests were run. A verdict off a graph the guard " diff --git a/tests/test_verify.py b/tests/test_verify.py index 3ef38b6..a157b52 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -196,6 +196,41 @@ def test_a_refused_run_has_its_own_code(self): ) +class SummaryShapeTests(unittest.TestCase): + """Every exit from `run` carries the same keys. + + The three returns used to build their dicts independently and the two short + ones omitted `repo`, so a NONE answer rendered `testgraph verify[?]`. + Cosmetic in the text, not in `--json`: a consumer reading `summary["repo"]` + got a KeyError on exactly the quiet runs it is most likely aggregating.""" + + KEYS = {"repo", "commit", "selection", "journeys", "marker_expression", + "collected", "credited", "unvalidated", "per_journey", "uncovered", + "rows_written", "pytest_exit"} + + def test_the_empty_summary_carries_every_key(self): + got = verify._summary("/nonexistent", "HEAD", {"base": "a", "head": "b"}) + self.assertTrue(self.KEYS <= set(got), self.KEYS - set(got)) + + def test_a_refused_summary_carries_them_too(self): + got = verify._summary("/nonexistent", "HEAD", {}, refused=True) + self.assertTrue(self.KEYS <= set(got), self.KEYS - set(got)) + self.assertTrue(got["refused"]) + + def test_extras_override_the_defaults(self): + got = verify._summary("/nonexistent", "HEAD", {}, journeys=["J2"], + collected=3) + self.assertEqual(got["journeys"], ["J2"]) + self.assertEqual(got["collected"], 3) + + def test_the_repo_name_reaches_a_none_answer(self): + summary = verify._summary("/tmp/some-repo", "HEAD", + {"base": "a", "head": "b", "warnings": []}) + out = verify.render(summary, REGISTRY) + self.assertIn("some-repo", out) + self.assertNotIn("[?]", out) + + class RenderTests(unittest.TestCase): def test_uncovered_journeys_are_named_loudly_and_denied_a_pass(self): out = verify.render({ @@ -217,6 +252,7 @@ def test_uncovered_journeys_are_named_loudly_and_denied_a_pass(self): def test_a_refused_run_says_no_tests_were_run(self): out = verify.render({ "refused": True, + "repo": "demo", "selection": {"blocking": ["index likely corrupt"]}, }) self.assertIn("REFUSED", out)