From 8423a79196288722061e1765252353fd74c0b056 Mon Sep 17 00:00:00 2001 From: Eric Minish Date: Fri, 30 Oct 2026 13:04:48 -0400 Subject: [PATCH 1/2] fix: tgtrace writes its trace atomically; verify refuses a missing or truncated trace (#108) A truncated trace raised an uncaught JSONDecodeError out of verify.run, so the CLI exited 1 (= tests failed); a missing trace read as {}, so every selected journey was reported uncovered (exit 3). Both are a broken run, not a verdict. - tgtrace writes .tmp. and os.replace()s it; a failed write removes the temp file, so the target is absent or whole. - verify.run returns an error (exit 2, REFUSED) for a missing or unreadable trace; pytest_adapter.run refuses a truncated one too. - README and --help say exit 2 also covers "no readable trace". --- README.md | 2 +- harness/plugin/tgtrace.py | 16 +++- testgraph/pytest_adapter.py | 8 +- testgraph/verify.py | 21 +++++- tests/test_verify.py | 142 ++++++++++++++++++++++++++++++++++++ 5 files changed, 181 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index dce00da..877bde4 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ instead of being folded into PASS. python3 -m testgraph.verify --repo . --base origin/main -- tests/ -q # exit 0 every selected journey ran and passed # exit 1 a journey's tests failed -# exit 2 refused: the index can't be trusted +# exit 2 refused: the index can't be trusted, or the run left no readable trace # exit 3 a selected journey has no journey-level test, or the selection is UNKNOWN — named, not hidden ``` diff --git a/harness/plugin/tgtrace.py b/harness/plugin/tgtrace.py index 9c914c8..e2ee3fe 100644 --- a/harness/plugin/tgtrace.py +++ b/harness/plugin/tgtrace.py @@ -183,8 +183,20 @@ def pytest_unconfigure(config): directory = os.path.dirname(os.path.abspath(_out_path)) if directory: os.makedirs(directory, exist_ok=True) - with open(_out_path, "w") as f: - json.dump(payload, f, indent=1, sort_keys=True) + # Write beside the target and rename (#108): a run killed mid-dump used to + # leave a truncated file where a reader expects a whole one. os.replace is + # atomic on the same filesystem, so the target is either absent or complete. + tmp = f"{_out_path}.tmp.{os.getpid()}" + try: + with open(tmp, "w") as f: + json.dump(payload, f, indent=1, sort_keys=True) + os.replace(tmp, _out_path) + except BaseException: + try: + os.remove(tmp) + except OSError: + pass + raise def pytest_runtest_call(item): diff --git a/testgraph/pytest_adapter.py b/testgraph/pytest_adapter.py index 8862ef0..2c56f61 100644 --- a/testgraph/pytest_adapter.py +++ b/testgraph/pytest_adapter.py @@ -320,8 +320,12 @@ def run(repo, registry_path=None, db_path=None, pytest_args=(), commit=None, ) import json - with open(trace_path) as f: - trace = json.load(f) + try: + with open(trace_path) as f: + trace = json.load(f) + except (OSError, ValueError) as exc: + # Same refusal as verify (#108): a cut-short run is not a verdict. + return None, f"the tgtrace trace is truncated or not JSON ({exc})" if trace_out: with open(trace_out, "w") as f: json.dump(trace, f, indent=1, sort_keys=True) diff --git a/testgraph/verify.py b/testgraph/verify.py index 52e2887..9413f5f 100644 --- a/testgraph/verify.py +++ b/testgraph/verify.py @@ -214,10 +214,24 @@ def run(repo, base, head="HEAD", registry_path=None, db_path=None, proc = runner() import json - trace = {} - if os.path.isfile(trace_path): + # A missing or unreadable trace is a broken run, not a verdict + # (#108). Read as `{}` it reported every selected journey as having + # no journey-level test (exit 3); a truncated one raised out of here + # and exited 1, the code for "tests failed". + if not os.path.isfile(trace_path): + return None, ( + f"pytest exited {getattr(proc, 'returncode', '?')} and tgtrace " + f"wrote no trace — the run was killed, or the plugin did not " + f"load; no verdict was recorded" + ) + try: with open(trace_path) as fh: trace = json.load(fh) + except (OSError, ValueError) as exc: + return None, ( + f"the tgtrace trace is truncated or not JSON ({exc}) — the run " + f"was cut short; no verdict was recorded" + ) verdicts = {} if os.path.isfile(junit_path): verdicts = pa.parse_junit(junit_path) @@ -383,7 +397,8 @@ def main(argv=None): prog="testgraph.verify", description="run the declared tests for the journeys a diff endangers", epilog="Exit: 0 every selected journey ran and passed, 1 a journey " - "failed, 2 refused (untrustworthy index), 3 incomplete — at " + "failed, 2 refused (untrustworthy index, or no readable trace), " + "3 incomplete — at " "least one selected journey had no passing declared test, or " "the selection was NONE but UNKNOWN.\n" "Put pytest's own flags after `--`.", diff --git a/tests/test_verify.py b/tests/test_verify.py index 226faee..553d96d 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -9,6 +9,9 @@ run printed `NO JOURNEY-LEVEL TEST: J1, J5, J6, J7` and exited 0 — four journeys unverified behind a green from the other four. """ +import contextlib +import io +import json import os import shutil import sqlite3 @@ -353,6 +356,145 @@ def __getattr__(self, n): self.assertEqual(len(closed), len(opened)) +class UnreadableTraceTests(unittest.TestCase): + """Issue #108: a truncated trace made `json.load` raise, and the uncaught + exception exited 1 — the code for "tests failed". A missing trace read as + an empty one, so every selected journey was reported as having no + journey-level test (exit 3). Both are a broken run, not a verdict.""" + + def setUp(self): + self.tmp = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, self.tmp, ignore_errors=True) + self.db = _db_on_disk(self.tmp, build_fixture()) + self.registry = _registry_file(self.tmp, { + "J1": {"name": "ok", "entries": [{"name": "handler_a", "file": "app/svc.py"}]}, + }) + self.repo, run = _git_repo(self.tmp, {"app/svc.py": 22}) + full = os.path.join(self.repo, "app", "svc.py") + with open(full) as fh: + lines = fh.readlines() + lines[11] = "changed = 1\n" # inside handler_a -> J1 + with open(full, "w") as fh: + fh.writelines(lines) + run("git", "add", "-A") + run("git", "commit", "-qm", "touch") + self.work = os.path.join(self.tmp, "work") + os.makedirs(self.work) + + def _run(self, trace_text): + work = self.work + + class FixedDir: + def __enter__(self): + return work + + def __exit__(self, *exc): + return False + + def runner(): + if trace_text is not None: + with open(os.path.join(work, "trace.json"), "w") as fh: + fh.write(trace_text) + return types.SimpleNamespace(returncode=0) + + with unittest.mock.patch.object(verify.tempfile, "TemporaryDirectory", + lambda: FixedDir()): + return verify.run(self.repo, "HEAD~1", "HEAD", registry_path=self.registry, + db_path=self.db, _runner=runner, append=lambda r: True) + + def test_a_truncated_trace_is_refused_not_a_crash(self): + summary, err = self._run('{"tests": {"t::a": [["app/svc.py", "hand') + self.assertIsNone(summary) + self.assertIn("trace", err) + self.assertIn("truncated or not JSON", err) + + def test_a_missing_trace_is_refused_not_uncovered(self): + summary, err = self._run(None) + self.assertIsNone(summary) + self.assertIn("wrote no trace", err) + + def test_main_exits_refused(self): + with unittest.mock.patch.object(verify, "run", return_value=(None, "trace broke")), \ + contextlib.redirect_stdout(io.StringIO()): + rc = verify.main(["--repo", self.repo, "--base", "HEAD~1"]) + self.assertEqual(rc, verify.EXIT_REFUSED) + + +class AtomicTraceWriteTests(unittest.TestCase): + """#108: the trace was written in place, so a run killed mid-write left a + truncated file where a reader expects a whole one.""" + + def test_a_write_that_dies_leaves_no_partial_file(self): + from harness.plugin import tgtrace + tmp = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, tmp, ignore_errors=True) + out = os.path.join(tmp, "trace.json") + + seen_target = [] + + def dying_dump(obj, fh, **kw): + fh.write('{"tests": {') + # a SIGKILL here runs no cleanup: the target must not exist yet + seen_target.append(os.path.exists(out)) + raise KeyboardInterrupt + + with unittest.mock.patch.object(tgtrace, "_out_path", out), \ + unittest.mock.patch.object(tgtrace, "_stop", lambda: None), \ + unittest.mock.patch.object(tgtrace.json, "dump", dying_dump): + with self.assertRaises(KeyboardInterrupt): + tgtrace.pytest_unconfigure(None) + self.assertFalse(os.path.exists(out)) + self.assertEqual(os.listdir(tmp), []) + self.assertEqual(seen_target, [False]) + + def test_a_whole_write_lands(self): + from harness.plugin import tgtrace + tmp = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, tmp, ignore_errors=True) + out = os.path.join(tmp, "trace.json") + with unittest.mock.patch.object(tgtrace, "_out_path", out), \ + unittest.mock.patch.object(tgtrace, "_stop", lambda: None): + tgtrace.pytest_unconfigure(None) + with open(out) as fh: + self.assertIn("tests", json.load(fh)) + self.assertEqual(os.listdir(tmp), ["trace.json"]) + + +class AdapterTruncatedTraceTests(unittest.TestCase): + def test_the_adapter_refuses_a_truncated_trace(self): + from testgraph import pytest_adapter as pa + tmp = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, tmp, ignore_errors=True) + db = _db_on_disk(tmp, build_fixture()) + registry = _registry_file(tmp, { + "J1": {"name": "ok", "entries": [{"name": "handler_a", "file": "app/svc.py"}]}, + }) + repo, _ = _git_repo(tmp, {"app/svc.py": 22}) + work = os.path.join(tmp, "work") + os.makedirs(work) + + class FixedDir: + def __enter__(self): + return work + + def __exit__(self, *exc): + return False + + def runner(): + with open(os.path.join(work, "junit.xml"), "w") as fh: + fh.write("") + with open(os.path.join(work, "trace.json"), "w") as fh: + fh.write('{"tests": {') + return types.SimpleNamespace(returncode=0, stderr="") + + with unittest.mock.patch.object(pa.tempfile, "TemporaryDirectory", + lambda: FixedDir()): + summary, err = pa.run(repo, registry_path=registry, db_path=db, + _runner=runner, append=lambda r: True) + self.assertIsNone(summary) + self.assertIn("truncated or not JSON", err) + + class SummaryShapeTests(unittest.TestCase): """Every exit from `run` carries the same keys. From daaef68fefcdf27b4e6b5310825b8d7ba20effe0 Mon Sep 17 00:00:00 2001 From: Eric Minish Date: Fri, 30 Oct 2026 13:11:48 -0400 Subject: [PATCH 2/2] fix: refuse a trace that is not an object, and keep pytest's stderr in the refusal (#108 review) - H1: `null` / `[]` / a string parse as JSON, then crashed validate() with an AttributeError (exit 1 again). verify and pytest_adapter refuse a non-object trace like a truncated one. - H3: the missing-trace refusal now carries the tail of pytest's stderr, which names the real cause (a bad flag, tgtrace's xdist refusal). - ConnectionClosedTests writes a whole (empty) trace, so it covers the validate/ledger path again instead of stopping at the new refusal. --- testgraph/pytest_adapter.py | 3 ++ testgraph/verify.py | 9 +++++ tests/test_verify.py | 71 +++++++++++++++++++++++++++++++++---- 3 files changed, 77 insertions(+), 6 deletions(-) diff --git a/testgraph/pytest_adapter.py b/testgraph/pytest_adapter.py index 2c56f61..8dff904 100644 --- a/testgraph/pytest_adapter.py +++ b/testgraph/pytest_adapter.py @@ -326,6 +326,9 @@ def run(repo, registry_path=None, db_path=None, pytest_args=(), commit=None, except (OSError, ValueError) as exc: # Same refusal as verify (#108): a cut-short run is not a verdict. return None, f"the tgtrace trace is truncated or not JSON ({exc})" + if not isinstance(trace, dict): + return None, (f"the trace file is not a tgtrace payload (a JSON " + f"{type(trace).__name__}, not an object)") if trace_out: with open(trace_out, "w") as f: json.dump(trace, f, indent=1, sort_keys=True) diff --git a/testgraph/verify.py b/testgraph/verify.py index 9413f5f..7da8b70 100644 --- a/testgraph/verify.py +++ b/testgraph/verify.py @@ -219,10 +219,14 @@ def run(repo, base, head="HEAD", registry_path=None, db_path=None, # no journey-level test (exit 3); a truncated one raised out of here # and exited 1, the code for "tests failed". if not os.path.isfile(trace_path): + # pytest's own stderr usually names the cause (a bad flag, the + # plugin's xdist refusal); the guess below is only a fallback. + tail = (getattr(proc, "stderr", "") or "").strip()[-400:] return None, ( f"pytest exited {getattr(proc, 'returncode', '?')} and tgtrace " f"wrote no trace — the run was killed, or the plugin did not " f"load; no verdict was recorded" + + (f"\n pytest stderr (tail): {tail}" if tail else "") ) try: with open(trace_path) as fh: @@ -232,6 +236,11 @@ def run(repo, base, head="HEAD", registry_path=None, db_path=None, f"the tgtrace trace is truncated or not JSON ({exc}) — the run " f"was cut short; no verdict was recorded" ) + if not isinstance(trace, dict): + return None, ( + f"the trace file is not a tgtrace payload (a JSON " + f"{type(trace).__name__}, not an object); no verdict was recorded" + ) verdicts = {} if os.path.isfile(junit_path): verdicts = pa.parse_junit(junit_path) diff --git a/tests/test_verify.py b/tests/test_verify.py index 553d96d..da95f61 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -346,11 +346,33 @@ def close(self): def __getattr__(self, n): return getattr(self._c, n) + # A whole (empty) trace, so the run goes through validate and the + # ledger section — the part a leak would hide in. With no trace it now + # stops at the #108 refusal and would test nothing past it. + work = os.path.join(tmp, "work") + os.makedirs(work) + + class FixedDir: + def __enter__(self): + return work + + def __exit__(self, *exc): + return False + + def runner(): + with open(os.path.join(work, "trace.json"), "w") as fh: + fh.write('{"tests": {}, "declared": {}}') + return types.SimpleNamespace(returncode=0) + with unittest.mock.patch.object( - verify.dbmod, "connect", lambda p: Proxy(real(p))): - verify.run(repo, "HEAD~1", "HEAD", registry_path=registry, db_path=db, - _runner=lambda: types.SimpleNamespace(returncode=0), - append=lambda row: True) + verify.dbmod, "connect", lambda p: Proxy(real(p))), \ + unittest.mock.patch.object(verify.tempfile, "TemporaryDirectory", + lambda: FixedDir()): + summary, err = verify.run(repo, "HEAD~1", "HEAD", registry_path=registry, + db_path=db, _runner=runner, + append=lambda row: True) + self.assertIsNone(err) + self.assertIsNotNone(summary["marker_expression"]) # select opens one and verify opens one; both must be closed self.assertEqual(len(opened), 2) self.assertEqual(len(closed), len(opened)) @@ -408,6 +430,36 @@ def test_a_truncated_trace_is_refused_not_a_crash(self): self.assertIn("trace", err) self.assertIn("truncated or not JSON", err) + def test_a_trace_that_is_not_an_object_is_refused(self): + """Review H1: `null` or `[]` parse, then crashed validate() with an + AttributeError — exit 1 again.""" + for text in ("null", "[]", '"x"'): + with self.subTest(text): + summary, err = self._run(text) + self.assertIsNone(summary) + self.assertIn("not a tgtrace payload", err) + + def test_the_refusal_carries_pytest_stderr(self): + """Review H3: the real cause (an xdist refusal, a bad flag) was in + pytest's stderr, which was dropped.""" + work = self.work + + class FixedDir: + def __enter__(self): + return work + + def __exit__(self, *exc): + return False + + runner = lambda: types.SimpleNamespace( + returncode=4, stderr="ERROR: usage: unrecognized arguments: --bogus") + with unittest.mock.patch.object(verify.tempfile, "TemporaryDirectory", + lambda: FixedDir()): + summary, err = verify.run(self.repo, "HEAD~1", "HEAD", + registry_path=self.registry, db_path=self.db, + _runner=runner, append=lambda r: True) + self.assertIn("unrecognized arguments: --bogus", err) + def test_a_missing_trace_is_refused_not_uncovered(self): summary, err = self._run(None) self.assertIsNone(summary) @@ -484,7 +536,7 @@ def runner(): with open(os.path.join(work, "junit.xml"), "w") as fh: fh.write("") with open(os.path.join(work, "trace.json"), "w") as fh: - fh.write('{"tests": {') + fh.write(self.trace_text) return types.SimpleNamespace(returncode=0, stderr="") with unittest.mock.patch.object(pa.tempfile, "TemporaryDirectory", @@ -492,7 +544,14 @@ def runner(): summary, err = pa.run(repo, registry_path=registry, db_path=db, _runner=runner, append=lambda r: True) self.assertIsNone(summary) - self.assertIn("truncated or not JSON", err) + self.assertIn(self.expect, err) + + trace_text, expect = '{"tests": {', "truncated or not JSON" + + +class AdapterNonObjectTraceTests(AdapterTruncatedTraceTests): + """Review H1, adapter side: `[]` crashed at `trace.get("tests")`.""" + trace_text, expect = "[]", "not a tgtrace payload" class SummaryShapeTests(unittest.TestCase):