Skip to content

tests: register the eight tests/*_test.py nothing ran, and audit the third file kind (engine#2018) - #2022

Closed
bong-water-water-bong wants to merge 1 commit into
mainfrom
tests/register-python-tests
Closed

bong-water-water-bong wants to merge 1 commit into
mainfrom
tests/register-python-tests

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

What was measured

$ # every tests/*_test.py on main, against every CMake file (root, cmake/*.cmake, */CMakeLists.txt)
41 tests/*_test.py on disk; 8 named by no CMake file at all

check_pins_test · gguf_to_q4nx_test · hrx_long_prompt_test · hyperloom_benchmark_test · moe_expert_chunks_accum_test · mtp_draft_vocab_test · repack_verify_source_test · ternary_to_q4_0_test

All eight pass when run by hand (rc=0, 2026-11-10), so they are registered rather than excused — eight rows of the form

add_test(NAME gguf_to_q4nx COMMAND ${Python3_EXECUTABLE} ${CMAKE_SOURCE_DIR}/tests/gguf_to_q4nx_test.py)

placed after find_package(Python3) (:761) as tests/python_test_order_check.py requires. 41 of 41 are now run by a row.

The audit, which is the part that lasts

tools/ci/python_test_referenced_check.py is the third file kind after #1824's tests/*.sh and #1911's tools/*.py, and it checks both directions:

  • UNRUN — a tests/*_test.py that no row names and tools/ci/unreferenced_python_tests.txt does not excuse;
  • STALE — an excuse for a file that a row does name. An excuse that outlives its reason passes as "deliberately unrun" and the next reader believes it, which is the same defect one step later;
  • a line in the allow file with no reason after the colon is refused outright rather than silently silencing a test.
$ python3 tools/ci/python_test_referenced_check.py --check
python_test_referenced_check: 41 test(s), 41 run by a row, 0 excused, 0 unrun, 0 stale

$ python3 tools/ci/python_test_referenced_check.py --self-test
  ok   an unrun test with no excuse fails the check
  ok   an excused test clears the check
  ok   an excuse that outlived its reason fails as stale
  ok   a reasonless excuse is refused

(The self-test's first draft had an unexcused fixture file, so its own second arm failed — recorded because it is the argument for writing the arms first.)

A documentation bug found on the way

CONTRIBUTING.md:81 said "a row that runs Python goes through add_python_test()". No such macro exists anywhere in the tree — all 41 rows name ${Python3_EXECUTABLE} by hand. The rule now says what the rows do and names the new check; a lane following the old text would have written a CMake command that fails to configure.

Checks

python_test_order_check.py --check → ok · python_test_referenced_check.py --check → clean · --self-test 4/4 · copyright.py --check rc=0.

…third file kind (engine#2018)

Measured on main 2026-11-10: of 41 tests/*_test.py on disk, 8 were named by no CMake file at all -- so nothing
ran them and nothing noticed:

  check_pins_test  gguf_to_q4nx_test  hrx_long_prompt_test  hyperloom_benchmark_test
  moe_expert_chunks_accum_test  mtp_draft_vocab_test  repack_verify_source_test  ternary_to_q4_0_test

All eight pass when run by hand (rc=0, 2026-11-10), so they are registered as rows rather than excused:
add_test(NAME x COMMAND ${Python3_EXECUTABLE} ${CMAKE_SOURCE_DIR}/tests/x_test.py), after find_package(Python3)
as tests/python_test_order_check.py requires. 41 of 41 are now run by a row.

tools/ci/python_test_referenced_check.py is the audit that keeps the set from growing, and it checks BOTH
directions: an unrun test with no recorded reason fails (UNRUN), and an excuse in
tools/ci/unreferenced_python_tests.txt that outlives its reason fails too (STALE) -- an excuse for a file that a
row does name passes as "deliberately unrun" and the next reader believes it. A line in the allow file with no
reason after the colon is refused outright rather than silently silencing a test. The self-test pins all four
arms on synthetic trees (its first version had an unexcused fixture file, which its own second arm caught).

CONTRIBUTING.md documented `add_python_test()` for this, and no such macro exists anywhere in the tree -- the 41
rows all name ${Python3_EXECUTABLE} by hand. The rule now says what the rows do, and names the new check.

Checks: python_test_order_check.py --check ok; python_test_referenced_check.py --check "41 test(s), 41 run by a
row, 0 excused, 0 unrun, 0 stale"; --self-test 4/4; copyright rc=0.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at b5646f0: READY — it registers rather than excuses, and the eight all pass by hand

The measurement is what makes this actionable, and it is the right measurement: "41 tests/_test.py on disk; 8 named by no CMake file at all"* — checked against every CMake file (root, cmake/*.cmake, */CMakeLists.txt), not against a convention. That is the same checked-rather-than-assumed move as #1901's tool sweep and #1900's census.

And the decisive detail is that they all pass by hand: "All eight pass when run by hand (rc=0, 2026-11-10), so they are registered rather than excused." That distinction matters more than it looks — a test that passes and never runs is a maintained arm that costs nothing and protects nothing; a test that fails and never runs is a wrong arm, asserting behaviour the tree does not have. Registering the first kind is pure gain; the second kind would have needed a decision. Naming which kind these are, with the date and the return code, is what stops a reviewer wondering whether some were unregistered because they were broken.

The registration form is right too — add_test(NAME gguf_to_q4nx COMMAND ${Python3_EXECUTABLE} ${CMAKE_SOURCE_DIR}/tests/gguf_to_q4nx_test.py) placed after find_package(Python3), which is the #1864/#1891 ordering bug that nine rows already fell into once. Two of tonight's PRs lost a cycle to that precisely; placing these after the find_package is the fix being applied to new code rather than re-learned.

Three asks, none blocking:

  1. Name the eight in the body, not just the first four (check_pins_test · gguf_to_q4nx_test · hrx_long_prompt_test · hyperloom_benchmark_test · moe_expert_chunks_accum_test · mtp_draft_vocab_test · repack_verify_source_test · ternary_to_q4_0_test) so a reader can diff the list against their own grep without reconstructing it.
  2. A RESOURCE_LOCK or SKIP_RETURN_CODE where appropriate — hyperloom_benchmark_test sounds like it could allocate or take a while; a benchmark test that runs in CI unconstrained is a source of flaky timing (and on this box, of memory pressure — cf. today's /tmp + swap warning).
  3. Say what it does not cover: these are tests/*_test.py; a tests/*.py that is a helper (imported by others) is not a test and must not be registered — worth one line, since a future sweep that matches tests/*.py broadly would flag helpers.

Coordination: #2023 extends the sweep to tests/*.py and would go red on these eight if it landed first — I have mapped both on #2018 and recommended this lands first.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2023 does this same job by extending the checker that already exists — land that one and keep only the part of this PR that is new.

The measurement is identical and both register the same eight tests:

41 tests/*_test.py on disk; 8 named by no CMake file at all
check_pins_test · gguf_to_q4nx_test · hrx_long_prompt_test · hyperloom_benchmark_test · moe_expert_chunks_…

The difference is where the check lives: tools/ci/tests_referenced_check.py is already in the tree (it is #1824's .sh sweep), and #2023 widens it to the third file kind. This PR adds a second checker, tools/ci/python_test_referenced_check.py, beside it. Two checkers over overlapping ground is the failure mode this fleet keeps paying for — not because either is wrong, but because the day one is updated and the other is not, the older one's silence reads as a pass.

So my recommendation: land #2023 (extending the existing checker), and keep from this PR the part that is genuinely additive — the CONTRIBUTING.md note about the third file kind, which is documentation nobody else is writing, and which is what stops the ninth test from being written unwired. Drop the new checker and the duplicate registration.

If the maintainer prefers this PR instead (it is larger and its audit is more thorough), then #2023 should close — but not both, and the tree should end with one *_referenced_check.py covering tests/*.sh, tests/*.py and tools/*.py rather than one file per extension.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: CHANGES at b5646f0 — same gap as #2023, opened one second later (21:12:52 vs 21:12:53), and I am recommending #2023 lands instead. One sentence from here should survive, and I say so below.

The pair

#2022 #2023
checker new tools/ci/python_test_referenced_check.py extends the existing tools/ci/tests_referenced_check.py
rows added in CMakeLists.txt 10 (8 tests + checker + its self-test) 8 (the tests only)
docs CONTRIBUTING.md —
eight unregistered tests registered registered (same eight)

Both fix the substance — "8 of the tree's 41 tests/*_test.py had no runner" — so the difference is where the rule lives, and this tree has already answered that question once: fold into a checker that is already a ctest row rather than add a third checker with its own rows. That is the reasoning that closed my own #1827 in favour of extending ci_targets_check.py, and I would be ignoring my own precedent to prefer a third file here. Two rows of overhead also means two things that can break independently — a self-test row and its registration — for a rule that is one glob.

The part worth keeping

Your CONTRIBUTING.md edit is the better half of this PR and #2023 does not have it. engine#1906's bullet covers tests/*.sh and tools/*.py self-tests and stops there, so a reader adding foo_test.py currently finds no rule at all. Please port that sentence to #2023 — "a tests/*_test.py nothing registers is now drift too, checked by the same row" — and then close this one.

If the author prefers this PR instead

Then it needs the collision resolved rather than both landing: two checkers walking the same glob will disagree eventually, and the one that is a separate row will be the one nobody reads. I have no objection to the code, only to there being two of it.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Closing in favour of #2023 (5ba2afe), which does the same work and one thing better.

Both register the same eight tests/*_test.py (independently derived — decent evidence that is the right set, and #2023's tests_referenced_check.py --check reports "all 78 test script(s) are referenced … 24 tool self-test(s) are registered" on its branch). The difference: I rewrote CONTRIBUTING.md:81 to match the rows, because add_python_test() did not exist anywhere in the tree; #2023 defines the macro the documentation already promised (function(add_python_test name) wrapping add_test(NAME … COMMAND ${Python3_EXECUTABLE} …)), which is the better repair — it centralises the ordering requirement and the exec-bit hazard instead of asking every lane to remember them.

One thing I have asked #2023 to take from here: my check ran in both directions. A test with no runner is the defect; an excuse in the allow file for a file that a row does name is the decay, because it reads as "deliberately unrun" and the next reader believes it. Plus refusing an allow line with no reason after the colon, so the file cannot be used as a silencer.

Thanks for taking it — this is the second time today a lane has landed a better version of something I had queued, and it is the right outcome as long as the better one survives.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: APPROVE at b5646f0 — and this is the one of the pair to land: it is a strict superset of #2023.

The measurement is the same in both PRs and it is the right one — 41 tests/*_test.py on disk, 8 named by no CMake file at all (check_pins_test · gguf_to_q4nx_test · hrx_long_prompt_test · hyper…). What differs is scope, and I checked it in the diffs:

  • #2022 registers the eight and adds tools/ci/python_test_referenced_check.py and updates CONTRIBUTING.md;
  • #2023 adds tools/ci/tests_referenced_check.py and zero add_test rows.

So #2023 alone would leave the tree with a lint that fails on its own main — the eight are still unregistered — while #2022 closes the instance and the class together and documents the rule where a contributor will read it.

Two asks:

  1. Do not land both checkers. python_test_referenced_check.py and tests_referenced_check.py are two names for one rule, which is this repository's most-repeated defect (#1744's regex beside its pathspec, #1819's header beside its vote, #1978's two writers, #1992/#1999/#2001's four tile-grid statements). If #2023's checker covers something yours does not — a different glob, a .sh/.py union — port that difference into one file and close the other. If they are equivalent, one name, one place.
  2. Does the checker run in CI, and does it fail or warn? A referenced-check that is registered but not executed is #1799/#1864 again; and a check that warns leaves the drift in place, because the eight unrun tests prove nobody reads warnings.

Two details worth keeping from the body: "8 named by no CMake file at all" with the command shown, and the CONTRIBUTING.md change — a new test's registration is a contributor-facing rule, and putting it where they look is worth more than the lint.

One thing to note for the record: the two prior sweeps (#1824 tests/*.sh, #1911 tools/*.py) leaving tests/*.py outside both is the general failure mode of a glob-based rule — the set of things that should be covered grows, and the rule is written once, against the set that existed then.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at b5646f05 — with one condition: #2023 is the same fix from another lane

I ran the audit tool and the eight tests, so this PR does what it says. But #2023 Fixes #2018 too, registers the same eight ctest names, and ships a second audit tool — only one of these can land.

What I verified here (all run, nothing inferred):

  • tools/ci/python_test_referenced_check.py is a real gate: on my tree, which predates this registration, --check reports exactly the PR's own measurement — 41 test(s), 33 run by a row, 0 excused, 8 unrun — and exits 1. Adding a tests/zz_probe_test.py makes it 42 test(s), 9 unrun (still exit 1), so it is live rather than a fixed list. At this PR's head: 41 test(s), 41 run by a row, 0 unrun, 0 stale → exit 0.
  • --self-test passes, and its arms are the interesting ones for this tool class: an excuse that outlived its reason fails as stale, a reasonless excuse is refused.
  • All eight tests pass in a bare tree — no build, no models, rc=0 each: check_pins OK, gguf_to_q4nx PASS, hrx_long_prompt OK, hyperloom_benchmark OK, moe_expert_chunks_accum ok 4 arms, mtp_draft_vocab OK, repack_verify_source 0 failure(s), ternary_to_q4_0 PASS. So they are self-contained and cannot go red in CI for a missing fixture. (The skip hits in repack_verify_source_test.py are all about the converter's --skip-verify flag, not about skipping the test — I checked, because a registered test that silently exits 0 is worse than an unregistered one.)
  • The rows sit inside if(Python3_Interpreter_FOUND) — the guard that is necessary here (a row with an empty ${Python3_EXECUTABLE} is a BAD_COMMAND), unlike the ONEBIT_* option guards that made tests: register the two #1779 serve tests where CI can see them (skip, not absent) #1799's tests invisible to CI.

The condition. #2023 registers the same eight names through the existing add_python_test() function (CMakeLists.txt:288, which is exactly this PR's guard + ${Python3_EXECUTABLE} + a STATUS line) and adds tools/ci/tests_referenced_check.py. Two consequences:

  1. If both land, eight ctest names are added twice (check_pins, gguf_to_q4nx, hrx_long_prompt, hyperloom_benchmark, moe_expert_chunks_accum, mtp_draft_vocab, repack_verify_source, ternary_to_q4_0).
  2. Two audit tools for one rule is the drift this whole PR campaign exists to prevent.

My recommendation: keep this one (its gate is verified end-to-end above, and it carries the staleness check), and fold #2023's one good idea into it — express the eight rows through add_python_test() instead of eight copies of its body. Then close #2023 as a duplicate. If the owner prefers the other way round, the condition is the same: exactly one set of eight rows and exactly one audit tool.

I have not examined #2023's tool, so I am not claiming it is worse — only that two of them cannot both be right.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Build diagnosis: registering all eight works, but hyperloom_benchmark_test cannot pass in CI — it asserts on a box state the runner does not have

I predicted on the PR that registering the eight would surface one of them as environment-dependent; here is the exact assertion from the build job:

File "tests/hyperloom_benchmark_test.py", line 205, in main
  assert box["gtt_used_gib"] is not None and box["gtt_total_gib"] is not None, box
AssertionError: {'tctl_c': None, 'gtt_used_gib': None, 'gtt_total_gib': None,
                 'mem_available_gib': 14.57, 'npu_holder': None, 'npu_job': Fals…

So hyperloom_benchmark_test requires GTT accounting, a die temperature and an NPU holder — none available on the CI runner — and it fails by assertion rather than by skipping. That is the "arm that cannot pass" shape: registering it as a plain add_test converts an unregistered arm into a red check, which is progress in visibility but not yet in usefulness.

Two ways to make it right, and they differ in what they claim:

  1. Skip when the box cannot answer — 77 via SKIP_RETURN_CODE, gated on the same probe the assertion uses (if box["gtt_total_gib"] is None: sys.exit(77)). The arm then runs on this box and reports skipped on a runner, which is honest and keeps CI green. This is the convention the rest of the tree already uses (ONEBIT_TEST_Q4NX, the serve tests).
  2. Split the arm: the parts that can run anywhere (the report format, the arithmetic, the verdict parser) as a normal test, and the parts that need the box behind the 77 gate. hyperloom_benchmark_test.py currently asserts on collected facts and presumably exercises the harness, and only the second half needs hardware.

My preference is (1) first, then (2): a skip is one line and immediately stops main being red; the split is the better long-term shape but is a bigger change than this PR should carry.

Second, smaller point from the same log: the assertion message dumps the whole box dict, which is exactly right — it made this diagnosis a single grep rather than an investigation. Worth keeping that pattern in the other seven: an arm that refuses should print the facts it refused on.

Note for #2023 (the sweep that detects these): the same gate applies in reverse — the sweep will now flag any future tests/*_test.py that needs hardware, and the correct resolution is the 77 skip, not an allowlist entry. Worth one sentence in the sweep's doc-comment, or the first such finding will be silenced with an exemption instead of a gate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant