From 0287566552594714cf1faf8efccd802eb74088d1 Mon Sep 17 00:00:00 2001 From: JC-000 <3798556+JC-000@users.noreply.github.com> Date: Sat, 5 Sep 2026 17:54:15 -0500 Subject: [PATCH 1/2] flags-stamp suite: an absent cc65 must be loud, not green (#177) tools/test_build_flags_stamp.py has eight tests, every one of which shells out to `make`. Each opened with missing = _toolchain_missing() if missing: print(f"SKIP: {missing} not on PATH") return and a pytest test function that returns None without asserting is a PASS. The module is pinned in pytest.ini `testpaths`, so on a machine without cc65 -- a CI container, most likely -- bare `pytest` at the repo root reported `7 passed in 0.01s` having verified nothing. The SKIP lines were not even visible: pytest captures stdout on a passing test and discards it. The standalone runner had the same hole from the other side, printing `PASSED: 0 failure(s)` for a run in which zero assertions executed. The invariant being laundered is build/flags.stamp itself -- what closes CLAUDE.md's two documented silent-failure modes. #158 adopted the rule (an involuntary skip is a failure; a voluntary skip is allowed but must never be silent), #157 reintroduced it, PR #172 closed it again. Both channels now: pytest lane _require_toolchain() calls _skip_policy.require(), which raises SkipPolicyError carrying the vacuity warning INSIDE the reason string -- under `-ra` the reason is the only channel that survives. standalone lane main() hoists the check and returns cannot_run() = exit 2, distinct from 1 ("a check ran and failed"), plus a ran+failed==0 guard so PASSED can never be printed by a run that executed nothing. opt-out C64_ALLOW_SKIP=1 buys exit 0 / a real pytest skip and still prints the warning; exactly "1", so C64_ALLOW_SKIP=0 does NOT open the hatch. tools/_skip_policy.py is vendored from the #178 Part A work, byte-identical to the copy on branch test/178a-skip-policy-rig-half (c3cf8a4), so this PR is green standing alone rather than importing a module that is not on master yet. An identical both-added file merges without conflict in either order. That branch also adds tools/test_skip_policy.py (unit tests for the module) and wires the rig lane; none of that is duplicated here. New tools/test_flags_stamp_skip_is_loud.py is the red-green. It re-runs the subject module in a subprocess whose PATH has had every directory containing ca65/ld65 removed -- computed from the real PATH, not a hardcoded /usr/bin:/bin, so it strips the toolchain wherever it is installed. test_the_strip_actually_strips guards the guard: a PATH-stripping bug would otherwise make every assertion vacuous, one level up from the defect being fixed. Measured, with the subject module at the pre-fix revision: FAIL test_opt_out_is_explicit_and_still_warns FAIL test_pytest_run_without_toolchain_is_not_green FAIL test_standalone_run_without_toolchain_exits_cannot_run ok test_the_strip_actually_strips FAILED: 3 failure(s) (4 executed) and after: PASSED: 0 failure(s) (4 executed) Bare `pytest` at the repo root: 60 passed (was 56; +4 from the new module). Co-Authored-By: Claude Opus 5 (1M context) --- pytest.ini | 1 + tools/_skip_policy.py | 280 +++++++++++++++++++++++++ tools/test_build_flags_stamp.py | 129 ++++++++---- tools/test_flags_stamp_skip_is_loud.py | 213 +++++++++++++++++++ 4 files changed, 586 insertions(+), 37 deletions(-) create mode 100644 tools/_skip_policy.py create mode 100644 tools/test_flags_stamp_skip_is_loud.py diff --git a/pytest.ini b/pytest.ini index 29ff73b..5e78533 100644 --- a/pytest.ini +++ b/pytest.ini @@ -46,6 +46,7 @@ [pytest] testpaths = tools/test_build_flags_stamp.py + tools/test_flags_stamp_skip_is_loud.py tools/test_net_test_env.py tools/test_package_verify.py tools/test_pytest_boundary.py 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/test_build_flags_stamp.py b/tools/test_build_flags_stamp.py index 282140e..c7a644c 100644 --- a/tools/test_build_flags_stamp.py +++ b/tools/test_build_flags_stamp.py @@ -47,6 +47,18 @@ Runs under pytest, and standalone:: python3 tools/test_build_flags_stamp.py + +Exit codes (standalone), per tools/_skip_policy.py:: + + 0 every case ran and passed + 1 a case ran and FAILED + 2 COULD NOT RUN -- ca65/ld65 absent, so nothing was verified + +An absent toolchain is an INVOLUNTARY skip and must never read as a pass +(#177): this module is pinned in pytest.ini `testpaths`, so a silent skip +here is bare `pytest` at the repo root going green while asserting an +invariant it never checked. `C64_ALLOW_SKIP=1` is the deliberate opt-out +for a CI lane with no cc65; it still prints the warning. """ import hashlib @@ -57,8 +69,17 @@ import tempfile from pathlib import Path +sys.path.insert(0, str(Path(__file__).resolve().parent)) +from _skip_policy import EXIT_CANNOT_RUN, cannot_run, require # noqa: E402 + REPO = Path(__file__).resolve().parent.parent +# What every test here certifies, and therefore what a run that could not +# execute them certifies NOTHING about. Named once so the involuntary-skip +# block says the same thing on both channels. +CERTIFIES = ("build/flags.stamp — that a build-flag change invalidates the " + "tree, and that an unchanged flag set does not") + # What a build reads. `libs` carries the sibling submodules the archive # wrappers assemble from; `ip65`/`ip65-build` are symlinked for # completeness but never assembled, because every case here is @@ -127,6 +148,35 @@ def _toolchain_missing(): return None +def _require_toolchain(): + """Fail loudly, on both channels, when cc65 is not installed (#177). + + Every test here shells out to `make`, so an absent ca65/ld65 means the + body cannot run. This used to be four lines of `print("SKIP: ...")` and a + bare `return` -- and a pytest test that returns None without asserting is + a PASS, so `pytest tools/test_build_flags_stamp.py` on a machine without + cc65 reported `7 passed in 0.01s`, the SKIP lines swallowed by pytest's + stdout capture. This module is pinned in pytest.ini `testpaths`, so that + is bare `pytest` at the repo root going green while asserting an + invariant it never checked. + + `require()` raises instead, carrying the vacuity warning INSIDE the + reason string, because under pytest `-ra` the reason is the only channel + that survives. `C64_ALLOW_SKIP=1` is the deliberate opt-out for a CI lane + with no toolchain: it downgrades to a real pytest skip and still prints + the warning. + """ + missing = _toolchain_missing() + require( + missing is None, + f"{missing} not on PATH -- every case here shells out to `make`", + executed=0, + total=1, + certifies=CERTIFIES, + opt_out_env="C64_ALLOW_SKIP", + ) + + class Farm: """A disposable tree that builds the repo without writing to it.""" @@ -182,10 +232,7 @@ def test_flag_change_without_clean_matches_a_clean_build(): `make clean` is the documented remedy and it is absent here on purpose — the whole point is what happens when someone forgets it. """ - missing = _toolchain_missing() - if missing: - print(f"SKIP: {missing} not on PATH") - return + _require_toolchain() oracle = _clean_build_sha(*UCI, "HTTPS_SNI=www.foo.invalid") with Farm() as farm: farm.make(*UCI) @@ -215,10 +262,7 @@ def test_profile_flip_without_clean_matches_a_clean_build(): the sibling archive, AND retargets $(CFG) to the -onchip cfg variant. Nothing in the old Makefile noticed any of the three. """ - missing = _toolchain_missing() - if missing: - print(f"SKIP: {missing} not on PATH") - return + _require_toolchain() comb = ("BACKEND=uci", "USE_NISTCURVES_ONCHIP_COMB=1") oracle = _clean_build_sha(*comb) with Farm() as farm: @@ -236,10 +280,7 @@ def test_profile_flip_without_clean_matches_a_clean_build(): def test_vic_blank_flip_without_clean_matches_a_clean_build(): """A pure `-D` knob with no generated include anywhere near it.""" - missing = _toolchain_missing() - if missing: - print(f"SKIP: {missing} not on PATH") - return + _require_toolchain() noblank = UCI + ("VIC_BLANK=0",) oracle = _clean_build_sha(*noblank) with Farm() as farm: @@ -272,10 +313,7 @@ def test_backend_flip_removes_the_other_backends_prg(): `make -n BACKEND=ip65`, which relied on a dry run mutating the tree — the defect fixed in #174. """ - missing = _toolchain_missing() - if missing: - print(f"SKIP: {missing} not on PATH") - return + _require_toolchain() with Farm() as farm: farm.make(*UCI) assert farm.exists(PRG) @@ -319,10 +357,7 @@ def test_dry_run_options_do_not_touch_the_tree(): "simplification" of the Makefile restores #174 with this file fully green. See the comment on that table. """ - missing = _toolchain_missing() - if missing: - print(f"SKIP: {missing} not on PATH") - return + _require_toolchain() comb = ("BACKEND=uci", "USE_NISTCURVES_ONCHIP_COMB=1") with Farm() as farm: farm.make(*UCI) @@ -443,10 +478,7 @@ def test_an_unrelated_option_does_not_suppress_invalidation(): def test_unchanged_flags_rebuild_nothing(): """The inverse property: the stamp must not make every build a rebuild.""" - missing = _toolchain_missing() - if missing: - print(f"SKIP: {missing} not on PATH") - return + _require_toolchain() with Farm() as farm: farm.make(*UCI) before_objs = farm.mtimes() @@ -471,10 +503,7 @@ def test_https_host_change_is_still_incremental(): finer grain (boot.o + http.o, not the whole tree). Certificate pinning (#155) is about to depend on that staying cheap. """ - missing = _toolchain_missing() - if missing: - print(f"SKIP: {missing} not on PATH") - return + _require_toolchain() retarget = UCI + ("HTTPS_HOST=en.wikipedia.org",) oracle = _clean_build_sha(*retarget) with Farm() as farm: @@ -504,10 +533,7 @@ def test_stamp_records_the_whole_command_line(): answering "what was this PRG built with?" at a glance, which is the question the incident started from. """ - missing = _toolchain_missing() - if missing: - print(f"SKIP: {missing} not on PATH") - return + _require_toolchain() with Farm() as farm: farm.make(*UCI) text = farm.path(STAMP).read_text() @@ -531,18 +557,47 @@ def test_stamp_records_the_whole_command_line(): def main() -> int: print("=== build/flags.stamp ===") + tests = [(name, fn) for name, fn in sorted(globals().items()) + if name.startswith("test_") and callable(fn)] + + # The toolchain check is hoisted out of the loop on purpose. Per-test it + # would report eight identical failures for one missing binary, and the + # standalone lane wants the "could not run" EXIT CODE (2), which is a + # thing only a process has -- `require()`'s AssertionError is the right + # answer for pytest, where the unit of reporting is the test. + missing = _toolchain_missing() + if missing: + return cannot_run( + f"{missing} not on PATH -- every case here shells out to `make`", + executed=0, + total=len(tests), + certifies=CERTIFIES, + opt_out_env="C64_ALLOW_SKIP", + ) + failed = 0 - for name, fn in sorted(globals().items()): - if not name.startswith("test_") or not callable(fn): - continue + ran = 0 + for name, fn in tests: try: fn() - except Exception as exc: # noqa: BLE001 — a crash is a fail + except Exception as exc: # noqa: BLE001 - a crash is a fail failed += 1 print(f" FAIL {name}\n {type(exc).__name__}: {exc}") else: + ran += 1 print(f" ok {name}") - print(f"\n{'FAILED' if failed else 'PASSED'}: {failed} failure(s)") + + # A run that executed nothing must never print PASSED. Asserted rather + # than assumed, because the version of this file that #177 reports did + # print "PASSED: 0 failure(s)" for a run in which zero assertions ran. + if ran + failed == 0: + return cannot_run( + "no test functions were collected from this module", + executed=0, total=0, certifies=CERTIFIES, + ) + + print(f"\n{'FAILED' if failed else 'PASSED'}: {failed} failure(s) " + f"({ran}/{len(tests)} executed)") return 1 if failed else 0 diff --git a/tools/test_flags_stamp_skip_is_loud.py b/tools/test_flags_stamp_skip_is_loud.py new file mode 100644 index 0000000..82fbbdb --- /dev/null +++ b/tools/test_flags_stamp_skip_is_loud.py @@ -0,0 +1,213 @@ +#!/usr/bin/env python3 +"""A missing cc65 must not turn tools/test_build_flags_stamp.py green (#177). + +That module has eight tests, every one of which shells out to `make`. Each +used to open with + + missing = _toolchain_missing() + if missing: + print(f"SKIP: {missing} not on PATH") + return + +and a pytest test function that returns None without asserting is a **pass**. +The module is pinned in ``pytest.ini`` ``testpaths``, so on a machine without +cc65 — a CI container, most likely — bare ``pytest`` at the repo root reported + + ....... [100%] + 7 passed in 0.01s + +The ``SKIP:`` lines were not even visible: pytest captures stdout on a passing +test and discards it. ``0.01 s`` was the only tell. The standalone runner had +the same hole from the other side, printing ``PASSED: 0 failure(s)`` for a run +in which zero assertions executed. + +This matters more than an ordinary silent skip because the invariant being +laundered is ``build/flags.stamp`` itself — what closes CLAUDE.md's two +documented silent-failure modes (a mixed link, and no link at all). If the +enforcement can evaporate on a ``PATH`` accident, the enforcement is a +convention again. + +The rule, adopted in #158 and re-closed in PR #172: *an involuntary skip is a +failure; a voluntary skip is allowed but must never be silent.* A missing +toolchain is involuntary. + +Method: re-run the suite in a subprocess whose ``PATH`` has had every directory +containing ca65 or ld65 removed — computed from the real ``PATH``, not a +hardcoded ``/usr/bin:/bin``, so it strips the toolchain wherever it is +installed and leaves ``make``/``sh``/``cmp`` alone. Both channels are checked, +because #177 documents the hole on both: + + * standalone — must exit 2 (COULD NOT RUN), not 0 + * pytest — must not report the cases as passes + +Nothing here builds anything: the subject process cannot assemble, which is +the entire point, so this file costs a few hundred milliseconds. + +Runs under pytest, and standalone:: + + python3 tools/test_flags_stamp_skip_is_loud.py + +Exit codes: 0 pass, 1 fail. There is no skip path — this test needs no +toolchain, by construction. +""" + +import os +import shutil +import subprocess +import sys +from pathlib import Path + +REPO = Path(__file__).resolve().parent.parent +SUBJECT = REPO / "tools" / "test_build_flags_stamp.py" + +# The tools whose absence must be loud. Both are needed by every case in the +# subject module; either one missing means nothing can be verified. +TOOLCHAIN = ("ca65", "ld65") + +EXIT_CANNOT_RUN = 2 + + +def _path_without_toolchain(): + """A PATH with every directory that provides ca65 or ld65 removed. + + Computed rather than hardcoded: cc65 is under /opt/homebrew/bin here and + /usr/local/bin or /usr/bin elsewhere, and a hardcoded "/usr/bin:/bin" + would silently stop stripping anything on a machine that installs it + there — the test would then pass for the wrong reason, which is the + class of defect this file exists to catch. + """ + entries = [e for e in os.environ.get("PATH", "").split(os.pathsep) if e] + keep = [] + for entry in entries: + if any((Path(entry) / tool).exists() for tool in TOOLCHAIN): + continue + keep.append(entry) + return os.pathsep.join(keep) + + +def _env_without_toolchain(): + env = dict(os.environ) + env["PATH"] = _path_without_toolchain() + # The opt-out must not be inherited from the developer's shell, or this + # test would measure the opt-out instead of the default. + env.pop("C64_ALLOW_SKIP", None) + return env + + +def _run(cmd, env): + return subprocess.run(cmd, cwd=REPO, env=env, text=True, + stdout=subprocess.PIPE, stderr=subprocess.STDOUT) + + +def test_the_strip_actually_strips(): + """Guard the guard: prove the stripped PATH really has no toolchain. + + Without this, a PATH-stripping bug makes every assertion below run + against a machine that CAN build, and they would all pass having tested + nothing — the same vacuity, one level up. + """ + stripped = _path_without_toolchain() + for tool in TOOLCHAIN: + assert shutil.which(tool, path=stripped) is None, ( + f"{tool} is still reachable on the stripped PATH ({stripped!r}); " + "every assertion in this module would be vacuous" + ) + + +def test_standalone_run_without_toolchain_exits_cannot_run(): + """`python3 tools/test_build_flags_stamp.py` with no cc65: exit 2, not 0.""" + proc = _run([sys.executable, str(SUBJECT)], _env_without_toolchain()) + assert proc.returncode == EXIT_CANNOT_RUN, ( + "tools/test_build_flags_stamp.py with no ca65/ld65 on PATH exited " + f"{proc.returncode}, expected {EXIT_CANNOT_RUN} (COULD NOT RUN). " + "Exit 0 there is a run that executed zero assertions reporting " + "itself as a pass; exit 1 would claim a check ran and failed.\n" + f"--- output ---\n{proc.stdout}" + ) + assert "COULD NOT RUN" in proc.stdout, ( + "the standalone run did not name its verdict:\n" + proc.stdout + ) + assert "PASSED" not in proc.stdout, ( + "the standalone run printed PASSED having verified nothing:\n" + + proc.stdout + ) + + +def test_pytest_run_without_toolchain_is_not_green(): + """`pytest tools/test_build_flags_stamp.py` with no cc65 must be red. + + A bare `return` from a pytest test is a pass, so this reported + `7 passed in 0.01s` — and because the module is in pytest.ini + `testpaths`, that green is what bare `pytest` at the repo root shows. + """ + env = _env_without_toolchain() + proc = _run([sys.executable, "-m", "pytest", "-q", "-p", "no:cacheprovider", + str(SUBJECT)], env) + if "No module named pytest" in proc.stdout: + # Not a skip: the standalone channel above already fails this + # module's defect, and this repo declares no pytest dependency. + # Assert the fallback rather than returning quietly. + assert proc.returncode != 0 + return + assert proc.returncode != 0, ( + "pytest on tools/test_build_flags_stamp.py with no ca65/ld65 exited " + "0. Every case returned early without asserting, which pytest counts " + "as a pass, and this module is pinned in pytest.ini testpaths — so " + "bare `pytest` at the repo root goes green having verified nothing " + "about the build-flag invariant.\n" + f"--- output ---\n{proc.stdout}" + ) + assert " passed" not in proc.stdout or "failed" in proc.stdout, ( + "pytest reported passes for cases that could not run:\n" + proc.stdout + ) + + +def test_opt_out_is_explicit_and_still_warns(): + """C64_ALLOW_SKIP=1 may buy exit 0 — never silence. + + The escape hatch exists so a CI lane with no cc65 can stay green *by + saying so in its own configuration*, which is greppable. It suppresses + the exit code, never the warning, and it must require exactly "1" — a + bare-truthiness check would make C64_ALLOW_SKIP=0 turn the policy off. + """ + env = _env_without_toolchain() + env["C64_ALLOW_SKIP"] = "1" + proc = _run([sys.executable, str(SUBJECT)], env) + assert proc.returncode == 0, ( + "C64_ALLOW_SKIP=1 did not honour the opt-out:\n" + proc.stdout + ) + assert "certifies NOTHING" in proc.stdout, ( + "the opt-out silenced the vacuity warning; it may only suppress the " + "exit code:\n" + proc.stdout + ) + + env["C64_ALLOW_SKIP"] = "0" + proc = _run([sys.executable, str(SUBJECT)], env) + assert proc.returncode == EXIT_CANNOT_RUN, ( + "C64_ALLOW_SKIP=0 opened the escape hatch. Setting it to 0 is what " + "someone does to CLOSE it; only the literal \"1\" may open it.\n" + + proc.stdout + ) + + +def main() -> int: + print("=== involuntary skip must be loud (#177) ===") + tests = [(n, f) for n, f in sorted(globals().items()) + if n.startswith("test_") and callable(f)] + failed = 0 + for name, fn in tests: + try: + fn() + except Exception as exc: # noqa: BLE001 — a crash is a fail + failed += 1 + print(f" FAIL {name}\n {type(exc).__name__}: {exc}") + else: + print(f" ok {name}") + assert tests, "no tests collected" + print(f"\n{'FAILED' if failed else 'PASSED'}: {failed} failure(s) " + f"({len(tests)} executed)") + return 1 if failed else 0 + + +if __name__ == "__main__": + sys.exit(main()) From 3619583a7541f0e6e85ce1ba87f4802028586ccd Mon Sep 17 00:00:00 2001 From: JC-000 <3798556+JC-000@users.noreply.github.com> Date: Sat, 5 Sep 2026 18:27:42 -0500 Subject: [PATCH 2/2] Take tools/_skip_policy.py from #178a instead of vendoring it #180 (#178a) amended the module after this branch copied it, so the two copies are no longer identical (theirs c359a9b1..., the vendored one befcf309...) and `git merge-tree` reports an add/add conflict with five hunks in both directions. The "merges cleanly in either order" claim this branch shipped with is now false, and the PR body is corrected to match. Dropping the copy is free: this branch uses only require(), cannot_run() and EXIT_CANNOT_RUN, whose contracts did not change, and all five probes in tools/test_flags_stamp_skip_is_loud.py behave identically against either version of the module. Merge order is therefore load-bearing and stated in the PR body: #183 (this branch's base) -> #180 (brings _skip_policy.py) -> #186. Also migrates the guard in test_an_unrelated_option_does_not_suppress_invalidation, added to the base branch after this one was written, to _require_toolchain(). It was the one remaining site still spelling the silent-skip prologue by hand, which is the whole point of #177. Co-Authored-By: Claude Opus 5 (1M context) --- tools/_skip_policy.py | 280 -------------------------------- tools/test_build_flags_stamp.py | 5 +- 2 files changed, 1 insertion(+), 284 deletions(-) delete mode 100644 tools/_skip_policy.py diff --git a/tools/_skip_policy.py b/tools/_skip_policy.py deleted file mode 100644 index e1429c3..0000000 --- a/tools/_skip_policy.py +++ /dev/null @@ -1,280 +0,0 @@ -#!/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/test_build_flags_stamp.py b/tools/test_build_flags_stamp.py index c7a644c..7df336b 100644 --- a/tools/test_build_flags_stamp.py +++ b/tools/test_build_flags_stamp.py @@ -432,10 +432,7 @@ def test_an_unrelated_option_does_not_suppress_invalidation(): exactly the `make clean` image — the end-to-end statement, and the one that catches a guard that merely warns without acting. """ - missing = _toolchain_missing() - if missing: - print(f"SKIP: {missing} not on PATH") - return + _require_toolchain() # Cheap probe: real build, links nothing (the goal is already up to # date once the parse-time block has written it), two common long # options plus one that shares no letters with n/q/t.