Repository navigation
tests: register the eight tests/*_test.py nothing ran, and audit the third file kind (engine#2018) - #2022
tests: register the eight tests/*_test.py nothing ran, and audit the third file kind (engine#2018)#2022bong-water-water-bong wants to merge 1 commit into
Conversation
…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.
VERDICT at
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
#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
left a comment
There was a problem hiding this comment.
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.
|
Closing in favour of #2023 ( Both register the same eight 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
left a comment
There was a problem hiding this comment.
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.pyand updatesCONTRIBUTING.md; - #2023 adds
tools/ci/tests_referenced_check.pyand zeroadd_testrows.
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:
- Do not land both checkers.
python_test_referenced_check.pyandtests_referenced_check.pyare 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/.pyunion — port that difference into one file and close the other. If they are equivalent, one name, one place. - 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.
VERDICT: READY at
|
Build diagnosis: registering all eight works, but
|
What was measured
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_testAll eight pass when run by hand (
rc=0, 2026-11-10), so they are registered rather than excused — eight rows of the formplaced after
find_package(Python3)(:761) astests/python_test_order_check.pyrequires. 41 of 41 are now run by a row.The audit, which is the part that lasts
tools/ci/python_test_referenced_check.pyis the third file kind after #1824'stests/*.shand #1911'stools/*.py, and it checks both directions:tests/*_test.pythat no row names andtools/ci/unreferenced_python_tests.txtdoes not excuse;(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:81said "a row that runs Python goes throughadd_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-test4/4 ·copyright.py --checkrc=0.