diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index b6b49c75f..526e505e2 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -578,8 +578,15 @@ if "/status" in url: print("502 Bad Gateway") sys.exit(0) if mode.startswith("existing:"): + # DECOY contexts either side, because real heads carry several (a live head showed 7 rows, + # the first an unrelated CI context). Their status is deliberately `pending`, NOT `success`: + # a `success` decoy triggers the very same short-circuit as a real verdict, so dropping + # `select(.context == $c)` produced an identical outcome and the mutation survived. With + # `pending` decoys, mis-selecting means no short-circuit — the job posts, and the test sees it. print(json.dumps({"statuses": [ - {"context": "review-verdict/h10", "status": mode.split(":", 1)[1]}]})) + {"context": "Build & test (.NET)", "status": "pending"}, + {"context": "review-verdict/h10", "status": mode.split(":", 1)[1]}, + {"context": "Functional E2E", "status": "pending"}]})) sys.exit(0) print(json.dumps({"statuses": []})) sys.exit(0) @@ -589,13 +596,18 @@ print("{}") def _run_classify(tmp_path, enum_stub: str | None, author: str = "timothy", - status_mode: str = "none"): + status_mode: str = "none", jq16: bool = False): """Execute the workflow's classify `run:` block with a stubbed enumeration script. Returns the status payload the job POSTed, or None if it posted nothing. """ bindir = tmp_path / "bin"; bindir.mkdir() curl = bindir / "curl"; curl.write_text(WORKFLOW_STUB_CURL); curl.chmod(0o755) + if jq16: + # Reproduce the runner's jq 1.6 (`-e` over EMPTY input exits 0, not 4). The shim is the one + # already used for pr-changed-files.sh in this file, so its fidelity is covered by that + # suite's own verify-the-verifier test. + jq = bindir / "jq"; jq.write_text(_JQ16_SHIM); jq.chmod(0o755) scripts = tmp_path / "scripts"; scripts.mkdir() if enum_stub is not None: enum = scripts / "pr-changed-files.sh" @@ -746,8 +758,22 @@ def test_a_GARBAGE_status_response_posts_NOTHING(tmp_path): assert posted is None +# Two mutations to this read are NOT covered, and both are behaviourally equivalent rather than gaps: +# * `first` -> `last`: `select(.context == $c)` yields exactly ONE row, because the job reads the +# COMBINED status endpoint, which returns latest-per-context by contract. A fixture with two +# `review-verdict/h10` rows would be testing something the API does not produce. +# * `test_a_GARBAGE_status_response_posts_NOTHING` does not, on its own, defend the type guard: +# with the guard gone, the following `jq -r` fails on non-JSON and `set -e` kills the job anyway. +# That is fail-closed by REDUNDANCY. The guard's own behaviour is pinned by the jq-1.6 test below, +# which is where it actually matters. + @pytest.mark.parametrize("existing", ["success", "failure"]) -def test_an_existing_verdict_on_this_head_is_NEVER_overwritten(tmp_path, existing): +@pytest.mark.parametrize("paths,author", [ + (("ErsatzTV/Program.cs",), "timothy"), # non-exempt: only the short-circuit can stop it + (("docs/a.md",), "timothy"), # docs-only EXEMPT + (("Directory.Packages.props",), "renovate"), # bot EXEMPT +]) +def test_an_existing_verdict_on_this_head_is_NEVER_overwritten(tmp_path, existing, paths, author): """A human verdict for this exact head may already exist — the reviewer ran post-review-verdict.sh before this job finished, or the job re-ran. Re-posting would un-approve a reviewed head, or (worse) approve one a human marked BLOCKED. @@ -755,10 +781,11 @@ def test_an_existing_verdict_on_this_head_is_NEVER_overwritten(tmp_path, existin `failure` is the sharp case: that is a human saying NO, and an exemption posted over it would turn a rejection into a merge. """ - posted, r = _run_classify(tmp_path, _emitting("ErsatzTV/Program.cs"), + posted, r = _run_classify(tmp_path, _emitting(*paths), author=author, status_mode=f"existing:{existing}") assert r.returncode == 0, r.stderr - assert posted is None, f"overwrote an existing '{existing}' verdict on this head" + assert posted is None, ( + f"overwrote an existing '{existing}' verdict on this head with {paths} as {author}") # --- The `count -eq 0` guard, on the path where it is the ONLY guard --------------------------- @@ -799,25 +826,33 @@ def test_DOCS_ONLY_anchors_the_markdown_extension(tmp_path): assert posted["state"] == "pending", "a .mdx file was accepted as docs-only" -def test_the_status_read_checks_EMPTINESS_in_SHELL_before_invoking_jq(): - """Structural, and deliberately so — no behavioural test can catch this on a dev machine. +def test_a_transport_failure_under_jq_1_6_STILL_posts_nothing(tmp_path): + """The ersatztv#647 fail-open, tested where it actually lives: jq 1.6. - Deleting the `[ -z "${statusjson//[[:space:]]/}" ]` half of the guard leaves the empty case to - `jq -e`'s exit status, which is 4 on jq >= 1.7 (guard still fires, and - `test_a_transport_failure_on_the_STATUS_READ_posts_NOTHING` still passes on a Mac with jq 1.8) - but **0 on jq 1.6, which is what the runner ships**. There the job sails past the guard and - posts over a possibly-existing human verdict. That is literally ersatztv#647, and the reason - `ci.jq-version-contract` exists. + The shell emptiness check exists because `jq -e` over EMPTY input exits 4 on jq >= 1.7 but **0 on + jq 1.6, which is what the runner ships**. Remove that check and, on a dev Mac's 1.8, the guard + still fires and every test stays green — the bug is invisible locally. - So the behavioural test catches this mutation only when the suite runs under 1.6 — in CI, not - locally. This closes the local gap by pinning the construct. + An earlier version of this pinned the construct STRUCTURALLY instead, on the stated grounds that + "no behavioural test can catch this on a dev machine". That was wrong: this file already imports + `_JQ16_SHIM` for `pr-changed-files.sh`, so the runner's quirk is reproducible here. The structural + version was also weaker than it looked — it stripped only FULL-LINE comments, so leaving the + literal as a trailing comment on the surviving `if` satisfied it while the real guard was gone. - COMMENTS ARE STRIPPED FIRST. A raw substring search over the step text is satisfiable by moving - the guard into a comment while deleting the real one — the same "a comment mentioning it is not - a call" vacuity this file already fixed for the enumeration drift guard. + This test is strictly stronger: it catches that mutant, needs no comment-stripping, and fails for + the right reason. Verified by mutation under both jq versions. """ - body = _classify_step()["run"] - code = "\n".join(ln for ln in body.splitlines() if not ln.lstrip().startswith("#")) - assert '[ -z "${statusjson//[[:space:]]/}" ]' in code, ( - "the status read must check emptiness in SHELL before jq; leaving it to `jq -e`'s exit " - "status is fail-open on the runner's jq 1.6 (ersatztv#647)") + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), + status_mode="transport-error", jq16=True) + assert r.returncode != 0, ( + "under jq 1.6 an unreadable status read must still fail the job — this is the exact " + "ersatztv#647 fail-open") + assert posted is None, "nothing may be posted when the existing verdict state is unknown" + + +def test_DOCS_ONLY_anchors_the_START_of_the_path_too(tmp_path): + r"""`^(docs/|...)` must match only at the start. Losing the `^` exempts `ErsatzTV/docs/Evil.cs`, + which is a C# file — fail-OPEN, and the sibling of the `$` case above.""" + posted, _ = _run_classify(tmp_path, _emitting("ErsatzTV/docs/Evil.cs")) + assert posted is not None + assert posted["state"] == "pending", "a path merely CONTAINING docs/ was accepted as docs-only"