fix(tests): pin the pytest collection boundary so a root pytest cannot imply coverage it does not have - #111
Merged
Merged
Conversation
…onger implies coverage it does not have Closes#109. tests/ held five live-rig scripts named test_*.py with no `def test_` in them, so pytest walked the directory, collected zero, and reported nothing about it. The pass count a bare `pytest` printed came entirely from tools/ and read like whole-project coverage. Measured on master (pytest 9.0.3, run from the repo root): $ pytest -> 25 passed, 75 errors exit 1 $ pytest tests/ -> no tests ran exit 5 The 75 errors are `fixture 'transport' not found`: tools/test_*.py is not a pytest suite either. Its `test_*` functions take positional (transport, labels) supplied by tools/run_all_tests.py, and the repo has no conftest.py and declares no pytest dependency. So the true defect is wider than reported — nothing here is meaningfully pytest-shaped, and what a bare `pytest` prints depends on which interpreter and plugins happen to be around. Fixed by making the boundary explicit rather than accidental: - tests/test_*.py renamed to tests/rig_*.py. A rename holds from any working directory; testpaths only applies at the rootdir, so config alone could not do this. - pytest.ini pins testpaths to the three modules pytest can genuinely run, and keeps collection out of libs/, ip65/, tests/ and tools/uci/ (all test_*.py-named, all collecting zero). - conftest.py states the scope of the run in the header and again above the summary line. No skips: a vague skip reads like coverage, which is the thing being removed. - tools/test_package_verify.py::test_parse_build_info_records took a `tmp_lines` argument its own main() supplied, so it was the one erroring test in an otherwise pure-logic module. Defaulted. - tools/test_pytest_boundary.py guards both drift directions: a pure-logic module missing from testpaths, or a test_*.py file reappearing in tests/. Verified by probe in both directions. After: $ pytest -> 30 passed exit 0 $ pytest tests/ -> no tests ran exit 5, now explained Every reference to the renamed scripts updated (README, CLAUDE.md, docs/, tools/https_e2e/, and their own docstrings); `grep -rn 'tests/test_'` is clean. No .s, .cfg or Makefile change, so no build evidence applies. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JC-000 added a commit
that referenced
this pull request
Aug 15, 2026
…st measurement Two deferred follow-ups, both left with reasons recorded by earlier lanes. PR #111 renamed tests/test_*.py -> tests/rig_*.py because those files wear pytest's discovery convention while collecting zero tests, so a bare `pytest` overstates coverage. It explicitly deferred the identical problem in tools/uci/, whose blast radius runs through CLAUDE.md. This is that change: six scripts renamed, following #111's precedent exactly. `norecursedirs` already closed the default-invocation path, but the rename is what holds from an arbitrary working directory, since `testpaths` only applies at the rootdir. Both halves are now pinned by the guard rather than one. tools/test_pytest_boundary.py grows a RIG_DIRS tuple covering both directories and a new test_norecursedirs_covers_every_rig_dir. Probed in both directions: a stray tools/uci/test_*.py fails test_rig_dirs_hold_no_pytest_named_files, and dropping tools/uci from norecursedirs fails the new check. Bare `pytest` is now 31 passed / exit 0; `pytest tests/` and `pytest tools/uci/` both exit 5 with an explanation naming the right README. New tools/uci/README.md mirrors tests/README.md. Reference sweep covers CLAUDE.md, README.md, conftest.py, pytest.ini, tests/README.md, both phase_f docs, tools/https_e2e/, tools/package/listener/, and the cross-invocations (rig_https_print_body and rig_https_local_p384 both delegate to rig_https_local; import-checked after the rename). Zero references exist in c64-test-harness or any other sibling c64-* repo, and no open issue or PR in the org names any of the six. README was audited claim-by-claim in #104; CLAUDE.md never was. Every item below was confirmed by running a command, not by reading: - The ip65 blob section contradicted itself and the Makefile. It claimed `ip65-blob` is phony with "no rule connecting the two" and that a fresh clone dies at the `.incbin`. Measured: deleting the blob and running plain `make` rebuilds it byte-identically (6,951 B, cf1a5ff7...) and links the usual 47,105 B PRG. The real fresh-clone blocker is the missing ip65 .lib archives, which fail at the blob's ld65 step. `make ip65-blob` is not a required step. Fixed here and in README. - "the committed blob" / `touch ip65-build/ip65-c64.bin` advice: the blob is gitignored, so on a fresh clone there is nothing to touch. - Fence macro: 17 bytes per site, not 14 (counted from the macro body). 26 sites, not 24 — 11 write + 14 read + 1 pre-loop settle at net.s:199. - uci_drain_resp/uci_drain_status: 22 call sites in net.s, not 13. All 22 still `bcs` out, so that half was right. - CIA1 TOD read order is HOUR -> TENTHS; MIN and SEC are never read and have no equate in uci_cmd.s. - UCI memory table: NET_CODE is $2000-$3B65 and NET_BSS_TAIL $3B66-$41FF (cfg grew NET_CODE by $40 for the C64U WiFi iface-fallback loop). UCI_BSS_REGION is size 0, so quoting a 512 B span contradicted its own label and overlapped NET_BSS_TAIL. - src/exports.s no longer exports ip65_init/ip65_process; those moved to src/net/ip65/exports.s. Named the real backend-agnostic set instead. - KEEP_DEBUG_ON_PASS is the env var; UCI_DEBUG_KEEP_ON_PASS is only the Python variable name, so the documented spelling did nothing. - rig_https_local_p384.py defaults to 90 minutes (5400 s), not 30. - C64_SKIP_BUILD: 15 scripts, not 14 (ran the recipe the file itself quotes). - EMBED_P256_OVERLAY's stated mutual exclusions have no $(error) guard: one is a silent auto-disable, the other is unguarded. Only the USE_NISTCURVES_ONCHIP exclusions are hard errors. - "All in-tree VICE-driven tests go through default_vice_config()" is false: 8 suites do, 15 other files still build ViceConfig directly — including run_all_tests.py, which hand-spells the REU flags. - Smoke-test list is 8 entries, not 7, and the 97/97 total predates test_finished_verify.py joining it. - pytest counts 30 -> 31; src/boot.s:107-114 -> 110-117. Wall-clock figures are labelled rather than re-measured, per instruction: a pin-vs-commit table resolves 2ceb5b1 / f0127a0 / cb6eab4 to libs/nistcurves v0.6.0 (verified with git ls-tree), every table states the pin it was taken at, and "at HEAD" captions that no longer refer to HEAD are gone. No hardware was used and no benchmark was re-run. Deliberately NOT included: the x25519 sibling failure. It is measured and understood (both backends exit 2 on a duplicate reu_mul_tables_init export, not the overflow CLAUDE.md records), but PR #113 rewrites the same spans and fixes the underlying collision, so the prose belongs there. Details handed to that lane. docs/library-ingestion-architecture.md's two stale references are owned and taken by the contract lane. Python and docs only — no .s, no .cfg, no Makefile, so no PRG changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes#109.
What was wrong — and it is worse than the report
The report is right that
tests/holds fivetest_*.pyfiles with nodef test_in them, so pytest walks a directory named exactly the way its discovery convention expects, collects zero, and says nothing about it.But the reported "green 154 passed" does not reproduce. pytest is available on this bench (
/opt/homebrew/bin/pytest9.0.3, on an interpreter that can importc64_test_harnessandcryptography), so this was measured rather than inferred. On a cleanorigin/mastersnapshot, run from the repo root:The 75 errors are all
fixture 'transport' not found.tools/is not a pytest suite either. Itstest_*functions take positional(transport, labels)supplied bytools/run_all_tests.py, which allocates a VICE instance per suite; there is noconftest.pyanywhere in the repo, no plugin provides those fixtures, and the repo declares no pytest dependency at all.So the real defect is broader than "tests/ contributes zero": nothing in this repo is meaningfully pytest-shaped, and what a bare
pytestprints depends entirely on which interpreter and plugins happen to be around. The reporter got a green; this bench gets a red. Both are noise.Collection breakdown on master (100, not 154 — the
libs/submodules are unpopulated in this worktree, which accounts for the other 54):tools/test_net_test_env.pyunittest.TestCasesuite)tools/test_package_verify.pytools/test_*.pytests/tools/uci/test_*.pytests/)What shipped
Option 1 + 3 from the issue. Both were needed, and neither is sufficient alone:
tests/test_*.py->tests/rig_*.py. This part had to be a rename.testpathsonly takes effect when pytest is invoked from the rootdir, so config alone leaves the directory collectable from anywhere else. A rename holds unconditionally.pytest.inipinstestpathsto the three modules pytest can genuinely run, andnorecursedirskeeps collection out oflibs/,ip65/,tests/andtools/uci/— every one of which istest_*.py-named and contributes zero.conftest.pyprints the scope of the run in the header and again immediately above the summary line (the header scrolls away on a long run; the summary line is what people read). It imports nothing and defines no fixtures, so it stays inert without pytest — the repo's zero-dependency posture is unchanged.tools/test_package_verify.py::test_parse_build_info_recordstook atmp_linesargument its ownmain()passed in, which is why an otherwise pure-logic module had one erroring test. Defaulted to a module constant; both runners still work (python3 tools/test_package_verify.py-> 31 passed, unchanged, andmake package-verifyinvokes exactly that).tools/test_pytest_boundary.py— a pure-AST guard (no VICE, no build, milliseconds) so this cannot silently regress. It assertstests/holds notest_*.py, everytestpathsentry exists, andtestpathsis exactly the set oftools/test_*.pymodules pytest can run — both directions, so a new pure-logic suite cannot become invisible to a barepytesteither. It runs under pytest and standalone.tests/README.md— what each rig script needs, how to run it, and why it is not collectable.Before / after, measured
Run via
subprocesswith an explicitcwd(the "before" column is agit archiveoforigin/masterextracted to a scratch dir, so it is a genuinely pristine tree):pytest(repo root)pytest tests/After, at the repo root:
pytest tests/now ends with:One honest limitation, measured rather than glossed:
testpathsapplies only at the rootdir, sopytestfrom a subdirectory collects that subdirectory instead — fromtools/it is 30 passed + 74 fixture errors, exit 1. That is left as-is deliberately. It is loud, and it is correct: those modules really cannot run under pytest. Suppressing it withcollect_ignore_globwould trade a true red for a manufactured green. README and CLAUDE.md both state this rather than claiming determinism the config does not provide.Guard verified by probe, not by assertion
Both probes removed afterwards. The guard also correctly classifies
tools/test_net_test_env.pyas runnable (it is aunittest.TestCasemodule whose extra method arguments come from@mock.patch, not from fixtures) — an earlier, cruder version of the rule got that wrong and the probe caught it.Why not option 2 (pytest wrappers +
pytest.skip())Two reasons, and the second is the load-bearing one:
requirements.txtiscryptographyplus a comment; the documented runner ispython3 tools/run_all_tests.py. Wrapper modules thatimport pytestwould make a currently-optional tool mandatory for a test tree that pytest cannot run anyway.RELEASE ARTIFACTS VERIFIEDhaving checked nothing. "0 network tests ran" and "5 network tests skipped for a reason nobody read" are the same fact with different amounts of reassurance attached. The rename plus an unmissable scope banner says it once, in plain language, on every single run.Reference sweep
grep -rn 'tests/test_'is clean. Updated:README.md(x5 plus a new "pytestis not the runner here" subsection),CLAUDE.md(x4 plus a boundary subsection under Smoke tests, and theC64_SKIP_BUILDgrep recipe now globstests/rig_*.py),docs/library-ingestion-architecture.md,tools/https_e2e/__init__.py,tools/https_e2e/certs/README, and 6 docstring/comment references inside the rig scripts themselves.No
Makefiletarget, no CI (the repo has no.github/), and no reference anywhere in the siblingc64-test-harnessrepo (grepped the whole tree; zero hits).tests/rig_phase1_dhcp.pywas executed post-rename and still resolves its own paths correctly, skipping loudly:SKIP: missing prerequisites: ip not on PATH; iptables not on PATH; sudo requires a password.Scope
Python and docs only — no
.s, no.cfg, noMakefile, so the build-evidence bar does not apply and no PRG changes..gitignoregains.pytest_cache/.Adjacent, deliberately not fixed here:
tools/uci/test_*.pyhave the identical shape (rig scripts namedtest_*.pycollecting zero).norecursedirscloses the misleading path for the default invocation, but the honest fix is the samerig_rename, and that blast radius (CLAUDE.md documents those scripts extensively) belongs in its own PR.🤖 Generated with Claude Code