From 33810405a84e68a7c704eb3c49bcb937a531b3b2 Mon Sep 17 00:00:00 2001 From: JC-000 <3798556+JC-000@users.noreply.github.com> Date: Mon, 31 Aug 2026 11:53:16 -0500 Subject: [PATCH 1/5] skip policy: an involuntary skip is a failure (#178a, rig half) Add tools/_skip_policy.py and wire the in-scope entry points to it. Three lanes, deliberately different calls, because the REMEDY decides the verdict: not_applicable() VOLUNTARY -- this host or configuration can never run this rig and another rig owns the coverage, or the operator declined an opt-in rig. Nothing is lost, so exit 0 -- but as a named verdict, never a bare `return 0`. Remedy: "go use the other rig", or "opt in". cannot_run() INVOLUNTARY -- this IS the target platform/config and it still could not run: no toolchain, no PRG, no hardware. A real coverage hole, so exit 2 (distinct from 1, "a check ran and failed"). Remedy: "install something". cannot_run(opt_out_env=None) -- the two things no environment variable may silence: a failed build, and contention. Measured before the change, with build/ absent: tests/rig_phase{1_dhcp,2_http,3_https,3_https_1mhz}.py exit 0 at the missing-prerequisite site AND at `return _skip("c64-https.prg could not be built")` -- a failed `make` laundered into a pass. tests/rig_vice_https_macos.py exit 0 with the rig down; and `SystemExit("build failed")` exited 1, making a broken build indistinguishable from a real handshake failure. tools/test_p384_symbols.py exit 0 on BACKEND != uci (voluntary, but silent). tools/test_ecdsa_p384_kat.py --u64 with no U64_HOST printed "U64: 0/0 passed" and OVERALL: PASS, exit 0. BEHAVIOUR CHANGE worth stating rather than discovering: that last one is now refused UP FRONT, before the multi-hour VICE lane, instead of at the verdict. `--u64` without U64_HOST fails in ~0.1 s. Platform is asked FIRST, and it is a voluntary skip. The four bridge rigs are Linux-only; on Darwin check_prerequisites() reports "ip not on PATH; iptables not on PATH", which is not installable there, so routing that to cannot_run() would make four rigs a permanent red on the project's primary platform -- with the global opt-out as the only remedy, i.e. the policy gets disabled everywhere. https_e2e gains a separate platform_supported() predicate rather than partitioning check_prerequisites(): that function returns a flat list of strings whose entries mean different things on different platforms, and it is shared by four rigs and shadowed by a same-named function in tools/test_http_integration.py. Its contract is unchanged. rig_vice_https_macos.py gets the symmetric lift. Contention is a third category. In rig_vice_https_macos.py it is split out of _rig_check(); in rig_phase3_https_1mhz.py the port-443 check is the same thing. Both exit 2 with no opt-out: a lane that silences contention goes green exactly when it collides, and unlike a missing tool it clears on its own. It is evaluated AFTER `problems`, because contention on a rig that does not exist is a meaningless headline -- and, being non-opt-out- able, would otherwise leave a no-rig CI lane red with no recourse. The contention detector is narrowed accordingly. `pgrep -fl "ethernetioif feth0"` matched the full command line of every process against an unanchored pattern, so a grep, an editor, or a driver script carrying that text matched too (demonstrated: a plain `python3 -c ... "ethernetioif feth0"` matches). It is now `pgrep -x x64sc` plus an argv check, so it matches only a process whose executable is x64sc and whose arguments name feth0. rig_phase3_https_1mhz.py's two pre-flight gates predate this change but contradicted the contract it documents: an unset VICE_HTTPS_OK_TO_RUN is the operator declining an opt-in rig (now exit 0, named verdict), and a held port 443 is contention (exit 2, no opt-out). The platform gate moved ahead of both, so a host that can never run the rig is never told to opt in and never trips the port check. The opt-out requires the literal "1". Bare truthiness meant C64_ALLOW_SKIP=0 ENABLED it, so setting 0 to close the hatch disabled the whole policy; every other gate in this repo (C64_SKIP_BUILD, VICE_HTTPS_OK_TO_RUN, ...) compares to "1" and this one now does too. require() also fails closed when the opt-out is set but pytest is absent, rather than returning into a test body whose prerequisite is missing. Two test modules, both pinned in pytest.ini testpaths: tools/test_skip_policy.py 15 cases -- the "1" comparison across 0 / false / empty / yes / true / 2 / " 1 " / "1\n", both lanes, and require(). tools/test_rig_skip_contract.py 10 cases -- the CALL SITES. A helper with no call-site test is just another convention to miss, which is #178's own thesis: nothing otherwise pinned that a rig asks platform_supported() BEFORE check_prerequisites(), and swapping those two lines silently returns macOS to exit 2 with a green suite. Both ordering assertions were mutation-tested: swapping the gates in rig_phase1_dhcp.py trips the tripwire naming check_prerequisites, and swapping problems/contention in rig_vice_https_macos.py fails the routing assertion. No `total_run == 0` vacuity guard is left in test_ecdsa_p384_kat.py. One was written and removed: _build_vector_list() is never empty and _run_backend() counts every vector, including under --sha-only, so it could not fire under any flag combination. A check that matches nothing is the shape #178 exists to close, and shipping one inside #178's own implementation is the worst place for it. tools/test_rig_skip_contract.py does NOT import tools/https_e2e at module level. That package re-exports from .vice_on_bridge, which imports c64_test_harness from a sibling checkout, so a module-level import made bare `pytest` on a fresh clone die with a COLLECTION ERROR -- which pytest.ini's own comment says must never be mistaken for a passing run -- and it silently voided the very hazard the module's docstring cites as its reason not to import the macOS rig. It is now loaded on first use through require(), the API's first production call site: with c64_test_harness blocked, the module imports cleanly and six tests FAIL by name carrying the full reason, while the four source-inspection tests still pass. KNOWN GAP, follow-up owed. The four test_macos_rig_* cases assert on SOURCE TEXT only, so behavioural mutants of tests/rig_vice_https_macos.py survive them: reordering its gates at runtime, or changing a verdict without changing the anchored text, would not be caught (mutants M5-M8 in review). Closing that needs the rig restructured to be importable without the sibling harness, which is not a drive-by. Stated plainly: this commit pins the four bridge rigs BEHAVIOURALLY and the macOS rig by SHAPE only. SCOPE. #178 stays split: Part 2 (the tools/test_pytest_boundary.py guard) lands as #178b AFTER #172 and #177, because it codifies a rule those two are instances of and pinning a rule while live violations remain is the wrong order. The consequence is explicit: this PR's guard will never be SEEN to fail, and #178b owes the red-first demonstration. A ninth instance of the class exists at tools/test_uci_data_acc.py:714-721 (missing PRG -> pytest.skip), in the exact lane require() was written for. It is owned by open PR #172 and deliberately untouched here; a follow-up is owed once that lands. This PR's one-line pytest.ini addition was 3-way merge-tested against #172's pytest.ini: merges clean, no conflict. Co-Authored-By: Claude Opus 5 (1M context) REBASE NOTE (2026-09-05, onto dc06095). Written at 0b55c30; #168, #172, #173, #175 and #176 have landed since. Re-verified, and one paragraph above is now historical rather than current: - #172 is MERGED, so the "ninth instance" it owned is closed. It is closed by a hand-rolled implementation of this same policy inside tools/test_uci_data_acc.py (`_require`/`Unavailable`, the "1" comparison, exit 2, the reason string carrying its own vacuity warning), not by importing require(). The follow-up that paragraph says is owed is therefore now actionable: converge that module onto tools/_skip_policy.py. Not done here -- it is a separate change to a file this commit does not touch. - #177 is still OPEN, so the SCOPE paragraph stands: #178b still lands after it. - The only textual conflict on the rebase was pytest.ini's testpaths list, where #175 had added tools/test_runner_coverage.py. Resolved by keeping all three, in alphabetical order. - Neither new module defines a module-level run_tests(), so #175's tools/test_runner_coverage.py guard does not claim them; it passes. Co-Authored-By: Claude Opus 5 (1M context) (cherry picked from commit 85653f3399db790ac1a6c6e1057ce6cb0bac3505) --- pytest.ini | 2 + tests/rig_phase1_dhcp.py | 67 +++++- tests/rig_phase2_http.py | 67 +++++- tests/rig_phase3_https.py | 67 +++++- tests/rig_phase3_https_1mhz.py | 133 ++++++++--- tests/rig_vice_https_macos.py | 146 ++++++++++-- tools/_skip_policy.py | 280 +++++++++++++++++++++++ tools/https_e2e/__init__.py | 4 +- tools/https_e2e/env.py | 27 ++- tools/test_ecdsa_p384_kat.py | 49 +++- tools/test_p384_symbols.py | 36 ++- tools/test_rig_skip_contract.py | 393 ++++++++++++++++++++++++++++++++ tools/test_skip_policy.py | 243 ++++++++++++++++++++ 13 files changed, 1426 insertions(+), 88 deletions(-) create mode 100644 tools/_skip_policy.py create mode 100644 tools/test_rig_skip_contract.py create mode 100644 tools/test_skip_policy.py diff --git a/pytest.ini b/pytest.ini index 29ff73b..50e7c1f 100644 --- a/pytest.ini +++ b/pytest.ini @@ -50,7 +50,9 @@ testpaths = tools/test_package_verify.py tools/test_pytest_boundary.py tools/test_reserved_test_host.py + tools/test_rig_skip_contract.py tools/test_runner_coverage.py + tools/test_skip_policy.py tools/test_uci_data_acc.py norecursedirs = .git libs ip65 ip65-build build dist tests tools/uci diff --git a/tests/rig_phase1_dhcp.py b/tests/rig_phase1_dhcp.py index 76ef0ea..75e9f03 100644 --- a/tests/rig_phase1_dhcp.py +++ b/tests/rig_phase1_dhcp.py @@ -9,10 +9,16 @@ Run: PYTHONPATH=tools python3 tests/rig_phase1_dhcp.py -Exit codes: +Exit codes (tools/_skip_policy.py, issue #178): 0 -- PASS - 0 -- SKIP (clearly printed) - 1 -- FAIL + 1 -- FAIL (a check ran and failed) + 0 -- NOT APPLICABLE: this rig is Linux-only, and on any other host + tests/rig_vice_https_macos.py owns the coverage. A named verdict, + never a bare skip. + 2 -- COULD NOT RUN (a prerequisite is missing ON LINUX, or the build is + broken -- nothing was verified). Set C64_ALLOW_SKIP=1 to accept a + prerequisite-missing run as exit 0; a FAILED BUILD is never opted + out of. """ from __future__ import annotations @@ -27,6 +33,9 @@ if _TOOLS not in sys.path: sys.path.insert(0, _TOOLS) +# needs _TOOLS on sys.path, hence the placement below the block above +from _skip_policy import cannot_run, not_applicable # noqa: E402 + PRG_PATH = os.path.join(_REPO_ROOT, "build", "c64-https.prg") # Exact literal from src/boot.asm (menu_msg @ line 424-426). @@ -38,9 +47,42 @@ DHCP_TIMEOUT = 90.0 -def _skip(reason: str) -> int: - print(f"SKIP: {reason}") - return 0 +_CERTIFIES = "ip65 net_dhcp_acquire end-to-end in VICE" + + +def _cannot_run(reason: str, *, opt_out: bool = True) -> int: + """An involuntary skip is a FAILURE -- nothing was verified (issue #178). + + `opt_out=False` for a broken build: a failed `make` is never laundered + into a pass, not even by C64_ALLOW_SKIP. + """ + return cannot_run( + reason, + executed=0, + total=1, + certifies=_CERTIFIES, + opt_out_env="C64_ALLOW_SKIP" if opt_out else None, + ) + + +_COUNTERPART = ( + "tests/rig_vice_https_macos.py owns this coverage on macOS -- its handshake begins with the same ip65 DHCP acquisition" +) + + +def _wrong_platform() -> int: + """A VOLUNTARY skip: this host can never run this rig (issue #178). + + The load-bearing question is what the remedy is. "install iproute2" is + an involuntary skip and stays exit 2 -- but on a non-Linux host there is + no remedy at all, and another rig owns the coverage, so nothing is lost + and exit 2 would be a red nobody can ever clear. + """ + return not_applicable( + f"this rig is Linux-only (br-c64 bridge + netfilter + /proc/net/udp); " + f"this host is {sys.platform} -- {_COUNTERPART}", + certifies=_CERTIFIES, + ) def _ensure_built() -> bool: @@ -59,6 +101,7 @@ def main() -> int: from https_e2e import ( BridgeEnv, check_prerequisites, + platform_supported, launch_vice_on_bridge, shutdown_vice, press_key, @@ -66,12 +109,20 @@ def main() -> int: get_screen_text, ) + # Platform FIRST: check_prerequisites() cannot answer this, because the + # same string ("ip not on PATH") means "installable" on Linux and "wrong + # OS" everywhere else (issue #178). + if not platform_supported(): + return _wrong_platform() + missing = check_prerequisites() if missing: - return _skip("missing prerequisites: " + "; ".join(missing)) + return _cannot_run("missing prerequisites: " + "; ".join(missing)) if not _ensure_built(): - return _skip("c64-https.prg could not be built") + return _cannot_run("c64-https.prg could not be built -- `make` " + "failed; this is a broken build, not a " + "missing prerequisite", opt_out=False) # ---- Run the test ------------------------------------------------------ handle = None diff --git a/tests/rig_phase2_http.py b/tests/rig_phase2_http.py index 765aae5..20b24b5 100644 --- a/tests/rig_phase2_http.py +++ b/tests/rig_phase2_http.py @@ -9,10 +9,16 @@ Run: sudo PYTHONPATH=tools python3 tests/rig_phase2_http.py -Exit codes: +Exit codes (tools/_skip_policy.py, issue #178): 0 -- PASS - 0 -- SKIP (clearly printed) - 1 -- FAIL + 1 -- FAIL (a check ran and failed) + 0 -- NOT APPLICABLE: this rig is Linux-only, and on any other host + tests/rig_vice_https_macos.py owns the coverage. A named verdict, + never a bare skip. + 2 -- COULD NOT RUN (a prerequisite is missing ON LINUX, or the build is + broken -- nothing was verified). Set C64_ALLOW_SKIP=1 to accept a + prerequisite-missing run as exit 0; a FAILED BUILD is never opted + out of. """ from __future__ import annotations @@ -27,6 +33,9 @@ if _TOOLS not in sys.path: sys.path.insert(0, _TOOLS) +# needs _TOOLS on sys.path, hence the placement below the block above +from _skip_policy import cannot_run, not_applicable # noqa: E402 + PRG_PATH = os.path.join(_REPO_ROOT, "build", "c64-https.prg") # Screen needles (from src/boot.asm string labels). @@ -40,9 +49,42 @@ HTTP_TIMEOUT = 120.0 -def _skip(reason: str) -> int: - print(f"SKIP: {reason}") - return 0 +_CERTIFIES = "the plaintext HTTP path over emulated RR-Net" + + +def _cannot_run(reason: str, *, opt_out: bool = True) -> int: + """An involuntary skip is a FAILURE -- nothing was verified (issue #178). + + `opt_out=False` for a broken build: a failed `make` is never laundered + into a pass, not even by C64_ALLOW_SKIP. + """ + return cannot_run( + reason, + executed=0, + total=1, + certifies=_CERTIFIES, + opt_out_env="C64_ALLOW_SKIP" if opt_out else None, + ) + + +_COUNTERPART = ( + "tests/rig_vice_https_macos.py is the macOS counterpart for the emulated-RR-Net path; note it drives it over TLS, so plaintext HTTP specifically has no macOS rig" +) + + +def _wrong_platform() -> int: + """A VOLUNTARY skip: this host can never run this rig (issue #178). + + The load-bearing question is what the remedy is. "install iproute2" is + an involuntary skip and stays exit 2 -- but on a non-Linux host there is + no remedy at all, and another rig owns the coverage, so nothing is lost + and exit 2 would be a red nobody can ever clear. + """ + return not_applicable( + f"this rig is Linux-only (br-c64 bridge + netfilter + /proc/net/udp); " + f"this host is {sys.platform} -- {_COUNTERPART}", + certifies=_CERTIFIES, + ) def _ensure_built() -> bool: @@ -97,6 +139,7 @@ def main() -> int: from https_e2e import ( BridgeEnv, check_prerequisites, + platform_supported, launch_vice_on_bridge, shutdown_vice, press_key, @@ -106,12 +149,20 @@ def main() -> int: stop_http_listener, ) + # Platform FIRST: check_prerequisites() cannot answer this, because the + # same string ("ip not on PATH") means "installable" on Linux and "wrong + # OS" everywhere else (issue #178). + if not platform_supported(): + return _wrong_platform() + missing = check_prerequisites() if missing: - return _skip("missing prerequisites: " + "; ".join(missing)) + return _cannot_run("missing prerequisites: " + "; ".join(missing)) if not _ensure_built(): - return _skip("c64-https.prg could not be built") + return _cannot_run("c64-https.prg could not be built -- `make` " + "failed; this is a broken build, not a " + "missing prerequisite", opt_out=False) handle = None listener = None diff --git a/tests/rig_phase3_https.py b/tests/rig_phase3_https.py index a404689..abdfc5f 100644 --- a/tests/rig_phase3_https.py +++ b/tests/rig_phase3_https.py @@ -14,10 +14,16 @@ Run: sudo PYTHONPATH=tools python3 tests/rig_phase3_https.py -Exit codes: +Exit codes (tools/_skip_policy.py, issue #178): 0 -- PASS - 0 -- SKIP (clearly printed) - 1 -- FAIL + 1 -- FAIL (a check ran and failed) + 0 -- NOT APPLICABLE: this rig is Linux-only, and on any other host + tests/rig_vice_https_macos.py owns the coverage. A named verdict, + never a bare skip. + 2 -- COULD NOT RUN (a prerequisite is missing ON LINUX, or the build is + broken -- nothing was verified). Set C64_ALLOW_SKIP=1 to accept a + prerequisite-missing run as exit 0; a FAILED BUILD is never opted + out of. """ from __future__ import annotations @@ -32,6 +38,9 @@ if _TOOLS not in sys.path: sys.path.insert(0, _TOOLS) +# needs _TOOLS on sys.path, hence the placement below the block above +from _skip_policy import cannot_run, not_applicable # noqa: E402 + PRG_PATH = os.path.join(_REPO_ROOT, "build", "c64-https.prg") # Screen needles (from src/boot.asm string labels). @@ -82,9 +91,42 @@ HTTPS_TIMEOUT = 1800.0 -def _skip(reason: str) -> int: - print(f"SKIP: {reason}") - return 0 +_CERTIFIES = "the TLS 1.3 handshake + GET over emulated RR-Net" + + +def _cannot_run(reason: str, *, opt_out: bool = True) -> int: + """An involuntary skip is a FAILURE -- nothing was verified (issue #178). + + `opt_out=False` for a broken build: a failed `make` is never laundered + into a pass, not even by C64_ALLOW_SKIP. + """ + return cannot_run( + reason, + executed=0, + total=1, + certifies=_CERTIFIES, + opt_out_env="C64_ALLOW_SKIP" if opt_out else None, + ) + + +_COUNTERPART = ( + "tests/rig_vice_https_macos.py owns this coverage on macOS" +) + + +def _wrong_platform() -> int: + """A VOLUNTARY skip: this host can never run this rig (issue #178). + + The load-bearing question is what the remedy is. "install iproute2" is + an involuntary skip and stays exit 2 -- but on a non-Linux host there is + no remedy at all, and another rig owns the coverage, so nothing is lost + and exit 2 would be a red nobody can ever clear. + """ + return not_applicable( + f"this rig is Linux-only (br-c64 bridge + netfilter + /proc/net/udp); " + f"this host is {sys.platform} -- {_COUNTERPART}", + certifies=_CERTIFIES, + ) def _ensure_built() -> bool: @@ -458,6 +500,7 @@ def main() -> int: from https_e2e import ( BridgeEnv, check_prerequisites, + platform_supported, launch_vice_on_bridge, shutdown_vice, press_key, @@ -467,12 +510,20 @@ def main() -> int: stop_https_listener, ) + # Platform FIRST: check_prerequisites() cannot answer this, because the + # same string ("ip not on PATH") means "installable" on Linux and "wrong + # OS" everywhere else (issue #178). + if not platform_supported(): + return _wrong_platform() + missing = check_prerequisites() if missing: - return _skip("missing prerequisites: " + "; ".join(missing)) + return _cannot_run("missing prerequisites: " + "; ".join(missing)) if not _ensure_built(): - return _skip("c64-https.prg could not be built") + return _cannot_run("c64-https.prg could not be built -- `make` " + "failed; this is a broken build, not a " + "missing prerequisite", opt_out=False) handle = None listener = None diff --git a/tests/rig_phase3_https_1mhz.py b/tests/rig_phase3_https_1mhz.py index bbaa47e..eba0239 100644 --- a/tests/rig_phase3_https_1mhz.py +++ b/tests/rig_phase3_https_1mhz.py @@ -20,11 +20,19 @@ Run (after the UCI test has fully stopped): sudo env VICE_HTTPS_OK_TO_RUN=1 PYTHONPATH=tools python3 tests/rig_phase3_https_1mhz.py -Exit codes: +Exit codes (tools/_skip_policy.py, issue #178): 0 -- PASS - 0 -- SKIP (clearly printed) - 1 -- FAIL - 2 -- pre-flight gate refused (U64E test probably still running) + 0 -- NOT APPLICABLE, as a named verdict rather than a bare skip, in two + cases: this rig is Linux-only, and on any other host + tests/rig_vice_https_macos.py owns the coverage; or + VICE_HTTPS_OK_TO_RUN is unset, which is the operator declining an + opt-in rig. Neither loses coverage. + 1 -- FAIL (a check ran and failed) + 2 -- COULD NOT RUN (a prerequisite is missing ON LINUX, the build is + broken, or port 443 is held by another run -- nothing was + verified). Set C64_ALLOW_SKIP=1 to accept a prerequisite-missing + run as exit 0; a FAILED BUILD and CONTENTION are never opted out + of. """ from __future__ import annotations @@ -40,6 +48,9 @@ if _TOOLS not in sys.path: sys.path.insert(0, _TOOLS) +# needs _TOOLS on sys.path, hence the placement below the block above +from _skip_policy import cannot_run, not_applicable # noqa: E402 + PRG_PATH = os.path.join(_REPO_ROOT, "build", "c64-https.prg") # Screen needles (from src/boot.asm string labels). @@ -98,9 +109,42 @@ SCREEN_SNAP_KEEP = 30 -def _skip(reason: str) -> int: - print(f"SKIP: {reason}") - return 0 +_CERTIFIES = "the TLS 1.3 handshake + GET at honest 1 MHz over emulated RR-Net" + + +def _cannot_run(reason: str, *, opt_out: bool = True) -> int: + """An involuntary skip is a FAILURE -- nothing was verified (issue #178). + + `opt_out=False` for a broken build: a failed `make` is never laundered + into a pass, not even by C64_ALLOW_SKIP. + """ + return cannot_run( + reason, + executed=0, + total=1, + certifies=_CERTIFIES, + opt_out_env="C64_ALLOW_SKIP" if opt_out else None, + ) + + +_COUNTERPART = ( + "tests/rig_vice_https_macos.py owns this coverage on macOS (E2E_NO_WARP=1 for the 1 MHz variant)" +) + + +def _wrong_platform() -> int: + """A VOLUNTARY skip: this host can never run this rig (issue #178). + + The load-bearing question is what the remedy is. "install iproute2" is + an involuntary skip and stays exit 2 -- but on a non-Linux host there is + no remedy at all, and another rig owns the coverage, so nothing is lost + and exit 2 would be a red nobody can ever clear. + """ + return not_applicable( + f"this rig is Linux-only (br-c64 bridge + netfilter + /proc/net/udp); " + f"this host is {sys.platform} -- {_COUNTERPART}", + certifies=_CERTIFIES, + ) def _ensure_built() -> bool: @@ -540,48 +584,73 @@ def _emit(line: str) -> None: def main() -> int: + # This import is inert (module-level constants and defs only), so it can + # precede the pre-flight gate, whose real constraint is that it run + # before BridgeEnv mutates host netfilter via sudo. + from https_e2e import ( + BridgeEnv, + check_prerequisites, + platform_supported, + launch_vice_on_bridge, + shutdown_vice, + press_key, + wait_for_screen_text, + get_screen_text, + start_https_listener, + stop_https_listener, + ) + + # Platform FIRST: check_prerequisites() cannot answer this, because the + # same string ("ip not on PATH") means "installable" on Linux and "wrong + # OS" everywhere else (issue #178). Ahead of the gates below so that a + # host which can never run this rig is never told to opt in, and never + # trips the port check. + if not platform_supported(): + return _wrong_platform() + # --- Pre-flight gate (must run BEFORE BridgeEnv, which mutates host # netfilter via sudo). Refuses if the UCI HTTPS listener is still up. + # + # An unset opt-in flag is the operator DECLINING to run this rig: a + # voluntary skip, exit 0 with a named verdict. It is not a coverage + # hole -- nothing was lost and nothing is broken (issue #178). if os.environ.get("VICE_HTTPS_OK_TO_RUN") != "1": - print( - "ABORT: VICE_HTTPS_OK_TO_RUN is not set.\n" - " This test is gated to prevent collision with the UCI HTTPS\n" - " listener (which binds 443/4433 on the LAN interface). The\n" - " U64E test is likely still running. Wait for it to finish,\n" - " then re-run with:\n" + return not_applicable( + "VICE_HTTPS_OK_TO_RUN is not set -- this rig is opt-in because it\n" + " collides with the UCI HTTPS listener (which binds 443/4433 on\n" + " the LAN interface); the U64E test may still be running.\n" + " To run it, wait for that to finish, then:\n" " sudo env VICE_HTTPS_OK_TO_RUN=1 PYTHONPATH=tools \\\n" - " python3 tests/rig_phase3_https_1mhz.py" + " python3 tests/rig_phase3_https_1mhz.py", + certifies=_CERTIFIES, ) - return 2 + + # A held port is CONTENTION, the third category: the rig exists, someone + # else has it. Exit 2 like any could-not-run, but deliberately NOT + # opt-out-able -- a lane that silences contention goes green exactly when + # it collides, and unlike a missing tool it clears on its own. Same + # decision as tests/rig_vice_https_macos.py's feth0 check. for port in (443,): s = socket.socket(socket.AF_INET, socket.SOCK_STREAM) try: s.bind(("0.0.0.0", port)) except OSError as e: - # something else is holding it — refuse - print(f"refusing: port {port} is in use (UCI test?): {e}") - return 2 + return _cannot_run( + f"port {port} is held by another process (the UCI test?): {e}" + f"\n fix: wait for the other run to finish", + opt_out=False, + ) finally: s.close() - from https_e2e import ( - BridgeEnv, - check_prerequisites, - launch_vice_on_bridge, - shutdown_vice, - press_key, - wait_for_screen_text, - get_screen_text, - start_https_listener, - stop_https_listener, - ) - missing = check_prerequisites() if missing: - return _skip("missing prerequisites: " + "; ".join(missing)) + return _cannot_run("missing prerequisites: " + "; ".join(missing)) if not _ensure_built(): - return _skip("c64-https.prg could not be built") + return _cannot_run("c64-https.prg could not be built -- `make` " + "failed; this is a broken build, not a " + "missing prerequisite", opt_out=False) handle = None listener = None diff --git a/tests/rig_vice_https_macos.py b/tests/rig_vice_https_macos.py index 52e547f..660853c 100644 --- a/tests/rig_vice_https_macos.py +++ b/tests/rig_vice_https_macos.py @@ -34,7 +34,13 @@ and the C64 dies at TCP CONNECT with nothing on the wire. The listener self-probe below detects this and says so. -Exit codes: 0 PASS / 0 SKIP (printed) / 1 FAIL. +Exit codes (tools/_skip_policy.py, issue #178): + 0 PASS, or NOT APPLICABLE on a non-macOS host (tests/rig_phase3_https.py + owns that coverage on Linux) -- a named verdict, never a bare skip. + 1 FAIL (a check ran and failed) + 2 COULD NOT RUN -- the rig is not up, or it is contended, so nothing was + verified. C64_ALLOW_SKIP=1 accepts a rig-NOT-READY run as exit 0; it + does NOT cover contention, and does not cover a failed build. """ from __future__ import annotations @@ -51,6 +57,10 @@ if p not in sys.path: sys.path.insert(0, p) +# needs _TOOLS on sys.path, hence the placement below the block above +from _skip_policy import cannot_run, not_applicable # noqa: E402 + +_CERTIFIES = "the TLS 1.3 handshake + GET over emulated RR-Net on macOS" from c64_test_harness.backends.vice_binary import BinaryViceTransport # noqa: E402 from c64_test_harness import Labels, read_bytes # noqa: E402 from https_e2e import ( # noqa: E402 @@ -88,12 +98,39 @@ ) -def _rig_check() -> list[str]: - """Return a list of missing-prerequisite messages (empty = rig OK).""" - problems = [] - if sys.platform != "darwin": - problems.append("not macOS (use tests/rig_phase3_https.py on Linux)") - return problems +def _platform_supported() -> bool: + """True if this host can run the feth/pcap rig at all. + + Module-level and parameterless on purpose: a test can replace THIS to + exercise both branches, instead of patching `sys.platform` globally. + """ + return sys.platform == "darwin" + + +def _rig_check() -> "tuple[list[str], list[str]]": + """Return (problems, contention) for a macOS host. Both empty = rig OK. + + Two lists because the remedies differ in kind, and issue #178's rule is + that the remedy decides the verdict: + + problems -- something is not installed or not set up. Remedy: run + `sudo bash tools/rig-up-macos.sh`, build the VICE. An + involuntary skip, exit 2, opt-out-able by a lane that + knowingly has no rig. + contention -- the rig exists but another process holds it. Remedy: + wait, or kill YOUR stale instance. Also exit 2, but + deliberately NOT opt-out-able: a lane that silences + contention goes green every time it collides, which is + the exact vacuous-pass this policy exists to stop, and + unlike a missing tool it is transient, so silencing it + hides a condition that would have cleared on its own. + + The platform question is asked by the CALLER, before this runs -- a + non-Darwin host is a voluntary skip owned by tests/rig_phase3_https.py, + not a problem with this rig. + """ + problems: list[str] = [] + contention: list[str] = [] if not os.path.exists(VICE_BIN): problems.append( f"{VICE_BIN} missing — build it per c64-test-harness#144 " @@ -113,13 +150,31 @@ def _rig_check() -> list[str]: # eats/garbles ARP and TCP meant for this run. Other x64sc processes # NOT on feth0 (other projects' fleets on this shared bench) are # fine — never kill those. - r = subprocess.run(["pgrep", "-fl", "ethernetioif feth0"], - capture_output=True, text=True) - if r.stdout.strip(): - problems.append( + # Match the BINARY first, then that process's argv. `pgrep -fl + # "ethernetioif feth0"` matched the full command line of EVERY process + # against an unanchored pattern, so anything merely containing that text + # matched too: a grep for it, an editor opened on this file, a driver + # script passing it as an argument. A false positive is unrecoverable + # here, because contention is deliberately not opt-out-able -- so the + # detector has to be narrow. `pgrep -x x64sc` matches only processes + # whose executable name is exactly x64sc; the argv check then confirms it + # is the one on feth0. + attached: list[str] = [] + r = subprocess.run(["pgrep", "-x", "x64sc"], capture_output=True, text=True) + for pid in r.stdout.split(): + if not pid.isdigit(): + continue + ps = subprocess.run(["ps", "-o", "command=", "-p", pid], + capture_output=True, text=True) + cmd = " ".join(ps.stdout.split()) + if "ethernetioif" in cmd and "feth0" in cmd: + attached.append(f"pid {pid}: {cmd}") + if attached: + contention.append( "another VICE is already attached to feth0 (duplicate-MAC " - f"conflict):\n {r.stdout.strip()}\n kill YOUR stale " - "instance (do not touch other projects' x64sc processes)") + "conflict):\n " + "\n ".join(attached) + + "\n kill YOUR stale instance (do not touch other projects' " + "x64sc processes)") try: pid = int(open("/tmp/c64-rig-dnsmasq.pid").read().strip()) os.kill(pid, 0) @@ -127,7 +182,7 @@ def _rig_check() -> list[str]: pass # EPERM = process exists (it's root-owned) — rig is up except (OSError, ValueError): problems.append("rig dnsmasq not running (pid file stale/absent)") - return problems + return problems, contention def _build_prg() -> None: @@ -141,7 +196,18 @@ def _build_prg() -> None: if r.returncode != 0: print(r.stdout[-2000:]) print(r.stderr[-2000:]) - raise SystemExit("build failed") + # A failed build is exit 2 (could not run), not exit 1 (a check ran + # and failed) -- the same verdict the other four rigs give, and never + # opted out of. SystemExit("build failed") used to exit 1 here, which + # made a broken build indistinguishable from a real handshake failure. + raise SystemExit(cannot_run( + "the PRG could not be built -- `make` failed; this is a broken " + "build, not a missing prerequisite", + executed=0, + total=1, + certifies=_CERTIFIES, + opt_out_env=None, + )) def _launch_vice() -> subprocess.Popen: @@ -223,13 +289,51 @@ def _sigterm(_sig, _frame): signal.signal(signal.SIGTERM, _sigterm) - problems = _rig_check() + # Platform FIRST, and it is a VOLUNTARY skip: a Linux host can never run + # the feth/pcap rig, and tests/rig_phase3_https.py owns that coverage + # there. Exit 2 would be a red nobody on that platform could clear + # (issue #178). + if not _platform_supported(): + return not_applicable( + f"this rig is macOS-only (feth pair + /dev/bpf pcap); this host " + f"is {sys.platform} -- tests/rig_phase3_https.py owns this " + f"coverage on Linux", + certifies=_CERTIFIES, + ) + + problems, contention = _rig_check() + + # PROBLEMS FIRST. Contention on a rig that does not exist is a + # meaningless statement: with no VICE binary, no feth1 and no dnsmasq, + # "another VICE holds feth0" is the wrong headline and demotes the real + # cause to a parenthetical. Worse, contention is deliberately not + # opt-out-able, so evaluating it first left a CI lane that legitimately + # has no rig -- and correctly set C64_ALLOW_SKIP=1 -- red with no + # recourse. An involuntary skip is a FAILURE, but this one is the kind + # an operator can opt out of (issue #178). if problems: - print("SKIP: rig not ready:") - for p in problems: - print(f" - {p}") - print(" fix: (re)run `sudo bash tools/rig-up-macos.sh`") - return 0 + return cannot_run( + "rig not ready:\n - " + "\n - ".join(problems) + + "\n fix: (re)run `sudo bash tools/rig-up-macos.sh`", + executed=0, + total=1, + certifies=_CERTIFIES, + opt_out_env="C64_ALLOW_SKIP", + ) + + # Only once the rig is otherwise complete does contention mean anything. + # Its own category: the rig is here, someone else has it. Exit 2 like any + # could-not-run, but with NO opt-out -- see _rig_check(). + if contention: + return cannot_run( + "rig is contended:\n - " + "\n - ".join(contention) + + "\n fix: wait for the other run, or kill YOUR stale " + "instance", + executed=0, + total=1, + certifies=_CERTIFIES, + opt_out_env=None, + ) if os.environ.get("C64_SKIP_BUILD") != "1": _build_prg() diff --git a/tools/_skip_policy.py b/tools/_skip_policy.py new file mode 100644 index 0000000..e1429c3 --- /dev/null +++ b/tools/_skip_policy.py @@ -0,0 +1,280 @@ +#!/usr/bin/env python3 +"""_skip_policy.py -- the project's involuntary-skip rule, in one import. + + An involuntary skip is a failure; a voluntary skip is allowed but must + never be silent. + +This repo has closed that class three times (#158 audit commit `7497e48`, +re-found as #165, fixed again in PR #172) because the correct shape was a +convention rather than a callable. This module is the callable. + +Two lanes, and the whole point is that they are different calls: + + * ``cannot_run()`` -- INVOLUNTARY. A prerequisite the caller asked for is + missing: no toolchain, no PRG, a failed ``make``, no hardware, no + fixture. Nothing was verified, so the run must not read as success. + Returns ``EXIT_CANNOT_RUN`` (2) -- deliberately distinct from 1 ("a + check ran and failed") so a caller can tell "broken" from "unproven". + + * ``not_applicable()`` -- VOLUNTARY. The caller chose a configuration + this suite does not cover (the other backend, a diagnostic mode). There + is genuinely nothing to verify, so exit 0 is honest -- but it is printed + as a named verdict with its scope spelled out, never as a bare line that + a reader mistakes for a pass. + +Exit-code contract for every caller wired to this module: + + 0 PASS, or a declared NOT APPLICABLE, or an acknowledged opt-out + 1 a check ran and FAILED + 2 COULD NOT RUN -- prerequisites missing, nothing was verified + +The opt-out +----------- +``cannot_run(..., opt_out_env="C64_ALLOW_SKIP")`` returns 0 when that +variable is set to exactly ``"1"`` -- not merely set, so ``=0`` and +``=false`` do NOT open the hatch. It still prints the full block: the +opt-out suppresses the exit code, never the warning. It exists so a CI lane +that legitimately has no hardware can stay green *by saying so in its own +configuration*, which is a decision someone made on purpose and can be +grepped for -- unlike a bare ``return 0`` buried in a prerequisite branch. + +The pytest channel +------------------ +Under pytest the skip *reason* is the only channel that survives: ``-ra`` +(pinned in ``pytest.ini`` ``addopts``) prints that string and nothing else, +and module stdout is swallowed. So ``reason_text()`` folds the vacuity +warning INTO the reason string rather than printing it alongside, and +``require()`` hands that same string to the failure/skip it raises. Without +that, an exit-0 opt-out reads as a bare ``3 skipped`` and the warning is +gone. + +Usage (script lane):: + + from _skip_policy import cannot_run, not_applicable + + if missing: + return cannot_run( + "missing prerequisites: " + "; ".join(missing), + executed=0, total=TOTAL_CHECKS, + certifies="the ip65 DHCP path in VICE", + opt_out_env="C64_ALLOW_SKIP", + ) + + if backend != "uci": + return not_applicable( + f"dual-overlay smoke test is UCI-only (backend={backend})", + certifies="the UCI dual-overlay swap dispatcher", + ) + +Usage (pytest lane):: + + from _skip_policy import require + + def test_something(): + require(shutil.which("ca65") is not None, + "ca65 not on PATH", + executed=0, total=1, + certifies="the build-flag stamp", + opt_out_env="C64_ALLOW_SKIP") + ... +""" + +from __future__ import annotations + +import os +import sys +from typing import Optional, TextIO + +__all__ = [ + "EXIT_PASS", + "EXIT_FAIL", + "EXIT_CANNOT_RUN", + "SkipPolicyError", + "reason_text", + "cannot_run", + "not_applicable", + "require", +] + +EXIT_PASS = 0 +EXIT_FAIL = 1 +EXIT_CANNOT_RUN = 2 + + +class SkipPolicyError(AssertionError): + """Raised by require() when an involuntary skip must be a failure. + + Subclasses AssertionError so pytest renders it as a plain failure and so + a caller that only catches AssertionError still catches it. + """ + + +def _opted_out(opt_out_env: Optional[str]) -> bool: + """True only if `opt_out_env` names a variable whose value is exactly "1". + + Bare truthiness would make ``C64_ALLOW_SKIP=0`` and + ``C64_ALLOW_SKIP=false`` ENABLE the opt-out -- someone setting 0 to shut + the escape hatch would silently disable the whole policy. Every other + gate in this repo (``C64_SKIP_BUILD``, ``C64_SKIP_TEMP_GC``, + ``C64_NET_TESTS_OPTIONAL``, ``VICE_HTTPS_OK_TO_RUN``) compares to the + literal "1"; this one does too. Surrounding whitespace is tolerated + because a shell export can carry it; nothing else is. + """ + if not opt_out_env: + return False + return os.environ.get(opt_out_env, "").strip() == "1" + + +def _coverage_clause(executed: Optional[int], total: Optional[int]) -> str: + if executed is None or total is None: + return "no checks executed" + return f"{executed} of {total} checks executed" + + +def reason_text( + reason: str, + *, + executed: Optional[int] = 0, + total: Optional[int] = None, + certifies: Optional[str] = None, + opt_out_env: Optional[str] = None, +) -> str: + """Build the one-string reason that carries its own vacuity warning. + + This is the string that must reach pytest's ``-ra`` line, where it is the + only surviving channel. It always states the coverage, and it always + states what the run therefore certifies nothing about. + """ + parts = [f"COULD NOT RUN: {reason}", _coverage_clause(executed, total)] + subject = certifies or "the behaviour under test" + parts.append(f"this run certifies NOTHING about {subject}") + if opt_out_env: + parts.append(f"set {opt_out_env}=1 to accept an unverified run") + return " -- ".join(parts) + + +def _print_block( + heading: str, + reason: str, + lines: "list[str]", + out: Optional[TextIO] = None, +) -> None: + stream = out if out is not None else sys.stdout + bar = "=" * 60 + print(bar, file=stream) + print(f"{heading}: {reason}", file=stream) + for line in lines: + print(f" {line}", file=stream) + print(bar, file=stream) + try: + stream.flush() + except Exception: # noqa: BLE001 - a closed/odd stream must not mask the verdict + pass + + +def cannot_run( + reason: str, + *, + executed: Optional[int] = 0, + total: Optional[int] = None, + certifies: Optional[str] = None, + opt_out_env: Optional[str] = None, + out: Optional[TextIO] = None, +) -> int: + """An INVOLUNTARY skip. Print the standard block, return 2 (or 0 if opted out). + + Returns ``EXIT_CANNOT_RUN`` so the caller can ``return cannot_run(...)`` + straight out of ``main()``. Returns ``EXIT_PASS`` only when + ``opt_out_env`` names a variable set to exactly "1" -- and even then the + block is printed in full. + """ + subject = certifies or "the behaviour under test" + opted_out = _opted_out(opt_out_env) + lines = [ + _coverage_clause(executed, total), + f"this run certifies NOTHING about {subject}", + ] + if opted_out: + lines.append( + f"{opt_out_env}=1 is set -- exiting 0 by explicit opt-out, " + "NOT because anything passed" + ) + _print_block("COULD NOT RUN (opt-out honoured)", reason, lines, out) + return EXIT_PASS + if opt_out_env: + lines.append( + f"set {opt_out_env}=1 to accept an unverified run (exit 0 instead of " + f"{EXIT_CANNOT_RUN})" + ) + lines.append(f"exit {EXIT_CANNOT_RUN} = could not run (1 would mean a check failed)") + _print_block("COULD NOT RUN", reason, lines, out) + return EXIT_CANNOT_RUN + + +def not_applicable( + reason: str, + *, + certifies: Optional[str] = None, + out: Optional[TextIO] = None, +) -> int: + """A VOLUNTARY skip. Print a named verdict, return 0. + + Use this ONLY when the caller's own configuration puts the subject out of + scope -- the other backend, a diagnostic mode -- so that there is nothing + to verify and exit 0 is the honest answer. If a prerequisite is missing, + that is ``cannot_run()``, not this. + """ + subject = certifies or "the behaviour under test" + lines = [ + "0 of 0 checks executed -- there is nothing here to verify in this " + "configuration", + f"this run certifies NOTHING about {subject}", + "exit 0 = not applicable (a prerequisite that is merely MISSING is " + f"exit {EXIT_CANNOT_RUN}, not this)", + ] + _print_block("NOT APPLICABLE", reason, lines, out) + return EXIT_PASS + + +def require( + condition: object, + reason: str, + *, + executed: Optional[int] = 0, + total: Optional[int] = None, + certifies: Optional[str] = None, + opt_out_env: Optional[str] = None, +) -> None: + """pytest-side ``cannot_run``: raise unless the prerequisite holds. + + On a false ``condition`` this raises :class:`SkipPolicyError` carrying the + full ``reason_text()`` -- pytest records a FAILURE, and the reason string + is self-contained because module stdout does not survive. + + If ``opt_out_env`` is set to exactly "1" it calls ``pytest.skip()`` + with the same self-contained string instead, so the ``-ra`` summary still + carries the vacuity warning rather than a bare "skipped". + """ + if condition: + return + text = reason_text( + reason, + executed=executed, + total=total, + certifies=certifies, + opt_out_env=opt_out_env, + ) + if _opted_out(opt_out_env): + try: + import pytest # noqa: PLC0415 - optional, only needed on this branch + except ImportError: + # No pytest, so there is no skip to record -- and returning would + # be the worst answer available: the caller would carry on into a + # test body whose prerequisite is missing, which is the vacuous + # pass this module exists to prevent. Fail closed instead. + raise SkipPolicyError( + f"{text} [{opt_out_env}=1 is set, but pytest is not installed, " + "so the skip cannot be recorded; failing closed rather than " + "running the body as if the prerequisite held]") + pytest.skip(f"{text} [{opt_out_env}=1 set]") + raise SkipPolicyError(text) diff --git a/tools/https_e2e/__init__.py b/tools/https_e2e/__init__.py index 59f9a8c..27ab880 100644 --- a/tools/https_e2e/__init__.py +++ b/tools/https_e2e/__init__.py @@ -7,12 +7,13 @@ launch_vice_on_bridge, shutdown_vice, press_key, wait_for_screen_text, check_prerequisites, + platform_supported, ) Internals live in underscored helpers in each submodule. """ -from .env import BridgeEnv, check_prerequisites +from .env import BridgeEnv, check_prerequisites, platform_supported from .vice_on_bridge import launch_vice_on_bridge, shutdown_vice from .c64_menu import press_key, wait_for_screen_text, get_screen_text from .http_listener import start_http_listener, stop_http_listener @@ -21,6 +22,7 @@ __all__ = [ "BridgeEnv", "check_prerequisites", + "platform_supported", "launch_vice_on_bridge", "shutdown_vice", "press_key", diff --git a/tools/https_e2e/env.py b/tools/https_e2e/env.py index f94aa79..aa28da2 100644 --- a/tools/https_e2e/env.py +++ b/tools/https_e2e/env.py @@ -12,6 +12,7 @@ import shutil import socket import subprocess +import sys import time from contextlib import contextmanager @@ -25,8 +26,32 @@ TAP1 = "tap-c64-1" +def platform_supported() -> bool: + """True if this host can run the br-c64 bridge rig at all. + + Asked BEFORE check_prerequisites(), and deliberately NOT folded into + it: that function returns a flat list of strings, and "ip not on PATH" + means "apt install iproute2" on Linux but "this OS has no iproute2 and + never will" on Darwin. No partition of the list can tell those apart, + because the platform question has to be answered first. + + The rig needs Linux netfilter (the setup script's iptables rules), the + iproute2 `ip` command, and /proc/net/udp -- which _port_open_udp() + below reads directly. A wrong platform is a VOLUNTARY skip owned by + tests/rig_vice_https_macos.py; a missing tool ON Linux is installable + and stays an involuntary one. See issue #178. + """ + return sys.platform.startswith("linux") + + def check_prerequisites() -> list[str]: - """Return a list of missing prereqs. Empty list means all OK.""" + """Return a list of missing prereqs. Empty list means all OK. + + Contract unchanged (issue #178 added platform_supported() beside it + rather than partitioning this list): the five tool checks and the sudo + probe below all describe things that are INSTALLABLE on the rig's + target platform. Callers ask platform_supported() first. + """ missing: list[str] = [] for tool in ("x64sc", "dnsmasq", "sudo", "ip", "iptables"): if shutil.which(tool) is None: diff --git a/tools/test_ecdsa_p384_kat.py b/tools/test_ecdsa_p384_kat.py index 413c1fd..f9c7530 100644 --- a/tools/test_ecdsa_p384_kat.py +++ b/tools/test_ecdsa_p384_kat.py @@ -48,8 +48,17 @@ verdict, so it always exits non-zero and can never report OVERALL: PASS — read the per-step output, not the tally. +Exit codes (tools/_skip_policy.py, issue #178): + 0 PASS + 1 FAIL (a vector ran and failed) + 2 COULD NOT RUN -- a requested prerequisite is missing (e.g. --u64 with + no U64_HOST) or no vector produced a verdict. Set C64_ALLOW_SKIP=1 to + accept such a run as exit 0. Not passing --u64 at all is a VOLUNTARY + skip and still exits 0. + Environment: C64_SKIP_BUILD=1 Reuse existing build artifacts (skip make). + C64_ALLOW_SKIP=1 Accept a could-not-run as exit 0 (prints why). U64_HOST= Ultimate 64 host (default 192.168.1.81). P384_KAT_VICE_TIMEOUT_S Per-VERIFY-step timeout under VICE (default 1800 s = 30 min). @@ -78,6 +87,12 @@ import time from pathlib import Path +_HERE = os.path.dirname(os.path.abspath(__file__)) +if _HERE not in sys.path: + sys.path.insert(0, _HERE) + +from _skip_policy import cannot_run # noqa: E402 + PROJECT_ROOT = Path(__file__).resolve().parents[1] PRG_PATH = PROJECT_ROOT / "build" / "c64-https.prg" LABELS_PATH = PROJECT_ROOT / "build" / "labels.txt" @@ -788,6 +803,19 @@ def main() -> int: print(f"=== test_ecdsa_p384_kat.py (P-384 dual-overlay KAT) ===") os.chdir(str(PROJECT_ROOT)) + # --u64 is the caller asking for hardware. Without U64_HOST the hardware + # lane cannot run at all, and the tally below would otherwise add 0/0 to + # the VICE result and print OVERALL: PASS (issue #178). Refuse up front, + # before the multi-hour VICE lane, rather than at the verdict. + if run_u64 and not os.environ.get("U64_HOST"): + return cannot_run( + "--u64 requested but U64_HOST is not set in the environment", + executed=0, + total=1, + certifies="the P-384 verify path on real hardware", + opt_out_env="C64_ALLOW_SKIP", + ) + # Check the upstream test vector file exists. if not NIST_VECTORS_PATH.exists(): print(f"FATAL: vector file not found: {NIST_VECTORS_PATH}") @@ -838,7 +866,16 @@ def main() -> int: if run_u64: print(f"\n=== U64 backend (real hardware) ===") if not os.environ.get("U64_HOST"): - print(" SKIP: --u64 requested but U64_HOST not set in env") + # Unreachable: the early gate in main() already refused. Kept as + # a backstop, and it must NOT leave a 0/0 lane -- that is exactly + # what the tally used to read as OVERALL: PASS (issue #178). + return cannot_run( + "--u64 requested but U64_HOST is not set in the environment", + executed=v_pass + v_fail, + total=v_pass + v_fail + len(vectors), + certifies="the P-384 verify path on real hardware", + opt_out_env="C64_ALLOW_SKIP", + ) else: try: u_pass, u_fail, u_details = _run_backend( @@ -861,6 +898,16 @@ def main() -> int: print(f"{'=' * 60}") total_fail = v_fail + u_fail + # NOTE: there is deliberately no `total_run == 0` vacuity guard here. + # One was written and removed: _build_vector_list() never returns an + # empty list (1 vector for the smoke default, 4 for --full) and + # _run_backend() puts every vector into `passed` or `failed`, including + # under --sha-only, whose short-circuit in _run_one_vector() returns an + # "error" dict and is therefore counted as a failure. So the guard could + # not fire under any flag combination. A check that matches nothing is + # the same vacuous-green shape issue #178 exists to close, so it does not + # belong in #178's own implementation. If a vector FILTER is ever added, + # add the guard back beside it -- where it can actually fire. overall = "PASS" if total_fail == 0 else "FAIL" print(f"OVERALL: {overall}") if sha_only: diff --git a/tools/test_p384_symbols.py b/tools/test_p384_symbols.py index e0bb038..8058726 100755 --- a/tools/test_p384_symbols.py +++ b/tools/test_p384_symbols.py @@ -44,7 +44,11 @@ only assert the state byte updates. 9. JSR crypto_swap_none. Confirm current_overlay == 0. -Exits 0 on PASS, 1 on FAIL or environmental error. +Exit codes (tools/_skip_policy.py, issue #178): + 0 PASS, or a declared NOT APPLICABLE (BACKEND != uci) + 1 FAIL (a check ran and failed) + 2 COULD NOT RUN -- a required build artifact is missing, so nothing was + verified. VICE harness gotcha: the PRG's boot path executes nistcurves P-256 fp_mul, which fetches 8x8 multiply rows from REU banks 0/1. Without @@ -57,8 +61,8 @@ BACKEND=uci /Users/someone/.local/share/c64-test-harness/venv/bin/python3 \ tools/test_p384_symbols.py [--verbose] -Under BACKEND=ip65 the script exits 0 with a skip message -- the -embedded-blobs path is UCI-only (ip65 has no main-RAM headroom for the +Under BACKEND=ip65 the script prints a NOT APPLICABLE verdict and exits 0 +(a voluntary skip, issue #178) -- the embedded-blobs path is UCI-only (ip65 has no main-RAM headroom for the extra 15 KB; see cfg/c64-https-ip65.cfg's Phase 3 comment block). """ @@ -66,6 +70,12 @@ import subprocess import sys +_HERE = os.path.dirname(os.path.abspath(__file__)) +if _HERE not in sys.path: + sys.path.insert(0, _HERE) + +from _skip_policy import cannot_run, not_applicable # noqa: E402 + PROJECT_ROOT = os.path.join(os.path.dirname(os.path.abspath(__file__)), "..") PRG_PATH = os.path.join(PROJECT_ROOT, "build", "c64-https.prg") LABELS_PATH = os.path.join(PROJECT_ROOT, "build", "labels.txt") @@ -125,10 +135,16 @@ def main() -> int: # Phase 3 dual-overlay embed is UCI-only -- ip65 has no main-RAM # headroom for the extra 15 KB after the existing layout. Under # ip65 the OVERLAY_BLOB_* segments are empty and the boot DMA is - # a no-op, so there is nothing meaningful to test. Skip cleanly. + # a no-op, so there is nothing meaningful to test. if backend != "uci": - print(f" SKIP: dual-overlay smoke test is UCI-only (backend={backend})") - return 0 + # VOLUNTARY skip (issue #178): the caller chose this backend and the + # segments under test are empty in it, so there is nothing to verify + # and exit 0 is honest -- but it is announced as a named verdict, not + # a bare `return 0` that reads as a pass. + return not_applicable( + f"dual-overlay smoke test is UCI-only (backend={backend})", + certifies="the UCI dual-overlay swap dispatcher", + ) if os.environ.get("C64_SKIP_BUILD") != "1": subprocess.run(["make", "clean", f"BACKEND={backend}"], @@ -145,8 +161,12 @@ def main() -> int: # Sanity-check on-disk artifacts. for path in (PRG_PATH, LABELS_PATH, SHA_BIN_PATH, CURVE_BIN_PATH): if not os.path.exists(path): - print(f"FATAL: required artifact missing: {path}") - return 1 + return cannot_run( + f"required artifact missing: {path}", + executed=0, + total=7, + certifies="the UCI dual-overlay swap dispatcher", + ) sha_image = open(SHA_BIN_PATH, "rb").read() curve_image = open(CURVE_BIN_PATH, "rb").read() diff --git a/tools/test_rig_skip_contract.py b/tools/test_rig_skip_contract.py new file mode 100644 index 0000000..944a197 --- /dev/null +++ b/tools/test_rig_skip_contract.py @@ -0,0 +1,393 @@ +#!/usr/bin/env python3 +"""Pin the skip contract AT THE CALL SITES, not just in the helper (#178). + +``tools/_skip_policy.py`` has its own tests. This file tests the thing those +cannot: that the eleven wired call sites still *use* it, and still ask their +questions in the right order. + +The gap this closes was the sharpest point of the #178 review. The four +bridge rigs must ask ``platform_supported()`` BEFORE ``check_prerequisites()``: +on macOS the prerequisite list contains "ip not on PATH", which is not +installable there, so asking it first turns four Linux-only rigs into a +permanent red on this project's primary platform -- and the only remedy on +offer is the global opt-out, i.e. the policy gets switched off everywhere. +Swap those two lines back in a future edit and nothing else in the tree +notices. That is #178's own thesis -- "a helper on its own is just another +convention to miss" -- turned on #178's own implementation. + +Pure logic: no VICE, no hardware, no build, no network. Every rig entry point +called here is called with a tripwire in place of the next step, so a test +that fails an ordering assertion fails loudly rather than starting a `make`. + +Two deliberate limitations, stated rather than hidden: + + * ``tests/rig_vice_https_macos.py`` is NOT imported. It imports + ``c64_test_harness`` from a hard-coded sibling checkout at module level, + which is a separate repo (``pip install -e ../c64-test-harness``), so + importing it here would make this module error on any machine without + that checkout. Its two ordering contracts are pinned by source + inspection instead -- weaker evidence, and labelled as such. + * ``platform_supported()`` is a pure function of ``sys.platform``, so it is + tested by patching exactly that one input. That is the whole of its + behaviour; there is nothing else to observe. + * ``tools/https_e2e`` reaches the same sibling checkout transitively (its + ``__init__`` re-exports from ``.vice_on_bridge``), so it is imported + lazily inside the tests that need it, through ``require()``. Without the + checkout those six tests FAIL by name; the four source-inspection tests + below still run. A module-level import would instead have been a pytest + COLLECTION ERROR on a fresh clone -- and ``pytest.ini`` says in as many + words that a collection error must never be mistaken for a passing run. + +KNOWN GAP, follow-up owed +------------------------- +The four ``test_macos_rig_*`` cases assert on SOURCE TEXT, so behavioural +mutants of ``tests/rig_vice_https_macos.py`` survive them -- reordering its +gates at runtime, or changing a verdict without changing the anchored text, +would not be caught. Closing that needs the rig restructured to be +importable without the sibling harness, which is not a drive-by. So: the +four bridge rigs are pinned BEHAVIOURALLY here; the macOS rig is pinned by +SHAPE only. +""" + +import io +import os +import sys +from contextlib import redirect_stdout + +_HERE = os.path.dirname(os.path.abspath(__file__)) +_REPO = os.path.dirname(_HERE) +_TESTS = os.path.join(_REPO, "tests") +for _p in (_HERE, _TESTS): + if _p not in sys.path: + sys.path.insert(0, _p) + +from _skip_policy import require # noqa: E402 + +# tools/https_e2e is NOT imported at module level. Its __init__ re-exports +# from .vice_on_bridge, which imports c64_test_harness from a sibling +# checkout (`pip install -e ../c64-test-harness`) -- a separate repo. This +# module is in pytest.ini testpaths, so a module-level import would make bare +# `pytest` on a fresh clone die with a COLLECTION ERROR, and pytest.ini's own +# comment says a collection error must never be mistaken for a passing run. +# +# It is also the exact hazard the docstring above cites as the reason not to +# import the macOS rig -- which the module-level import quietly voided. +# +# So it is loaded on first use and routed through require(): a missing +# sibling checkout is INVOLUNTARY, so the six tests that need it FAIL (exit 2 +# semantics), loudly and by name, while the four source-inspection tests below +# still run and still pin what they pin. +_HTTPS_E2E = None +_HTTPS_E2E_ERROR = None + +# The tests that cannot run without it, named so the failure says how much +# coverage is lost rather than just "import failed". +_NEEDS_HTTPS_E2E = 6 + + +def _https_e2e(): + """Return the https_e2e package, or fail this test loudly (never collect-fail).""" + global _HTTPS_E2E, _HTTPS_E2E_ERROR + if _HTTPS_E2E is None and _HTTPS_E2E_ERROR is None: + try: + import https_e2e as mod + except Exception as exc: # noqa: BLE001 - a broken sibling can raise anything + _HTTPS_E2E_ERROR = exc + else: + _HTTPS_E2E = mod + require( + _HTTPS_E2E is not None, + f"tools/https_e2e could not be imported ({_HTTPS_E2E_ERROR!r}) -- it " + "re-exports from .vice_on_bridge, which needs c64_test_harness from a " + "sibling checkout: pip install -e ../c64-test-harness", + executed=0, + total=_NEEDS_HTTPS_E2E, + certifies="the rigs' platform-before-prerequisites gate order", + opt_out_env="C64_ALLOW_SKIP", + ) + return _HTTPS_E2E + +# The four Linux bridge rigs. rig_phase3_https_1mhz is included even though +# it has two extra gates in front (an opt-in flag and a port check), because +# it is the one whose gate order was got wrong once already. +BRIDGE_RIGS = ( + "rig_phase1_dhcp", + "rig_phase2_http", + "rig_phase3_https", + "rig_phase3_https_1mhz", +) + +MACOS_RIG = os.path.join(_TESTS, "rig_vice_https_macos.py") + + +class Tripwire(AssertionError): + """Raised when a rig reaches a step an ordering test must never reach.""" + + +def _tripwire(name): + def _fire(*_a, **_k): + raise Tripwire(f"reached {name}() -- a gate let the run through") + return _fire + + +class _FakeSocket: + """Stands in for socket.socket in the 1 MHz rig's port-443 check. + + The bind succeeds, so the port check is not an environmental variable in + an ordering test. Whether 443 is free on the machine running the test + says nothing about gate order. + """ + + def __init__(self, *_a, **_k): + pass + + def bind(self, _addr): + return None + + def close(self): + return None + + +class _Patched: + """Restore module attributes and environment after a with-block.""" + + def __init__(self, mod, **kw): + self.mod = mod + self.kw = kw + self.saved = {} + self.env_saved = {} + + def env(self, **kw): + for k, v in kw.items(): + self.env_saved[k] = os.environ.get(k) + if v is None: + os.environ.pop(k, None) + else: + os.environ[k] = v + return self + + def __enter__(self): + for name, value in self.kw.items(): + self.saved[name] = getattr(self.mod, name) + setattr(self.mod, name, value) + return self + + def __exit__(self, *exc): + for name, value in self.saved.items(): + setattr(self.mod, name, value) + for k, v in self.env_saved.items(): + if v is None: + os.environ.pop(k, None) + else: + os.environ[k] = v + return False + + +def _import_rig(name): + import importlib + return importlib.import_module(name) + + +def _run_main(mod, extra_tripwires=()): + """Call mod.main() with output captured; returns (rc, output).""" + saved = {} + for attr in extra_tripwires: + saved[attr] = getattr(mod, attr) + setattr(mod, attr, _tripwire(attr)) + buf = io.StringIO() + try: + with redirect_stdout(buf): + rc = mod.main() + finally: + for attr, value in saved.items(): + setattr(mod, attr, value) + return rc, buf.getvalue() + + +# --------------------------------------------------------------------------- +# The platform predicate. +# --------------------------------------------------------------------------- + +def test_platform_supported_is_true_only_on_linux(): + _env = _https_e2e().env + saved = _env.sys.platform + try: + for value in ("linux", "linux2"): + _env.sys.platform = value + assert _env.platform_supported() is True, value + for value in ("darwin", "win32", "freebsd13", "cygwin"): + _env.sys.platform = value + assert _env.platform_supported() is False, value + finally: + _env.sys.platform = saved + + +def test_platform_supported_is_exported_from_the_package(): + # The rigs import it from the package, not from .env. + mod = _https_e2e() + assert mod.platform_supported is mod.env.platform_supported + + +# --------------------------------------------------------------------------- +# The ordering contract, behaviourally, on all four bridge rigs. +# --------------------------------------------------------------------------- + +def test_bridge_rigs_ask_platform_before_prerequisites(): + """Wrong platform -> exit 0, and check_prerequisites() is never called. + + The tripwire is the whole point: if a future edit moves the prerequisite + check back in front of the platform check, this raises instead of + quietly returning 2. + """ + pkg = _https_e2e() + for name in BRIDGE_RIGS: + mod = _import_rig(name) + with _Patched(pkg, platform_supported=lambda: False, + check_prerequisites=_tripwire("check_prerequisites")): + rc, out = _run_main(mod) + assert rc == 0, f"{name}: wrong platform must be exit 0, got {rc}\n{out}" + assert "NOT APPLICABLE" in out, f"{name}: no named verdict\n{out}" + + +def test_bridge_rigs_still_fail_when_the_platform_is_right(): + """Right platform + a missing prerequisite -> exit 2. + + The other half of the contract. A fix for the macOS over-fail that also + silenced the real coverage hole would pass the test above and fail this + one. + """ + pkg = _https_e2e() + for name in BRIDGE_RIGS: + mod = _import_rig(name) + saved_socket = getattr(mod, "socket", None) + if saved_socket is not None: + # Only rig_phase3_https_1mhz binds a port before the prerequisite + # check; neutralise it so gate ORDER is what this test measures. + mod.socket = type("m", (), {"socket": _FakeSocket, + "AF_INET": 0, "SOCK_STREAM": 0}) + try: + with _Patched(pkg, platform_supported=lambda: True, + check_prerequisites=lambda: ["a tool is missing"]).env( + VICE_HTTPS_OK_TO_RUN="1", C64_ALLOW_SKIP=None): + rc, out = _run_main(mod, extra_tripwires=("_ensure_built",)) + finally: + if saved_socket is not None: + mod.socket = saved_socket + assert rc == 2, f"{name}: missing prereq on-platform must be 2, got {rc}\n{out}" + assert "COULD NOT RUN" in out, f"{name}: no named verdict\n{out}" + + +def test_bridge_rigs_route_a_broken_build_to_two_without_an_opt_out(): + """A failed `make` is exit 2 even with C64_ALLOW_SKIP=1.""" + pkg = _https_e2e() + for name in BRIDGE_RIGS: + mod = _import_rig(name) + saved_socket = getattr(mod, "socket", None) + if saved_socket is not None: + mod.socket = type("m", (), {"socket": _FakeSocket, + "AF_INET": 0, "SOCK_STREAM": 0}) + saved_built = mod._ensure_built + mod._ensure_built = lambda: False # the build failed + try: + with _Patched(pkg, platform_supported=lambda: True, + check_prerequisites=lambda: []).env( + VICE_HTTPS_OK_TO_RUN="1", C64_ALLOW_SKIP="1"): + rc, out = _run_main(mod) + finally: + mod._ensure_built = saved_built + if saved_socket is not None: + mod.socket = saved_socket + assert rc == 2, f"{name}: broken build must be 2 even opted out, got {rc}\n{out}" + + +def test_the_opt_in_rig_treats_an_unset_flag_as_not_applicable(): + """VICE_HTTPS_OK_TO_RUN unset is the operator declining: exit 0.""" + mod = _import_rig("rig_phase3_https_1mhz") + with _Patched(_https_e2e(), platform_supported=lambda: True, + check_prerequisites=_tripwire("check_prerequisites")).env( + VICE_HTTPS_OK_TO_RUN=None): + rc, out = _run_main(mod) + assert rc == 0, f"unset opt-in flag must be exit 0, got {rc}\n{out}" + assert "NOT APPLICABLE" in out, out + assert "VICE_HTTPS_OK_TO_RUN" in out, out + + +# --------------------------------------------------------------------------- +# tests/rig_vice_https_macos.py -- source inspection only (see module docstring). +# --------------------------------------------------------------------------- + +def _macos_source(): + with open(MACOS_RIG, "r", encoding="utf-8") as fh: + return fh.read() + + +def _index_of(src, needle, label): + i = src.find(needle) + # A matcher that matches nothing is the same vacuous shape this whole + # file is about, one level up: fail loudly rather than pass by absence. + assert i >= 0, f"anchor not found in rig_vice_https_macos.py: {label} ({needle!r})" + return i + + +def test_macos_rig_reports_problems_before_contention(): + """Contention on a rig that does not exist is the wrong headline. + + It is also not opt-out-able, so evaluating it first left a no-rig CI lane + red with no recourse even when it correctly set C64_ALLOW_SKIP=1. + """ + src = _macos_source() + check = _index_of(src, "problems, contention = _rig_check()", "the _rig_check call") + problems = _index_of(src[check:], "if problems:", "the problems branch") + contention = _index_of(src[check:], "if contention:", "the contention branch") + assert problems < contention, ( + "rig_vice_https_macos.py must test `problems` before `contention`") + + +def test_macos_rig_asks_platform_before_checking_the_rig(): + src = _macos_source() + main_at = _index_of(src, "\ndef main() -> int:", "main()") + body = src[main_at:] + platform = _index_of(body, "_platform_supported()", "the platform gate") + check = _index_of(body, "_rig_check()", "the _rig_check call") + assert platform < check, ( + "rig_vice_https_macos.py must ask the platform question before " + "inspecting the rig") + + +def test_macos_rig_contention_detector_is_not_a_bare_substring_match(): + """`pgrep -fl "ethernetioif feth0"` matched any process whose command + line merely contained that text -- a grep, an editor, a driver script. + A false positive is unrecoverable because contention has no opt-out. + """ + src = _macos_source() + assert '"-fl", "ethernetioif feth0"' not in src, ( + "the unanchored pgrep -fl substring match is back") + _index_of(src, '"pgrep", "-x", "x64sc"', "the binary-name match") + + +def test_macos_rig_build_failure_exits_two(): + src = _macos_source() + assert 'raise SystemExit("build failed")' not in src, ( + 'SystemExit("build failed") exits 1, which makes a broken build ' + "indistinguishable from a real handshake failure") + _index_of(src, "raise SystemExit(cannot_run(", "the build-failure verdict") + + +def _standalone() -> int: + tests = [(n, o) for n, o in sorted(globals().items()) + if n.startswith("test_") and callable(o)] + assert tests, "FATAL: no tests found -- a matcher that matches nothing" + failed = 0 + for name, fn in tests: + try: + fn() + print(f" PASS {name}") + except Exception as exc: # noqa: BLE001 + failed += 1 + print(f" FAIL {name}: {exc!r}") + print(f"\n{len(tests) - failed}/{len(tests)} passed") + return 1 if failed else 0 + + +if __name__ == "__main__": + sys.exit(_standalone()) diff --git a/tools/test_skip_policy.py b/tools/test_skip_policy.py new file mode 100644 index 0000000..718cd0e --- /dev/null +++ b/tools/test_skip_policy.py @@ -0,0 +1,243 @@ +#!/usr/bin/env python3 +"""Tests for tools/_skip_policy.py -- the involuntary-skip rule itself (#178). + +Pure logic: no VICE, no hardware, no build. Runs under pytest from the repo +root (it is pinned in ``pytest.ini`` ``testpaths``) and standalone:: + + python3 tools/test_skip_policy.py + +The two things worth guarding here, both of which have a history: + + 1. **The opt-out must require the literal "1".** Bare truthiness makes + ``C64_ALLOW_SKIP=0`` *enable* the escape hatch, so someone setting 0 to + shut it off silently disables the whole policy instead. Every other + gate in this repo compares to "1"; the cases below pin that. + + 2. **The two lanes must not collapse into each other.** An involuntary + skip returns 2 and a voluntary one returns 0; a change that makes + everything fail is as wrong as the vacuous green it replaced. +""" + +import io +import os +import sys + +_HERE = os.path.dirname(os.path.abspath(__file__)) +if _HERE not in sys.path: + sys.path.insert(0, _HERE) + +from _skip_policy import ( # noqa: E402 + EXIT_CANNOT_RUN, + EXIT_PASS, + SkipPolicyError, + cannot_run, + not_applicable, + reason_text, + require, +) + +ENV = "C64_TEST_SKIP_POLICY_OPT_OUT" + + +class _Env: + """Set (or unset) ENV for the duration of a with-block.""" + + def __init__(self, value): + self.value = value + self.saved = None + + def __enter__(self): + self.saved = os.environ.get(ENV) + if self.value is None: + os.environ.pop(ENV, None) + else: + os.environ[ENV] = self.value + return self + + def __exit__(self, *exc): + if self.saved is None: + os.environ.pop(ENV, None) + else: + os.environ[ENV] = self.saved + return False + + +def _call(value, **kw): + """cannot_run() with ENV set to `value`; returns (exit code, output).""" + buf = io.StringIO() + with _Env(value): + rc = cannot_run( + "a prerequisite is missing", + executed=0, + total=3, + certifies="the thing under test", + opt_out_env=ENV, + out=buf, + **kw, + ) + return rc, buf.getvalue() + + +# --------------------------------------------------------------------------- +# 1. The opt-out is exactly "1", and nothing else. +# --------------------------------------------------------------------------- + +def test_opt_out_unset_is_a_failure(): + rc, out = _call(None) + assert rc == EXIT_CANNOT_RUN, rc + assert "COULD NOT RUN" in out + assert "opt-out honoured" not in out + + +def test_opt_out_one_is_honoured(): + rc, out = _call("1") + assert rc == EXIT_PASS, rc + assert "opt-out honoured" in out + # The warning survives the opt-out: silencing the exit code must not + # silence the vacuity notice. + assert "certifies NOTHING" in out + assert "NOT because anything passed" in out + + +def test_opt_out_zero_does_not_open_the_hatch(): + # The regression this file exists for: bare truthiness made "0" enable + # the opt-out, so setting 0 to CLOSE it disabled the policy instead. + rc, _ = _call("0") + assert rc == EXIT_CANNOT_RUN, f"C64_ALLOW_SKIP=0 must not opt out (got {rc})" + + +def test_opt_out_false_does_not_open_the_hatch(): + rc, _ = _call("false") + assert rc == EXIT_CANNOT_RUN, f"=false must not opt out (got {rc})" + + +def test_opt_out_empty_does_not_open_the_hatch(): + rc, _ = _call("") + assert rc == EXIT_CANNOT_RUN, f"empty must not opt out (got {rc})" + + +def test_opt_out_tolerates_surrounding_whitespace(): + # A shell export can carry a stray space or a trailing newline (`export + # C64_ALLOW_SKIP=$(...)` is the usual source of the latter); both are + # still an explicit 1, and the newline is the case .strip() exists for. + for value in (" 1 ", "1\n", "\t1", " 1\n"): + rc, _ = _call(value) + assert rc == EXIT_PASS, (repr(value), rc) + + +def test_opt_out_yes_and_true_are_not_one(): + for value in ("yes", "true", "TRUE", "on", "2", "11"): + rc, _ = _call(value) + assert rc == EXIT_CANNOT_RUN, f"={value} must not opt out (got {rc})" + + +def test_no_opt_out_env_means_no_escape_hatch_at_all(): + # The build-failure and contention sites pass opt_out_env=None. Nothing + # in the environment may rescue them. + buf = io.StringIO() + with _Env("1"): + rc = cannot_run("the build is broken", executed=0, total=1, + certifies="anything", opt_out_env=None, out=buf) + assert rc == EXIT_CANNOT_RUN, rc + # Plain containment. This used to .replace("opt-out honoured", "") first, + # which deleted the exact phrase the assertion was looking for -- so it + # could not fail for the defect it targets. + out = buf.getvalue() + assert "opt-out" not in out, out + assert "COULD NOT RUN" in out + + +# --------------------------------------------------------------------------- +# 2. The two lanes stay distinct. +# --------------------------------------------------------------------------- + +def test_not_applicable_is_a_pass_with_a_named_verdict(): + buf = io.StringIO() + rc = not_applicable("UCI-only (backend=ip65)", + certifies="the dual-overlay dispatcher", out=buf) + assert rc == EXIT_PASS, rc + out = buf.getvalue() + assert "NOT APPLICABLE" in out + assert "certifies NOTHING" in out # quiet, but never silent + assert "COULD NOT RUN" not in out + + +def test_not_applicable_has_no_environment_switch(): + # A voluntary skip is decided by the code's own conditions, so no env + # var may turn it into a failure or vice versa. + for value in (None, "0", "1"): + buf = io.StringIO() + with _Env(value): + rc = not_applicable("wrong backend", certifies="x", out=buf) + assert rc == EXIT_PASS, (value, rc) + + +def test_the_two_exit_codes_are_distinct(): + # 2 must not collapse onto 1: "could not run" and "a check failed" are + # different states and callers are documented to tell them apart. + assert EXIT_CANNOT_RUN == 2 + assert EXIT_PASS == 0 + + +# --------------------------------------------------------------------------- +# 3. The reason string carries its own warning (the pytest -ra channel). +# --------------------------------------------------------------------------- + +def test_reason_text_is_self_contained(): + text = reason_text("ca65 not on PATH", executed=0, total=7, + certifies="the build-flag stamp", opt_out_env=ENV) + # Under pytest -ra this string is the ONLY channel that survives, so + # every part of the warning has to be inside it. + assert "ca65 not on PATH" in text + assert "0 of 7 checks executed" in text + assert "certifies NOTHING about the build-flag stamp" in text + assert ENV in text + assert "\n" not in text, "must stay one line for the -ra summary" + + +def test_require_raises_when_the_prerequisite_is_missing(): + with _Env(None): + try: + require(False, "ca65 not on PATH", executed=0, total=7, + certifies="the build-flag stamp", opt_out_env=ENV) + except SkipPolicyError as exc: + assert "certifies NOTHING" in str(exc) + else: + raise AssertionError("require() did not raise") + + +def test_require_is_silent_when_the_prerequisite_holds(): + with _Env(None): + require(True, "not reached", executed=1, total=1) + + +def test_require_still_raises_when_the_opt_out_is_zero(): + with _Env("0"): + try: + require(False, "ca65 not on PATH", executed=0, total=7, + certifies="the build-flag stamp", opt_out_env=ENV) + except SkipPolicyError: + pass + else: + raise AssertionError("=0 must not suppress the failure") + + +def _standalone() -> int: + """Run every test_* in this module without pytest.""" + tests = [(n, o) for n, o in sorted(globals().items()) + if n.startswith("test_") and callable(o)] + assert tests, "FATAL: no tests found -- a matcher that matches nothing" + failed = 0 + for name, fn in tests: + try: + fn() + print(f" PASS {name}") + except Exception as exc: # noqa: BLE001 + failed += 1 + print(f" FAIL {name}: {exc!r}") + print(f"\n{len(tests) - failed}/{len(tests)} passed") + return 1 if failed else 0 + + +if __name__ == "__main__": + sys.exit(_standalone()) From 8cc2d1ce5a9673f904b606ff7463bc993330bed9 Mon Sep 17 00:00:00 2001 From: JC-000 <3798556+JC-000@users.noreply.github.com> Date: Sat, 5 Sep 2026 18:11:29 -0500 Subject: [PATCH 2/5] skip policy: fix four defects an adversarial review found in #178a All four reproduce; each is fixed with a test that is red before and green after. Two of them are this PR committing the defect class it exists to close, which is the reason to state them plainly rather than fold them in. 1. require()'s opt-out CRASHED outside pytest -- and the hand-rolled copy it supersedes had already fixed this. `import pytest` succeeds whenever pytest is INSTALLED, which on a developer machine is always; it says nothing about pytest DRIVING. So `require()` called `pytest.skip()` in the standalone lane too, and `pytest.outcomes.Skipped` derives from BaseException, which neither new module's `except Exception` runner can catch. Measured, with the sibling harness checkout blocked: C64_ALLOW_SKIP=1 python3 tools/test_rig_skip_contract.py -> Skipped raised out of require(), traceback, EXIT 1, no summary line, and the four source-inspection tests that could have run never ran. Exit 1 means "a check ran and failed" under this module's own contract, so it was wrong in both directions, and only on the escape hatch the PR advertises. Without the opt-out the same run was correct (6 fail, 4 pass, summary printed) -- which is why nothing caught it. tools/test_uci_data_acc.py:631 had already solved exactly this, with `sys.modules.get("pytest")` and a comment saying why: "Only hand the skip to pytest when pytest is actually driving; the standalone runner has its own reporting and must not see Skipped." A shared helper that is WORSE than the copy it supersedes is not a refactor. Fixed by mirroring it: outside pytest, require() now raises VoluntarySkip, a plain Exception the running lane can catch. Either way the test body does not run, so the fail-closed property is unchanged. The old ImportError branch is gone with it: `sys.modules.get` answers "absent" and "not driving" the same way, and both want the same answer. 2. The literal-"1" guard was SKIP-SWALLOWED by the mutation it targets. `test_require_still_raises_when_the_opt_out_is_zero` was spelled `except SkipPolicyError: pass`, which is vacuous against its own defect: revert _opted_out() to bare truthiness and `bool("0")` is true, require() takes the opt-out branch, pytest records SKIPPED, exit 0. Measured under that mutant: `1 skipped`, not a failure. The guard converted itself into a green skip under precisely the defect it guards -- the vacuous-skip shape #178 exists to kill, committed inside #178's own implementation, and it read as caught only because three sibling tests on the cannot_run() lane happen to fail. The catch is now BaseException-wide, because a Skipped here IS the bug. Under the same mutant the module now reports 4 failed, 13 passed. Two new cases pin defect 1 from both sides: the opt-out must raise VoluntarySkip when pytest is not driving, and must still be a pytest skip when it is -- so the fix cannot be "simplified" into never skipping, which would make the documented hatch a lie. 3. UNDISCLOSED LOOSENING: VICE_HTTPS_OK_TO_RUN unset went 2 -> 0. On master, tests/rig_phase3_https_1mhz.py printed "ABORT: VICE_HTTPS_OK_TO_RUN is not set." and returned 2, and its docstring documented `2 -- pre-flight gate refused`. This PR replaced both with not_applicable() -> 0 and deleted that documented code, while the PR body disclosed only the opposite-direction tightening on --u64. The first draft's reasoning ("the operator declining an opt-in rig") does not survive contact with this repo's own taxonomy. UNSET IS THE DEFAULT STATE: it cannot distinguish a considered decline from a forgotten flag, so exit 0 hands a green run to someone who verified nothing. And what the flag asserts -- "the UCI HTTPS listener has stopped" -- is a CONTENTION claim, which is the one category this policy gives no opt-out at all; the port-443 check three lines below is the same interlock measured directly and was already exit 2. Reverted to exit 2, non-opt-out-able. The only thing #178 changes about this gate now is that the verdict is a named block instead of a bare print. Pinned behaviourally, including that C64_ALLOW_SKIP=1 cannot rescue it. 4. tests/rig_phase2_http.py contradicted itself inside forty lines. Its module docstring said tests/rig_vice_https_macos.py "owns the coverage"; its own _COUNTERPART string -- the one that actually reaches the operator -- says that rig drives TLS, "so plaintext HTTP specifically has no macOS rig". The docstring is what a reader hits first, and it is load-bearing here: the voluntary-skip verdict is justified by "another rig owns the coverage, nothing is lost", which for this one rig is false. Coverage of plaintext HTTP over emulated RR-Net really is lost on macOS, at exit 0. Exit 0 stays -- there is no remedy on macOS and exit 2 would be a red nobody can clear -- but the reason is now the true one, in both the docstring and _wrong_platform(), and a new source-inspection test pins it so the two halves cannot drift apart again. 5. Minor, and the same root cause as 1: both _standalone() runners did `return 1 if failed else 0`, collapsing "could not run" onto 1 and contradicting the 0/1/2 contract of the module they test. They now separate VoluntarySkip / SkipPolicyError / Exception and return 0 / 1 / 2 accordingly. Measured on tools/test_rig_skip_contract.py with the sibling harness blocked: exit 2 plain, exit 0 with C64_ALLOW_SKIP=1, exit 0 with the harness present. Red-then-green, all four: D1 restore `import pytest` in require() -> test_require_does_not_hand_a_skip_to_a_non_pytest_runner FAILS D2 restore bare truthiness in _opted_out() -> test_require_still_raises_when_the_opt_out_is_zero FAILS (before this commit the same mutant made it SKIP, exit 0) D3 re-route the interlock to not_applicable() -> test_the_interlock_flag_unset_is_contention_and_stays_exit_two FAILS D4 restore the contradictory docstring -> test_phase2_does_not_claim_a_counterpart_it_does_not_have FAILS Bare `pytest` at the repo root: 83 passed (was 80; +3 new cases). Co-Authored-By: Claude Opus 5 (1M context) --- tests/rig_phase2_http.py | 21 ++++-- tests/rig_phase3_https_1mhz.py | 44 +++++++----- tools/_skip_policy.py | 62 +++++++++++++---- tools/test_rig_skip_contract.py | 104 +++++++++++++++++++++++++--- tools/test_skip_policy.py | 116 ++++++++++++++++++++++++++++++-- 5 files changed, 299 insertions(+), 48 deletions(-) diff --git a/tests/rig_phase2_http.py b/tests/rig_phase2_http.py index 20b24b5..a62ffc2 100644 --- a/tests/rig_phase2_http.py +++ b/tests/rig_phase2_http.py @@ -12,9 +12,14 @@ Exit codes (tools/_skip_policy.py, issue #178): 0 -- PASS 1 -- FAIL (a check ran and failed) - 0 -- NOT APPLICABLE: this rig is Linux-only, and on any other host - tests/rig_vice_https_macos.py owns the coverage. A named verdict, - never a bare skip. + 0 -- NOT APPLICABLE: this rig is Linux-only. A named verdict, never a + bare skip -- and, uniquely among the four bridge rigs, a verdict + that DOES cost coverage: tests/rig_vice_https_macos.py is the + macOS counterpart for the emulated-RR-Net path, but it drives it + over TLS, so plaintext HTTP specifically has no macOS rig. Exit 0 + anyway because there is no remedy on this platform (see + _wrong_platform), not because nothing is lost. Say so out loud + rather than let the exit code imply otherwise. 2 -- COULD NOT RUN (a prerequisite is missing ON LINUX, or the build is broken -- nothing was verified). Set C64_ALLOW_SKIP=1 to accept a prerequisite-missing run as exit 0; a FAILED BUILD is never opted @@ -77,8 +82,14 @@ def _wrong_platform() -> int: The load-bearing question is what the remedy is. "install iproute2" is an involuntary skip and stays exit 2 -- but on a non-Linux host there is - no remedy at all, and another rig owns the coverage, so nothing is lost - and exit 2 would be a red nobody can ever clear. + no remedy at all, so exit 2 would be a red nobody can ever clear. + + Note what this rig does NOT get to say, and what its three siblings do: + that another rig owns the coverage. For DHCP and for HTTPS the macOS + rig genuinely re-runs the same path; for PLAINTEXT HTTP over emulated + RR-Net it does not, so this verdict really does lose coverage on macOS. + That is disclosed here and in _COUNTERPART rather than smoothed over -- + a voluntary skip may be quiet, but it may not misdescribe what it costs. """ return not_applicable( f"this rig is Linux-only (br-c64 bridge + netfilter + /proc/net/udp); " diff --git a/tests/rig_phase3_https_1mhz.py b/tests/rig_phase3_https_1mhz.py index eba0239..beb7318 100644 --- a/tests/rig_phase3_https_1mhz.py +++ b/tests/rig_phase3_https_1mhz.py @@ -22,17 +22,16 @@ Exit codes (tools/_skip_policy.py, issue #178): 0 -- PASS - 0 -- NOT APPLICABLE, as a named verdict rather than a bare skip, in two - cases: this rig is Linux-only, and on any other host - tests/rig_vice_https_macos.py owns the coverage; or - VICE_HTTPS_OK_TO_RUN is unset, which is the operator declining an - opt-in rig. Neither loses coverage. + 0 -- NOT APPLICABLE, as a named verdict rather than a bare skip: this + rig is Linux-only, and on any other host + tests/rig_vice_https_macos.py owns the coverage. 1 -- FAIL (a check ran and failed) 2 -- COULD NOT RUN (a prerequisite is missing ON LINUX, the build is - broken, or port 443 is held by another run -- nothing was - verified). Set C64_ALLOW_SKIP=1 to accept a prerequisite-missing - run as exit 0; a FAILED BUILD and CONTENTION are never opted out - of. + broken, or the rig is interlocked -- VICE_HTTPS_OK_TO_RUN unset, + or port 443 held by another run. Nothing was verified). Set + C64_ALLOW_SKIP=1 to accept a prerequisite-missing run as exit 0; + a FAILED BUILD and CONTENTION are never opted out of, and the + pre-flight gate is contention. """ from __future__ import annotations @@ -611,18 +610,29 @@ def main() -> int: # --- Pre-flight gate (must run BEFORE BridgeEnv, which mutates host # netfilter via sudo). Refuses if the UCI HTTPS listener is still up. # - # An unset opt-in flag is the operator DECLINING to run this rig: a - # voluntary skip, exit 0 with a named verdict. It is not a coverage - # hole -- nothing was lost and nothing is broken (issue #178). + # This flag is a CONTENTION INTERLOCK, not a taste setting, so an unset + # flag stays exit 2 exactly as it was before #178 -- the verdict is now + # a named block instead of a bare print, and that is the whole change. + # + # An earlier draft called it "the operator declining an opt-in rig" and + # routed it to not_applicable() (exit 0). That was wrong in this repo's + # own terms. UNSET IS THE DEFAULT STATE: it cannot distinguish "I + # considered this and declined" from "I forgot", so exit 0 hands a green + # run to someone who verified nothing -- the class #178 exists to close, + # committed by #178's own implementation. What the flag actually + # asserts is "the UCI HTTPS listener has fully stopped", which is a + # contention claim, and contention is the one category this policy gives + # no opt-out at all -- see the port-443 check immediately below, which is + # the same interlock measured directly rather than asserted. if os.environ.get("VICE_HTTPS_OK_TO_RUN") != "1": - return not_applicable( - "VICE_HTTPS_OK_TO_RUN is not set -- this rig is opt-in because it\n" - " collides with the UCI HTTPS listener (which binds 443/4433 on\n" - " the LAN interface); the U64E test may still be running.\n" + return _cannot_run( + "VICE_HTTPS_OK_TO_RUN is not set -- this rig is interlocked because\n" + " it collides with the UCI HTTPS listener (which binds 443/4433\n" + " on the LAN interface); the U64E test may still be running.\n" " To run it, wait for that to finish, then:\n" " sudo env VICE_HTTPS_OK_TO_RUN=1 PYTHONPATH=tools \\\n" " python3 tests/rig_phase3_https_1mhz.py", - certifies=_CERTIFIES, + opt_out=False, ) # A held port is CONTENTION, the third category: the rig exists, someone diff --git a/tools/_skip_policy.py b/tools/_skip_policy.py index e1429c3..ec9cf7e 100644 --- a/tools/_skip_policy.py +++ b/tools/_skip_policy.py @@ -48,6 +48,10 @@ that, an exit-0 opt-out reads as a bare ``3 skipped`` and the warning is gone. +``require()`` decides that by asking whether pytest is DRIVING +(``sys.modules``), never whether it is importable: outside pytest it raises +``VoluntarySkip``, which a standalone runner can actually catch. + Usage (script lane):: from _skip_policy import cannot_run, not_applicable @@ -90,6 +94,7 @@ def test_something(): "EXIT_FAIL", "EXIT_CANNOT_RUN", "SkipPolicyError", + "VoluntarySkip", "reason_text", "cannot_run", "not_applicable", @@ -109,6 +114,22 @@ class SkipPolicyError(AssertionError): """ +class VoluntarySkip(Exception): + """Raised by require() when the opt-out is honoured and pytest is NOT driving. + + Deliberately an ``Exception`` and not a ``pytest.outcomes.Skipped``. + ``Skipped`` derives from ``BaseException``, so a standalone runner's + ``except Exception`` cannot catch it: the process dies mid-suite, prints + no summary, runs none of the remaining tests, and exits 1 -- which under + this module's own contract means "a check ran and failed". Raising a + plain Exception instead keeps the escape hatch catchable by the lane that + is actually running. + + A caller that does not catch it still does not run the test body, so the + fail-closed property is unchanged; only the reporting differs. + """ + + def _opted_out(opt_out_env: Optional[str]) -> bool: """True only if `opt_out_env` names a variable whose value is exactly "1". @@ -251,9 +272,17 @@ def require( full ``reason_text()`` -- pytest records a FAILURE, and the reason string is self-contained because module stdout does not survive. - If ``opt_out_env`` is set to exactly "1" it calls ``pytest.skip()`` - with the same self-contained string instead, so the ``-ra`` summary still - carries the vacuity warning rather than a bare "skipped". + If ``opt_out_env`` is set to exactly "1" the escape hatch opens, and how + it is reported depends on who is DRIVING, not on what is installed: + + * pytest driving -- ``pytest.skip()`` with the same self-contained + string, so the ``-ra`` summary still carries the vacuity warning + rather than a bare "skipped". + * anything else -- :class:`VoluntarySkip`, which a plain + ``except Exception`` can catch. See that class for why handing a + ``Skipped`` to a non-pytest runner is a bug, not a shortcut. + + Either way the test body does not run. """ if condition: return @@ -265,16 +294,21 @@ def require( opt_out_env=opt_out_env, ) if _opted_out(opt_out_env): - try: - import pytest # noqa: PLC0415 - optional, only needed on this branch - except ImportError: - # No pytest, so there is no skip to record -- and returning would - # be the worst answer available: the caller would carry on into a - # test body whose prerequisite is missing, which is the vacuous - # pass this module exists to prevent. Fail closed instead. - raise SkipPolicyError( - f"{text} [{opt_out_env}=1 is set, but pytest is not installed, " - "so the skip cannot be recorded; failing closed rather than " - "running the body as if the prerequisite held]") + # Hand the skip to pytest ONLY when pytest is actually DRIVING this + # run -- `sys.modules`, not `import`. `import pytest` succeeds + # whenever pytest is merely INSTALLED, which on a developer machine + # is always, and `pytest.skip()` raises `pytest.outcomes.Skipped`, + # a BaseException. A standalone runner's `except Exception` cannot + # catch that, so the whole suite died mid-run at exit 1 with no + # summary and the remaining tests never executed. + # + # tools/test_uci_data_acc.py's hand-rolled copy of this policy + # already had this right, with the same `sys.modules.get("pytest")` + # and the same reason: "the standalone runner has its own reporting + # and must not see Skipped". This helper exists to supersede that + # copy, so it has to be at least as correct as the thing it replaces. + pytest = sys.modules.get("pytest") + if pytest is None: + raise VoluntarySkip(f"{text} [{opt_out_env}=1 set]") pytest.skip(f"{text} [{opt_out_env}=1 set]") raise SkipPolicyError(text) diff --git a/tools/test_rig_skip_contract.py b/tools/test_rig_skip_contract.py index 944a197..18e7b11 100644 --- a/tools/test_rig_skip_contract.py +++ b/tools/test_rig_skip_contract.py @@ -49,6 +49,7 @@ SHAPE only. """ +import ast import io import os import sys @@ -61,7 +62,14 @@ if _p not in sys.path: sys.path.insert(0, _p) -from _skip_policy import require # noqa: E402 +from _skip_policy import ( # noqa: E402 + EXIT_CANNOT_RUN, + EXIT_FAIL, + EXIT_PASS, + SkipPolicyError, + VoluntarySkip, + require, +) # tools/https_e2e is NOT imported at module level. Its __init__ re-exports # from .vice_on_bridge, which imports c64_test_harness from a sibling @@ -119,6 +127,11 @@ def _https_e2e(): MACOS_RIG = os.path.join(_TESTS, "rig_vice_https_macos.py") +# The one bridge rig whose voluntary skip really does cost coverage: the +# macOS rig drives the emulated-RR-Net path over TLS, so plaintext HTTP has +# no counterpart there. Named here so the claim is asserted, not folklore. +PLAINTEXT_ONLY_RIG = os.path.join(_TESTS, "rig_phase2_http.py") + class Tripwire(AssertionError): """Raised when a rig reaches a step an ordering test must never reach.""" @@ -300,22 +313,74 @@ def test_bridge_rigs_route_a_broken_build_to_two_without_an_opt_out(): assert rc == 2, f"{name}: broken build must be 2 even opted out, got {rc}\n{out}" -def test_the_opt_in_rig_treats_an_unset_flag_as_not_applicable(): - """VICE_HTTPS_OK_TO_RUN unset is the operator declining: exit 0.""" +def test_the_interlock_flag_unset_is_contention_and_stays_exit_two(): + """VICE_HTTPS_OK_TO_RUN unset must NOT be laundered into a pass. + + This is the one gate where #178's own first draft got the taxonomy + backwards: it read an unset flag as "the operator declined an opt-in + rig" and returned 0, silently loosening the exit 2 this rig had on + master. Unset is the DEFAULT state, so it cannot tell a considered + decline from a forgotten flag -- and the flag asserts "the UCI HTTPS + listener has stopped", which is a contention claim. Contention is the + category with no opt-out at all. + + Pinned behaviourally, and pinned as NOT opt-out-able, so neither half + can be quietly reverted. + """ mod = _import_rig("rig_phase3_https_1mhz") with _Patched(_https_e2e(), platform_supported=lambda: True, check_prerequisites=_tripwire("check_prerequisites")).env( VICE_HTTPS_OK_TO_RUN=None): rc, out = _run_main(mod) - assert rc == 0, f"unset opt-in flag must be exit 0, got {rc}\n{out}" - assert "NOT APPLICABLE" in out, out + assert rc == 2, f"unset interlock must be exit 2, got {rc}\n{out}" + assert "COULD NOT RUN" in out, out + assert "NOT APPLICABLE" not in out, out assert "VICE_HTTPS_OK_TO_RUN" in out, out + # ...and no environment variable may rescue it, unlike the + # missing-prerequisite lane three lines further down the rig. + with _Patched(_https_e2e(), platform_supported=lambda: True, + check_prerequisites=_tripwire("check_prerequisites")).env( + VICE_HTTPS_OK_TO_RUN=None, C64_ALLOW_SKIP="1"): + rc, out = _run_main(mod) + assert rc == 2, f"contention must not be opt-out-able, got {rc}\n{out}" + # --------------------------------------------------------------------------- # tests/rig_vice_https_macos.py -- source inspection only (see module docstring). # --------------------------------------------------------------------------- +def test_phase2_does_not_claim_a_counterpart_it_does_not_have(): + """rig_phase2_http.py must not tell an operator its coverage is covered. + + The file used to contradict itself inside 40 lines: its module docstring + said "tests/rig_vice_https_macos.py owns the coverage", while its own + _COUNTERPART string -- the one that actually reaches the operator at + runtime -- said that rig drives TLS, "so plaintext HTTP specifically has + no macOS rig". The docstring is what a reader hits first. + + This matters past tidiness: the voluntary-skip verdict is justified by + "another rig owns the coverage, nothing is lost", and for this one rig + that is false. Exit 0 is still right (there is no remedy on macOS) but + the reason has to be the true one, or the exit code quietly overstates + what was verified -- the same defect class this module exists to catch. + """ + with open(PLAINTEXT_ONLY_RIG, "r", encoding="utf-8") as fh: + src = fh.read() + + # The runtime string is the anchor: assert it says what we think, so + # this test cannot pass because the string was silently reworded. + assert "no macOS rig" in src, ( + "rig_phase2_http.py must state that plaintext HTTP has no macOS " + "counterpart; the anchor string is gone") + + doc = ast.get_docstring(ast.parse(src)) or "" + assert doc, "rig_phase2_http.py lost its module docstring" + assert "owns the coverage" not in doc, ( + "rig_phase2_http.py's docstring claims another rig owns its " + "coverage, which its own _COUNTERPART denies:\n" + doc) + + def _macos_source(): with open(MACOS_RIG, "r", encoding="utf-8") as fh: return fh.read() @@ -374,19 +439,42 @@ def test_macos_rig_build_failure_exits_two(): def _standalone() -> int: + """Run every test_* in this module without pytest. + + Honours the 0/1/2 contract of the module under test rather than + collapsing "could not run" onto 1. The six https_e2e tests raise + SkipPolicyError when the sibling harness checkout is absent -- that is a + coverage hole (exit 2), not a failed check (exit 1). + """ tests = [(n, o) for n, o in sorted(globals().items()) if n.startswith("test_") and callable(o)] assert tests, "FATAL: no tests found -- a matcher that matches nothing" - failed = 0 + failed = cannot = skipped = 0 for name, fn in tests: try: fn() print(f" PASS {name}") + except VoluntarySkip as exc: + skipped += 1 + print(f" SKIP {name}: {exc}") + except SkipPolicyError as exc: + cannot += 1 + print(f" CANNOT RUN {name}: {exc}") except Exception as exc: # noqa: BLE001 failed += 1 print(f" FAIL {name}: {exc!r}") - print(f"\n{len(tests) - failed}/{len(tests)} passed") - return 1 if failed else 0 + passed = len(tests) - failed - cannot - skipped + tail = "" + if cannot: + tail += f", {cannot} COULD NOT RUN" + if skipped: + tail += f", {skipped} skipped by explicit opt-out" + print(f"\n{passed}/{len(tests)} passed{tail}") + if failed: + return EXIT_FAIL + if cannot: + return EXIT_CANNOT_RUN + return EXIT_PASS if __name__ == "__main__": diff --git a/tools/test_skip_policy.py b/tools/test_skip_policy.py index 718cd0e..567d1a9 100644 --- a/tools/test_skip_policy.py +++ b/tools/test_skip_policy.py @@ -28,8 +28,10 @@ from _skip_policy import ( # noqa: E402 EXIT_CANNOT_RUN, + EXIT_FAIL, EXIT_PASS, SkipPolicyError, + VoluntarySkip, cannot_run, not_applicable, reason_text, @@ -212,31 +214,137 @@ def test_require_is_silent_when_the_prerequisite_holds(): def test_require_still_raises_when_the_opt_out_is_zero(): + """=0 must FAIL, and must not be able to pass by SKIPPING either. + + The obvious spelling of this test -- `except SkipPolicyError: pass` -- + is vacuous against the exact defect it guards. If _opted_out() ever + reverts to bare truthiness, `bool("0")` is true, require() takes the + opt-out branch, calls pytest.skip(), and pytest records this case as + SKIPPED at exit 0. The guard converts itself into a green skip under + precisely the mutation it exists to catch -- the vacuous-skip shape + #178 exists to kill, inside #178's own implementation. + Measured: under that mutant this case was `1 skipped`, not a failure. + + So the catch is BaseException-wide. pytest.outcomes.Skipped derives + from BaseException, not Exception, which is what let it slip past. + """ with _Env("0"): try: require(False, "ca65 not on PATH", executed=0, total=7, certifies="the build-flag stamp", opt_out_env=ENV) except SkipPolicyError: pass + except BaseException as exc: # noqa: BLE001 - a Skipped here IS the bug + raise AssertionError( + f"=0 must fail, not skip or otherwise escape: {exc!r}") from None else: raise AssertionError("=0 must not suppress the failure") +def test_require_does_not_hand_a_skip_to_a_non_pytest_runner(): + """The opt-out must stay catchable by whoever is actually driving. + + require() asks sys.modules, not `import pytest`: pytest being INSTALLED + says nothing about pytest DRIVING, and pytest.skip() raises a + BaseException that a standalone runner's `except Exception` cannot + catch. Before this was fixed, `C64_ALLOW_SKIP=1 python3 + tools/test_rig_skip_contract.py` died mid-suite at exit 1 with no + summary and four runnable tests never run -- an opt-out that reports + as "a check ran and failed". + + Simulated by hiding pytest from sys.modules for the duration; the real + lane is any run of these modules as a script. + """ + saved = sys.modules.pop("pytest", None) + try: + with _Env("1"): + try: + require(False, "ca65 not on PATH", executed=0, total=7, + certifies="the build-flag stamp", opt_out_env=ENV) + except VoluntarySkip as exc: + assert isinstance(exc, Exception), "must be catchable as Exception" + assert "certifies NOTHING" in str(exc), str(exc) + except BaseException as exc: # noqa: BLE001 + raise AssertionError( + f"opt-out outside pytest must raise VoluntarySkip, got " + f"{exc!r}") from None + else: + raise AssertionError( + "opt-out must not RETURN outside pytest -- the caller would " + "run the body as if the prerequisite held") + finally: + if saved is not None: + sys.modules["pytest"] = saved + + +def test_require_hands_a_skip_to_pytest_when_pytest_is_driving(): + """The other half: with pytest driving, it is still a pytest skip. + + Pinned so the sys.modules fix cannot be "simplified" into never + skipping at all, which would make the documented escape hatch a lie. + """ + was_loaded = "pytest" in sys.modules + import pytest # noqa: PLC0415 - this test is about pytest specifically + + try: + with _Env("1"): + try: + require(False, "ca65 not on PATH", executed=0, total=7, + certifies="the build-flag stamp", opt_out_env=ENV) + except VoluntarySkip as exc: + raise AssertionError( + f"pytest is driving; the opt-out must be a pytest skip: " + f"{exc!r}") from None + except BaseException as exc: # noqa: BLE001 + assert type(exc).__name__ == "Skipped", repr(exc) + assert isinstance(exc, pytest.skip.Exception), repr(exc) + else: + raise AssertionError("the opt-out must not RETURN under pytest") + finally: + # Do not leave pytest in sys.modules for a standalone run that did + # not have it there: require() reads sys.modules, so this test would + # otherwise change how every later test in the file behaves. + if not was_loaded: + sys.modules.pop("pytest", None) + + def _standalone() -> int: - """Run every test_* in this module without pytest.""" + """Run every test_* in this module without pytest. + + Honours the module's own 0/1/2 contract rather than collapsing "could + not run" onto 1: a SkipPolicyError is exactly the involuntary skip this + policy calls a 2, and reporting it as 1 ("a check ran and failed") is + the confusion the exit codes exist to prevent. + """ tests = [(n, o) for n, o in sorted(globals().items()) if n.startswith("test_") and callable(o)] assert tests, "FATAL: no tests found -- a matcher that matches nothing" - failed = 0 + failed = cannot = skipped = 0 for name, fn in tests: try: fn() print(f" PASS {name}") + except VoluntarySkip as exc: + skipped += 1 + print(f" SKIP {name}: {exc}") + except SkipPolicyError as exc: + cannot += 1 + print(f" CANNOT RUN {name}: {exc}") except Exception as exc: # noqa: BLE001 failed += 1 print(f" FAIL {name}: {exc!r}") - print(f"\n{len(tests) - failed}/{len(tests)} passed") - return 1 if failed else 0 + passed = len(tests) - failed - cannot - skipped + tail = "" + if cannot: + tail += f", {cannot} COULD NOT RUN" + if skipped: + tail += f", {skipped} skipped by explicit opt-out" + print(f"\n{passed}/{len(tests)} passed{tail}") + if failed: + return EXIT_FAIL + if cannot: + return EXIT_CANNOT_RUN + return EXIT_PASS if __name__ == "__main__": From 76621b677882f36dfd9a9e02114cc55dfa9aba12 Mon Sep 17 00:00:00 2001 From: JC-000 <3798556+JC-000@users.noreply.github.com> Date: Sat, 5 Sep 2026 18:28:40 -0500 Subject: [PATCH 3/5] skip policy: close the one exit code that DID get weaker, and test D5 Review round 2. One real hole in a whole-PR claim, plus the fix from round 1 that shipped without a test of its own. F1. "No exit code in this PR is weaker than master's" was FALSE by omission, at tools/test_ecdsa_p384_kat.py -- the one wired file the round-1 sweep did not touch, so it was never re-checked. Master, `--u64` with U64_HOST unset: prints a SKIP line inside the u64 branch, but the VICE lane has ALREADY RUN, and main() ends at `return 0 if total_fail == 0 else 1`. A failing emulator vector therefore reported 1. This branch: the new gate fires before _build_prg() and before any VICE work, and carried opt_out_env="C64_ALLOW_SKIP". With that set it returned 0 with nothing run at all. The defect is SCOPE, not arithmetic. C64_ALLOW_SKIP answers "this lane has no hardware" -- which is precisely the operator who would set it here -- and honouring it at a gate that precedes the VICE lane silences the EMULATOR half too, which needs no hardware and could have run. They get exit 0 and never learn the emulator-only half regressed. Fixed with opt_out_env=None rather than by moving the gate: the up-front refusal is the disclosed tightening (~0.1 s instead of hours), and moving it below the VICE lane would give that back. The remedy needs no environment variable and costs nothing -- set U64_HOST, or drop --u64 and get the VICE lane on its own. "This lane has no hardware" is spelled by NOT PASSING --u64; asking for hardware and not supplying it is a malformed invocation, not a coverage gap to acknowledge. The unreachable backstop later in main() gets the same treatment, so one file does not answer the same question two ways. Severity was low -- three deliberate operator choices, the block prints in full, and _build_prg() would likely fail first because the P-384 build has never completed -- but the claim was checkable and did not hold, so the code moved rather than the claim. F3. D5 (the _standalone() 0/1/2 split) was the one round-1 fix with no test. Nothing collects either runner, so a future edit collapsing it back to `return 1 if failed else 0` would go unnoticed -- in a PR whose thesis is that a helper with no call-site test is just another convention to miss. Two subprocess cases in tools/test_skip_policy.py drive the REAL runner in tools/test_rig_skip_contract.py with c64_test_harness made unimportable by a meta_path finder (the sibling is pip-installed, so there is no path entry to remove). Target chosen deliberately: it is the module with a genuine could-not-run lane, and driving it from here cannot recurse, because it has no spawner of its own. no opt-out -> exit 2, "CANNOT RUN" and the vacuity string both present, summary line still printed C64_ALLOW_SKIP=1 -> exit 0, no traceback, "skipped by explicit opt-out", and the four tests that never needed the sibling checkout still show PASS That second case is also the D1 crash scenario, now pinned: before the sys.modules fix this exact invocation died with an uncaught pytest Skipped at exit 1. F2. VoluntarySkip is an Exception, so a consumer's broad `except Exception` can swallow it and label it a failure. Not reachable today -- every require() consumer names VoluntarySkip before the bare except -- and the direction is the safe one, since a swallowed voluntary skip reports as a FAILURE and never as a pass. One docstring paragraph says so, and tells a consumer to name it first. Deliberately NOT a BaseException: that is the choice that caused D1. Red-then-green: F1 restore opt_out_env="C64_ALLOW_SKIP" on the --u64 gate -> test_the_u64_gate_is_not_opt_out_able_because_it_precedes_the_vice_lane FAILS (1 failed, 11 passed) D5 collapse the runner to `return 1 if failed or cannot else 0` -> test_standalone_runner_returns_two_for_could_not_run FAILS (1 failed, 18 passed) Bare `pytest` at the repo root: 86 passed (was 83; +3 new cases). Co-Authored-By: Claude Opus 5 (1M context) --- tools/_skip_policy.py | 9 ++++ tools/test_ecdsa_p384_kat.py | 43 ++++++++++++++---- tools/test_rig_skip_contract.py | 40 +++++++++++++++++ tools/test_skip_policy.py | 79 +++++++++++++++++++++++++++++++++ 4 files changed, 162 insertions(+), 9 deletions(-) diff --git a/tools/_skip_policy.py b/tools/_skip_policy.py index ec9cf7e..147af92 100644 --- a/tools/_skip_policy.py +++ b/tools/_skip_policy.py @@ -127,6 +127,15 @@ class VoluntarySkip(Exception): A caller that does not catch it still does not run the test body, so the fail-closed property is unchanged; only the reporting differs. + + CONSUMERS: being an ``Exception`` makes this SWALLOWABLE BY DESIGN, so a + broad ``except Exception`` will catch it and label it whatever that + handler labels things. Name it explicitly, BEFORE the bare + ``except Exception``, as both runners in this repo do. The direction of + the hazard is the safe one -- a swallowed voluntary skip reports as a + FAILURE, never as a pass -- which is why it is worth one line of warning + rather than a BaseException, the choice that caused the bug this class + exists to fix. """ diff --git a/tools/test_ecdsa_p384_kat.py b/tools/test_ecdsa_p384_kat.py index f9c7530..ce8b3c2 100644 --- a/tools/test_ecdsa_p384_kat.py +++ b/tools/test_ecdsa_p384_kat.py @@ -51,14 +51,17 @@ Exit codes (tools/_skip_policy.py, issue #178): 0 PASS 1 FAIL (a vector ran and failed) - 2 COULD NOT RUN -- a requested prerequisite is missing (e.g. --u64 with - no U64_HOST) or no vector produced a verdict. Set C64_ALLOW_SKIP=1 to - accept such a run as exit 0. Not passing --u64 at all is a VOLUNTARY - skip and still exits 0. + 2 COULD NOT RUN -- --u64 was requested with no U64_HOST, so nothing ran. + NOT opt-out-able: honouring C64_ALLOW_SKIP here would fire before the + VICE lane and silence the emulator half too, which needs no hardware. + The remedy is free: set U64_HOST, or drop --u64 and get the VICE lane + on its own. Not passing --u64 at all is a VOLUNTARY skip, exits 0, + and is how you say "this lane has no hardware". Environment: C64_SKIP_BUILD=1 Reuse existing build artifacts (skip make). - C64_ALLOW_SKIP=1 Accept a could-not-run as exit 0 (prints why). + C64_ALLOW_SKIP=1 Honoured by the rig lane; NOT by the --u64 + gate above (see its comment in main()). U64_HOST= Ultimate 64 host (default 192.168.1.81). P384_KAT_VICE_TIMEOUT_S Per-VERIFY-step timeout under VICE (default 1800 s = 30 min). @@ -807,13 +810,30 @@ def main() -> int: # lane cannot run at all, and the tally below would otherwise add 0/0 to # the VICE result and print OVERALL: PASS (issue #178). Refuse up front, # before the multi-hour VICE lane, rather than at the verdict. + # + # NO OPT-OUT, and this gate is the reason the rule needs stating: the + # opt-out here would be scoped WRONG. C64_ALLOW_SKIP answers "this lane + # has no hardware", but firing before the VICE lane means honouring it + # also silences the EMULATOR half -- which needs no hardware and could + # have run. Measured against master: `--u64` with U64_HOST unset ran the + # whole VICE lane and returned 1 on a failing vector; with the opt-out + # honoured here that became exit 0, so an operator who set + # C64_ALLOW_SKIP=1 for the honest reason (no U64E on this lane) would + # never learn the emulator-only half had regressed. + # + # The remedy costs nothing and needs no environment variable: set + # U64_HOST, or drop --u64 and get the VICE lane on its own. "This lane + # has no hardware" is spelled by NOT PASSING --u64; asking for hardware + # and not supplying it is a malformed invocation, not a coverage gap to + # be acknowledged. if run_u64 and not os.environ.get("U64_HOST"): return cannot_run( - "--u64 requested but U64_HOST is not set in the environment", + "--u64 requested but U64_HOST is not set in the environment" + " -- drop --u64 to run the VICE lane alone", executed=0, total=1, certifies="the P-384 verify path on real hardware", - opt_out_env="C64_ALLOW_SKIP", + opt_out_env=None, ) # Check the upstream test vector file exists. @@ -869,12 +889,17 @@ def main() -> int: # Unreachable: the early gate in main() already refused. Kept as # a backstop, and it must NOT leave a 0/0 lane -- that is exactly # what the tally used to read as OVERALL: PASS (issue #178). + # Same no-opt-out rule as that gate, and for the same reason: + # one file must not answer the same question two ways, or a + # later reader "fixes" the inconsistency in whichever direction + # they happen to notice first. return cannot_run( - "--u64 requested but U64_HOST is not set in the environment", + "--u64 requested but U64_HOST is not set in the environment" + " -- drop --u64 to run the VICE lane alone", executed=v_pass + v_fail, total=v_pass + v_fail + len(vectors), certifies="the P-384 verify path on real hardware", - opt_out_env="C64_ALLOW_SKIP", + opt_out_env=None, ) else: try: diff --git a/tools/test_rig_skip_contract.py b/tools/test_rig_skip_contract.py index 18e7b11..75dd728 100644 --- a/tools/test_rig_skip_contract.py +++ b/tools/test_rig_skip_contract.py @@ -52,6 +52,7 @@ import ast import io import os +import subprocess import sys from contextlib import redirect_stdout @@ -127,6 +128,8 @@ def _https_e2e(): MACOS_RIG = os.path.join(_TESTS, "rig_vice_https_macos.py") +P384_KAT = os.path.join(_HERE, "test_ecdsa_p384_kat.py") + # The one bridge rig whose voluntary skip really does cost coverage: the # macOS rig drives the emulated-RR-Net path over TLS, so plaintext HTTP has # no counterpart there. Named here so the claim is asserted, not folklore. @@ -350,6 +353,43 @@ def test_the_interlock_flag_unset_is_contention_and_stays_exit_two(): # tests/rig_vice_https_macos.py -- source inspection only (see module docstring). # --------------------------------------------------------------------------- +def test_the_u64_gate_is_not_opt_out_able_because_it_precedes_the_vice_lane(): + """--u64 with no U64_HOST must be exit 2 even under C64_ALLOW_SKIP=1. + + This gate is the one place in this PR where honouring the opt-out would + make an exit code WEAKER than master's, and the reason is scope rather + than arithmetic. On master, `--u64` with U64_HOST unset still ran the + entire VICE lane and returned `0 if total_fail == 0 else 1`, so a failing + emulator vector reported 1. The gate added here fires BEFORE + _build_prg() and before any VICE work, so an honoured opt-out returns 0 + with nothing run at all. + + C64_ALLOW_SKIP answers "this lane has no hardware" -- which is exactly + the operator who would set it here -- yet it would silence the EMULATOR + half, which needs no hardware. The remedy costs nothing and needs no + variable: set U64_HOST, or drop --u64. Pinned as a subprocess because + the exit CODE is the whole contract. + + Cheap by construction: the gate returns before the vector file check, + before _build_prg(), and before VICE, and the module imports nothing but + stdlib and _skip_policy -- so this runs in well under a second and + touches no hardware. + """ + env = dict(os.environ) + env.pop("U64_HOST", None) + env["C64_ALLOW_SKIP"] = "1" + proc = subprocess.run([sys.executable, P384_KAT, "--u64"], + capture_output=True, text=True, cwd=_REPO, env=env, + timeout=120) + out = proc.stdout + proc.stderr + assert proc.returncode == 2, ( + f"--u64 with no U64_HOST must be exit 2 even opted out, got " + f"{proc.returncode}\n{out}") + assert "COULD NOT RUN" in out, out + # ...and it must not advertise a hatch it does not honour. + assert "opt-out honoured" not in out, out + + def test_phase2_does_not_claim_a_counterpart_it_does_not_have(): """rig_phase2_http.py must not tell an operator its coverage is covered. diff --git a/tools/test_skip_policy.py b/tools/test_skip_policy.py index 567d1a9..f52d2c6 100644 --- a/tools/test_skip_policy.py +++ b/tools/test_skip_policy.py @@ -20,7 +20,9 @@ import io import os +import subprocess import sys +import textwrap _HERE = os.path.dirname(os.path.abspath(__file__)) if _HERE not in sys.path: @@ -308,6 +310,83 @@ def test_require_hands_a_skip_to_pytest_when_pytest_is_driving(): sys.modules.pop("pytest", None) +# --------------------------------------------------------------------------- +# 4. The standalone runner honours the 0/1/2 contract. +# +# Driven as a SUBPROCESS, because the exit code is the whole contract and +# pytest never collects _standalone() at all. The target is +# tools/test_rig_skip_contract.py rather than this module: it is the one with +# a genuine could-not-run lane (six tests need the sibling c64_test_harness +# checkout), and driving it from here cannot recurse -- that module has no +# spawner of its own. +# --------------------------------------------------------------------------- + +_RIG_CONTRACT = os.path.join(_HERE, "test_rig_skip_contract.py") + +# Run a script with c64_test_harness made unimportable, so the six +# https_e2e-dependent tests take the require() path. A meta_path finder, +# not a PYTHONPATH trick: the sibling is pip-installed, so there is no path +# entry to remove. +_BLOCK_AND_RUN = textwrap.dedent(""" + import runpy, sys + class _Block: + def find_spec(self, name, path=None, target=None): + if name == "c64_test_harness" or name.startswith("c64_test_harness."): + raise ImportError("blocked by tools/test_skip_policy.py") + return None + sys.meta_path.insert(0, _Block()) + sys.argv = [sys.argv[1]] + runpy.run_path(sys.argv[0], run_name="__main__") +""") + + +def _run_blocked(script, **env_over): + env = dict(os.environ) + for k, v in env_over.items(): + if v is None: + env.pop(k, None) + else: + env[k] = v + return subprocess.run([sys.executable, "-c", _BLOCK_AND_RUN, script], + capture_output=True, text=True, timeout=180, + cwd=os.path.dirname(_HERE), env=env) + + +def test_standalone_runner_returns_two_for_could_not_run(): + """"Could not run" must not be reported as 1, which means "a check failed". + + Nothing collects _standalone(), so this is the only thing standing + between the 0/1/2 split and a future edit collapsing it back to + `return 1 if failed else 0` -- in a PR whose thesis is that a helper + with no call-site test is just another convention to miss. + """ + proc = _run_blocked(_RIG_CONTRACT, C64_ALLOW_SKIP=None) + out = proc.stdout + proc.stderr + assert proc.returncode == EXIT_CANNOT_RUN, ( + f"a missing prerequisite must be {EXIT_CANNOT_RUN}, not " + f"{proc.returncode}\n{out}") + assert "CANNOT RUN" in out, out + assert "COULD NOT RUN" in out, out # the vacuity string survives + assert "passed" in out, "the summary line must still be printed\n" + out + + +def test_standalone_runner_returns_zero_when_the_opt_out_is_honoured(): + """The opt-out lane, which is where defect D1 crashed the whole runner. + + Before the sys.modules fix this exact invocation died with an uncaught + pytest Skipped: exit 1, no summary, and the four tests that did not need + the sibling checkout never ran. + """ + proc = _run_blocked(_RIG_CONTRACT, C64_ALLOW_SKIP="1") + out = proc.stdout + proc.stderr + assert proc.returncode == EXIT_PASS, ( + f"an honoured opt-out must be {EXIT_PASS}, got {proc.returncode}\n{out}") + assert "Traceback" not in out, "the opt-out must not raise\n" + out + assert "skipped by explicit opt-out" in out, out + # The tests that did NOT need the sibling checkout must still have run. + assert "PASS test_macos_rig" in out, out + + def _standalone() -> int: """Run every test_* in this module without pytest. From 4e90ffcc98c80140695e2e1363846fb4ba8ee834 Mon Sep 17 00:00:00 2001 From: JC-000 <3798556+JC-000@users.noreply.github.com> Date: Sat, 5 Sep 2026 18:36:55 -0500 Subject: [PATCH 4/5] docs: the "cannot recurse" rationale was false in the commit that wrote it 76621b6's comment on the new subprocess cases says the target module "has no spawner of its own", so driving it from here cannot recurse. True when I reasoned it; false by the time the same commit was written, because that commit ALSO added a spawner to tools/test_rig_skip_contract.py -- test_the_u64_gate_* runs tools/test_ecdsa_p384_kat.py, and _standalone() runs every test_*. So the real chain is depth 2 and it fires: pytest -> test_rig_skip_contract.py (spawned by test_skip_policy.py) -> test_ecdsa_p384_kat.py (spawned by its u64-gate test) Measured rather than argued: inside the blocked standalone run the u64-gate case reports PASS, which it can only do by having spawned the KAT. The nested run is 0.065 s end to end and exits 2, because the KAT's gate returns before _build_prg() and before any VICE work. Timeouts nest correctly, 180 s outer and 120 s inner. So: no fork bomb, no hang, nothing to fix in the code. What needed fixing is that the comment leaned on an invariant -- "no spawner exists" -- which is exactly the property that would stop a third link being added later, and which was no longer true. A safety argument that has quietly become false is worse than no argument, because the next person reads it and stops looking. The comment now spells the chain out, states the depth, says the bound is by INSPECTION with no guard in the code, gives the measured cost and the nesting of the timeouts, and records that the earlier claim went stale inside one commit -- so a third link has to be added deliberately, in sight of all that. Comment-only. Bare `pytest`: 86 passed, unchanged. Co-Authored-By: Claude Opus 5 (1M context) --- tools/test_skip_policy.py | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/tools/test_skip_policy.py b/tools/test_skip_policy.py index f52d2c6..3a30735 100644 --- a/tools/test_skip_policy.py +++ b/tools/test_skip_policy.py @@ -317,8 +317,26 @@ def test_require_hands_a_skip_to_pytest_when_pytest_is_driving(): # pytest never collects _standalone() at all. The target is # tools/test_rig_skip_contract.py rather than this module: it is the one with # a genuine could-not-run lane (six tests need the sibling c64_test_harness -# checkout), and driving it from here cannot recurse -- that module has no -# spawner of its own. +# checkout). +# +# SPAWN DEPTH IS 2, AND BOUNDED BY INSPECTION ONLY -- there is no guard in the +# code, so read this before adding a third link. The full chain is: +# +# pytest +# -> test_rig_skip_contract.py (this file spawns it, below) +# -> test_ecdsa_p384_kat.py (its test_the_u64_gate_* spawns THAT, +# and _standalone() runs every test_*, +# so the link really does fire here) +# +# It terminates because the P-384 KAT spawns nothing, and it is cheap: the +# whole nested run measures ~0.07 s, because the KAT's gate returns before +# _build_prg() and before any VICE work. Timeouts nest correctly -- 180 s on +# the outer call here, 120 s on the inner one there. +# +# An earlier version of this comment claimed the target "has no spawner of its +# own". That was true when it was written and false in the same commit, which +# is the whole reason the chain is spelled out rather than asserted: a third +# link would have to be added deliberately, in sight of this. # --------------------------------------------------------------------------- _RIG_CONTRACT = os.path.join(_HERE, "test_rig_skip_contract.py") From 4251c17f7500272421f9f87f95431400be76c6d1 Mon Sep 17 00:00:00 2001 From: JC-000 <3798556+JC-000@users.noreply.github.com> Date: Sat, 5 Sep 2026 18:45:40 -0500 Subject: [PATCH 5/5] docs: the KAT is not a leaf in general, and that coupling has an owner 4e90ffc said the spawn chain "terminates because the P-384 KAT spawns nothing". Not true in general: _build_prg() runs `make clean` and two `make BACKEND=uci` invocations at test_ecdsa_p384_kat.py:646-665. The next sentence already gave the real reason -- the --u64 gate returns before _build_prg() -- so the load-bearing fact was present and the depth, cost and timeout figures were all correct. What was wrong is the attribution, and it matters because of what it hides. The KAT is a leaf ON THIS PATH ONLY, and the path is chosen by where that gate sits. So the cost bound of the D5 subprocess test depends on the F1 fix keeping the gate up front. Move it below _build_prg() -- the alternative fix considered and rejected when the gate was made non-opt-out-able -- and the nested run starts a full `make clean && make` inside a 120 s inner timeout. Two decisions, one line apart in a file that mentions neither, and nothing at the gate would tell you. Both halves now say so: - tools/test_skip_policy.py's chain comment names the KAT's `make` calls with their line numbers, says the leaf property is a property of the PATH and not of the file, and states outright that moving the gate is also a decision about this test's cost. - tools/test_rig_skip_contract.py -- which ADDS the second link and carried no note at all, the comment living only in the module that adds the first -- gets the chain drawn at the spawn site, with the same warning and a pointer to the fuller note. A cross-file invariant documented in exactly one of the two files is half-documented: the reader who breaks it is the one editing the other. Comment-only. Bare `pytest`: 86 passed, unchanged. Co-Authored-By: Claude Opus 5 (1M context) --- tools/test_rig_skip_contract.py | 13 +++++++++++++ tools/test_skip_policy.py | 20 ++++++++++++++++---- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/tools/test_rig_skip_contract.py b/tools/test_rig_skip_contract.py index 75dd728..cf51c68 100644 --- a/tools/test_rig_skip_contract.py +++ b/tools/test_rig_skip_contract.py @@ -374,6 +374,19 @@ def test_the_u64_gate_is_not_opt_out_able_because_it_precedes_the_vice_lane(): before _build_prg(), and before VICE, and the module imports nothing but stdlib and _skip_policy -- so this runs in well under a second and touches no hardware. + + THIS SPAWN IS THE SECOND LINK IN A TWO-DEEP CHAIN, and the depth is + bounded by inspection, not by a guard: + + pytest -> test_rig_skip_contract.py -> test_ecdsa_p384_kat.py + + tools/test_skip_policy.py spawns THIS module (its section 4 drives + _standalone() as a subprocess), _standalone() runs every test_*, so this + case runs nested and really does spawn the KAT. The KAT is a leaf only + because its gate returns before _build_prg(), which would otherwise run + `make clean` and two `make` invocations inside the 120 s timeout below. + Read the chain comment in tools/test_skip_policy.py before adding a third + link, or before moving that gate. """ env = dict(os.environ) env.pop("U64_HOST", None) diff --git a/tools/test_skip_policy.py b/tools/test_skip_policy.py index 3a30735..08e1f89 100644 --- a/tools/test_skip_policy.py +++ b/tools/test_skip_policy.py @@ -328,10 +328,22 @@ def test_require_hands_a_skip_to_pytest_when_pytest_is_driving(): # and _standalone() runs every test_*, # so the link really does fire here) # -# It terminates because the P-384 KAT spawns nothing, and it is cheap: the -# whole nested run measures ~0.07 s, because the KAT's gate returns before -# _build_prg() and before any VICE work. Timeouts nest correctly -- 180 s on -# the outer call here, 120 s on the inner one there. +# It terminates, and it is cheap, for the SAME reason -- and that reason is a +# cross-dependency worth naming, because it is not local to either file. +# +# The KAT is not a leaf in general: _build_prg() runs `make clean` and two +# `make BACKEND=uci` invocations (test_ecdsa_p384_kat.py:646-665). What makes +# it a leaf HERE is that its --u64-with-no-U64_HOST gate returns BEFORE +# _build_prg(), so nothing further is spawned and the whole nested run +# measures ~0.07 s. Timeouts nest correctly -- 180 s on the outer call here, +# 120 s on the inner one there. +# +# So THE COST BOUND OF THIS TEST RESTS ON WHERE THAT GATE SITS. Moving it +# below _build_prg() -- the alternative fix considered and rejected when the +# gate was made non-opt-out-able -- would start a full `make clean && make` +# inside that 120 s inner timeout. Anyone reconsidering the gate's placement +# is also deciding this, and there is nothing at the gate that would say so; +# hence the note here and the pointer at the spawn site itself. # # An earlier version of this comment claimed the target "has no spawner of its # own". That was true when it was written and false in the same commit, which