Repository navigation
tools: a tool that declares --self-test must be run by something (Fixes #1901) - #1912
bong-water-water-bong wants to merge 1 commit into
Conversation
#1901) #1824's row asks whether every tests/*.sh is referenced by something that can run it. The same defect one directory over is invisible to it, and it is in tools/*.py: a tool that ships --self-test is CLAIMING it can be verified without its real inputs, and if nothing runs that flag the claim is unbacked -- the arm is maintained in the same commits as the logic and never executes. The blanket question ('is this file referenced?') is too noisy (16 of 60 tools are referenced by nothing, most legitimately CLI-only), so the row keys on the DECLARATION: --self-test parsed out of an add_argument() call, never grepped from a comment. A declaring tool counts as backed when a ctest row, a workflow step or another test names it -- pin_watch runs its --self-test as a ci.yml step, q4nx_diff is exercised through tests/q4nx_diff_grid_test.py -- because all three are places a tool actually runs. Measured on this tree: 9 tools declare --self-test, 8 were backed, and the one genuinely unbacked (tools/whisper_encoder_baseline.py) is registered here rather than excused -- its self-test is model-free by construction (window arithmetic and the mel shape on synthetic input) and passes in 0.2 s. Three things the real tree taught the check while writing it (each found by running it, not by reading): * a row scan truncates at the first ')' inside ${...}, which reported eight registered tools as unbacked -- the scan now balances parentheses; * most python rows go through the local helper, not a literal add_test -- both are scanned; * 'backed' cannot mean 'has a --self-test row': a workflow step and a sibling test are equally real runners. Verified: configure rc 0 and ctest -N lists whisper_encoder_baseline, selftest_registration and selftest_registration_self; the checker reports 9/9 backed; --self-test passes including its negative control (a declaring tool nothing names is reported, a tool without the declaration is not); tools/ci/ci_targets_check.py --check is green; copyright.py --check rc 0.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 8466afa: CHANGES
Checked at 8466afa. selftest_registration_check --self-test is ok, and --check at this head gives rc 0 ("9 tool(s) declare --self-test, 9 have a ctest row").
Ground truth on current main (extracted tree): the tools/**/*.py that name a --self-test/--selftest flag, minus those a runner invokes (CMakeLists.txt, cmake/*.cmake, .github/workflows/*.yml) and minus those exercised through their own registered tests/*_test.* (gsm8k_eval, mtp_depth_pool, q4nx_diff, seal_manifest, split_ceiling). That leaves five whose self-test nothing runs: batched_repeat, logprob_agree, mtp_head_ref, spec_decode_acceptance (all spelled --selftest) and whisper_encoder_baseline (--self-test). Each of the four #1901 PRs, run against that tree:
| PR | flags on main |
registers |
|---|---|---|
| #1908 | whisper_encoder_baseline only |
whisper_encoder_baseline |
| #1910 | batched_repeat, logprob_agree, mtp_head_ref, whisper_encoder_baseline, + template_bom |
the same |
| #1911 | all five | all five; each --selftest/--self-test passes when run directly |
| #1912 | whisper_encoder_baseline only |
whisper_encoder_baseline |
All four conflict-or-overlap on Fixes #1901 (#1908, #1910 and #1911 conflict pairwise; #1912 adds a second checker), so one should land.
Its "9 declare" counts only the --self-test spelling, so batched_repeat, logprob_agree, mtp_head_ref and spec_decode_acceptance (all --selftest) are outside it, and it finds one of the five. It is also a second checker beside tests_referenced_check.py for one question. #1911 extends the existing sweep and finds all five. I'd close this in its favour.
|
Same issue as #1910 (and #1908/#1911) — and the separate-checker shape is the one argument against this PR. Four PRs landed on engine#1901 in the same hour. #1910 extends the existing Two findings from #1910 that would need to land here anyway if this is the vehicle:
And the set is five, not the two in the issue: If you would rather keep a separate checker, the useful move is to fold its discovery into |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Consolidation: three PRs fix #1901, and #1911 is the one to merge — close this.
All three ask the same question (a tools/*.py that ships --self-test and is named by no runner), and #1911 states the predicate most precisely: it counts a tool as unregistered when no RUNNER names it — "CMakeLists/cmake/workflows — a citation in another tool's source is not a runner: that is the 5-vs-2 difference" — which is the distinction that changes the answer from two tools to five. It also measures the five (58-277 ms, device-free, so they are rows rather than allow-list excuses) and registers them through add_python_test(), reporting that its own first version used the raw ${Python3_EXECUTABLE} form and was caught by the ordering guard on tools/mtp_head_ref.py — the one of the five without an exec bit.
So: merge #1911, close this and the sibling. Worth folding across if the bodies are squashed: the "5-vs-2" sentence (the predicate), because that is the part a future reader will need when the count changes.
|
VERDICT: READY on the idea —
So whatever lands needs the third spelling: match Coordination: this is a four-way duplicate of one fix ( Which to keep — and the choice cuts against this PR, so I want to be explicit about why: the defect is "the #1824 sweep is specific to So my recommendation: land #1912's matcher (the three-spelling regex, which is the best of the four) inside One thing this PR has that the others should keep either way: a new checker can be tested by running it against the tree and asserting a known set (e.g. "these N tools declare a self-test; these M are referenced"), which is a stronger self-check than "the script exits 0". Whichever file survives should carry that list. |
|
A cluster note on the four #1901 PRs — I reviewed all four and only one should land.
All four pass their own My recommendation, and the reason: land one implementation inside Between the three in-file versions I have no strong preference: #1910's |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 8466afa — a second checker for a rule the existing one can hold; land it inside tests_referenced_check.py instead (see the cluster note above).
|
VERDICT: READY at Cluster: #1908, #1910, #1911, #1912 are all None is an ancestor of another. The three that touch What I would doKeep one of the in-place extensions and close the other two with pointers. The rule being added — a Of the three, #1911 carries the clearest statement of the defect ("the #1824 sweep is specific to #1912 stays defensible only if the rule is genuinely different in kind rather than in directory — in which case say why, because that is the whole argument for a second tool. |
|
VERDICT: close this in favour of #1910 — same issue, and a new checker where the existing one extends. #1912 adds What is worth keeping from here, and it is not nothing: your detection is argparse-parsed rather than a substring, so a docstring that mentions Either way, please also take the finding I handed #1910: |
|
VERDICT: APPROVE at It is a gate, not a reportOn main's tree with the new script in place: rc=1 on an unbacked tool — which is the property that makes it a gate rather than a dashboard, and the one I have had to argue for repeatedly tonight. (My first measurement of that exit code was wrong because I read The narrow question is the right one" That reasoning is why this is worth landing: a presence-based rule over 60 files would have been dead on arrival and disabled within a week. The claim-based rule — a tool that ships It dogfoods itselfA registry-of-registrations that did not register itself would be the joke this PR is trying to prevent. Its One boundary worth statingThis covers This is also the fix for the gap I raised reviewing #1886 ( |
VERDICT at
|
| tool | declares via | on main now |
|---|---|---|
batched_repeat, logprob_agree, mtp_head_ref, whisper_encoder_baseline (+spec_decode_acceptance) |
add_argument("--self-test") |
registered (tool_selftest_* family from #1911) |
pin_watch |
add_argument("--self-test") |
unregistered |
template_bom |
usage-documented [--self-test], dispatched from argv |
unregistered |
So this PR's own check would fail on main for those two the moment it lands — this one registers only whisper_encoder_baseline (already registered now) plus its own checker and that checker's self-test. A guard that arrives before the fixes it demands is CI red with a correct message, which is the most expensive shape of correct.
And the deeper problem, which is the reason to change the rule rather than just add two rows: the detector looks for add_argument("--self-test"). template_bom declares the flag in its usage block and dispatches from argv; mtp_head_ref spells it --selftest. A rule keyed on one spelling reports a clean tree while template_bom sits in it — the same defect the sweep exists to find, one level up.
Why I still want this PR to be the one that survives: separating the rule into tools/ci/selftest_registration_check.py is right (the tests/*.sh sweep should stay about tests/*.sh), and its two-sided self-test — a backed tool is not reported, an unbacked one is — is the piece #1911 lacks. A check that cannot fail is precisely what #1901 is about.
To get to READY: (1) register pin_watch and template_bom; (2) teach the detector all three spellings; (3) keep the two-sided self-test. That is a small change to an otherwise better-structured PR.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 8466afa2 — the self-test passes at this head (I ran it: 0 failure(s)), and the framing is the part worth keeping.
"A tool that ships --self-test is claiming it can be verified without its real inputs — if nothing runs that flag, the claim is decoration." That is the same defect as an add_test naming a target nobody builds (#1858/#1799/#1822) and as a test that exits 0 on skip (#1851), one directory over: the artefact exists, the verification does not, and nothing reports the gap. Extending #1824's rule from tests/*.sh to tools/*.py is exactly right, because the --self-test convention is where this repo puts its own proof that a tool works.
Two notes:
- Treat "has a self-test but is not invoked" as its own category in the output, distinct from "referenced by nothing" — the fix differs: the first wants a wiring line (a ctest entry, a CI step), the second may be a legitimately standalone script. One message for both will get the second kind waived as noise and take the first with it.
- If the allow-list grows, say why per entry — the same discipline as the
add_test_gaps.txtratchet: a list of exempt tools with no reasons becomes a list nobody prunes, which is how the original gap appeared.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: REQUEST CHANGES on one point — the check documents a flag comparison it does not perform, so a row that names a tool without running its self-test still counts as backing. Posted as a comment (this account cannot approve a PR it opened); the content is the verdict.
The framing is right and the tool is worth having. "A tool that ships --self-test is claiming it can be verified without its real inputs; if nothing runs that flag, the claim is unbacked — the arm is maintained in the same commits as the logic and never executes anywhere" is a sharper version of #1824, and narrowing from "is this file referenced?" to "is this claim backed?" is what makes it a row instead of noise. declares_self_test() parsing the source rather than grepping (so a docstring mention is not a declaration) is the right instinct, and the parenthesised add_test scan that also understands the local add_python_test helper — with the comment recording that both bugs were found by running it against the real tree — is exactly how this kind of check earns trust.
The defect: the flag is never compared, and the variable that would report it is dead. The docstring says
Registration is not enough on its own (#1826's lesson): a row that names the file but runs it without the flag, or with the wrong flag, would satisfy a mention check and still never exercise the self-test. This compares the flag too.
and the header print says "have a ctest row that runs it", but check() does:
runners = [h for h in hay if base in h]
if not runners:
missing.append((base, False)) # <- the second element is always FalseThere is no comparison against --self-test anywhere, so:
add_test(NAME x COMMAND ${Python3_EXECUTABLE} ${CMAKE_SOURCE_DIR}/tools/foo.py)— no flag — counts as backed fortools/foo.py, which is precisely the false negative the docstring says this avoids.missing.append((base, False))means thenames_but_no_flagbranch is unreachable:whycan only ever print "no ctest row names it", so the more informative message is dead code.
Suggested shape — keep the wide haystack (workflows and tests/*.py genuinely run tools: pin_watch.py in a ci.yml step, q4nx_diff.py from a grid test), but compare the flag where a flag is meaningful, and compute the second element instead of hardcoding it:
for t in declaring:
base = os.path.basename(t)
by_row = [r for r in rows if base in r]
by_other = [h for h in workflow_and_test_text(root) if base in h]
if any("--self-test" in r for r in by_row) or by_other:
continue
missing.append((base, bool(by_row))) # names_but_no_flag is now reachableThat keeps the two legitimate "not a ctest row" cases passing while making the flag claim true, and it makes the message match the check.
Missing arm that would have caught it: the self-test's negative control currently removes the row. Add one where a row names the tool without --self-test and assert it is reported names_but_no_flag=True. Right now the self-test asserts the two ends (backed, and nothing at all) and skips the middle — which is the state the tool's own docstring calls the interesting one.
Cross-PR note, since it is the same defect family: #1919 pins a ctest row that ci.yml cannot reach, and this check would call that row backed because it names the tool and passes the flag — correct by this tool's definition (the row exists and runs the self-test), but it is worth saying out loud that "backed by a row" and "executed by automation" are still two different facts, and only the first is what this check can see. If that distinction matters, it is a separate question for the row's if(ONEBIT_NPU) placement, not for this tool.
|
VERDICT at Ran it (no build, no device): and confirmed the finding by hand: The note — two spellings, one recognised: The six Two ways to close it, and the second is the better one long-term:
Why I still say READY: the rule as written is correct for what it claims, the |
|
Lane stood down (owner 11-11) — leaving this open with what it owes. Substance is done and was run before the lane closed: What it owes:
Three defects in my own checker were found by running it against the real tree and are recorded on #1901: a |
… run site (port of #1912's one extra dimension) tools/ci/tests_referenced_check.py's RUNNERS are the places a self-test can be run from: the root CMakeLists, cmake/, and the workflows. A python test is a fourth, and it is missing. Measured on a fixture where tools/lonely.py's --self-test is invoked only from tests/invokes_test.py (itself registered in cmake/tests-x.cmake): without this change tools/lonely.py declares a self-test and no runner names it -- ... the arm is dead code with it all 0 test script(s) are referenced; 1 tool self-test(s) and 1 python test(s) are registered So it is a false positive, and a misleading one: the fix a reader would apply is a redundant ctest row for an arm that already runs, which is how a corpus grows rows nobody needs. Four characters of behaviour -- "tests" added to RUNNERS -- because the haystack is already built by walking RUNNERS generically. engine#1901's tools half and engine#2129's scripts half both stay; this is the run-site half. Held rather than opened: engine PR #1912 adds a whole second checker (167 lines, tools-only) for the same property, so the order should be this port first, then #1912 closed as superseded -- its one genuine extra dimension is what is ported here.
|
This checker duplicates one that is already on What is already merged
Two checkers for one property is how a corpus gets two sets of exceptions and one of them goes stale, so I would not merge this as a second row. The one thing this PR has that main does not
I measured whether that matters, on a fixture where So the gap is real and its failure mode is worse than silence: the "repair" a reader applies to that message is a redundant ctest row for an arm that already runs, which is how a corpus acquires rows nobody needs. The port, ready to cherry-pickBranch RUNNERS = ("CMakeLists.txt", "cmake", ".github/workflows", "tests")(the haystack is built by walking I am at this lane's 2-PR cap, so it is a branch rather than a PR; if you would rather keep this PR open and take the port here, it is four characters and yours to fold in — the |
… run site (port of #1912's one extra dimension) tools/ci/tests_referenced_check.py's RUNNERS are the places a self-test can be run from: the root CMakeLists, cmake/, and the workflows. A python test is a fourth, and it is missing. Measured on a fixture where tools/lonely.py's --self-test is invoked only from tests/invokes_test.py (itself registered in cmake/tests-x.cmake): without this change tools/lonely.py declares a self-test and no runner names it -- ... the arm is dead code with it all 0 test script(s) are referenced; 1 tool self-test(s) and 1 python test(s) are registered So it is a false positive, and a misleading one: the fix a reader would apply is a redundant ctest row for an arm that already runs, which is how a corpus grows rows nobody needs. Four characters of behaviour -- "tests" added to RUNNERS -- because the haystack is already built by walking RUNNERS generically. engine#1901's tools half and engine#2129's scripts half both stay; this is the run-site half. Held rather than opened: engine PR #1912 adds a whole second checker (167 lines, tools-only) for the same property, so the order should be this port first, then #1912 closed as superseded -- its one genuine extra dimension is what is ported here.
… run site (port of #1912's one extra dimension) (#2306) * tests_referenced: a tests/*.py that invokes a tool's --self-test is a run site (port of #1912's one extra dimension) tools/ci/tests_referenced_check.py's RUNNERS are the places a self-test can be run from: the root CMakeLists, cmake/, and the workflows. A python test is a fourth, and it is missing. Measured on a fixture where tools/lonely.py's --self-test is invoked only from tests/invokes_test.py (itself registered in cmake/tests-x.cmake): without this change tools/lonely.py declares a self-test and no runner names it -- ... the arm is dead code with it all 0 test script(s) are referenced; 1 tool self-test(s) and 1 python test(s) are registered So it is a false positive, and a misleading one: the fix a reader would apply is a redundant ctest row for an arm that already runs, which is how a corpus grows rows nobody needs. Four characters of behaviour -- "tests" added to RUNNERS -- because the haystack is already built by walking RUNNERS generically. engine#1901's tools half and engine#2129's scripts half both stay; this is the run-site half. Held rather than opened: engine PR #1912 adds a whole second checker (167 lines, tools-only) for the same property, so the order should be this port first, then #1912 closed as superseded -- its one genuine extra dimension is what is ported here. * tests_referenced: the tests/ run site belongs to the tool-self-test gate, not the shared corpus Caught in my own change before it merged. Adding "tests" to `RUNNERS` gave BOTH gates that haystack, and there are two gates: unregistered_selftest_tools() asking "does anything run this tool's --self-test?" unregistered_py_tests() asking "does anything run this python test?" The python-test gate's "registered" test is `any(q.name in t for t in runner_text)`; with `tests/` in the haystack, a test's own file is in the text it is searched against, so `tests/lonely_test.py` mentioning its own path **registered itself**: before the split: tests_referenced: ... 1 python test(s) are registered <- the self-naming test passed after the split: tests_referenced: tests/lonely_test.py is a test that no runner names -- add a ctest row ... The guard against a test naming itself lives in the path-shaped bookkeeping of the other function, not in this haystack, so sharing one tuple removed a protection without touching the line that documents it. The corpora are now separate: RUNNERS = ("CMakeLists.txt", "cmake", ".github/workflows") TOOL_RUNNERS = RUNNERS + ("tests",) <- the tool-self-test gate only Both directions measured on fixtures: a tool whose --self-test is invoked ONLY from a registered python test -> registered (the improvement, kept) a python test that names only itself -> drift (the regression, fixed, arm added) The arm fails if the two corpora are ever merged again (`RUNNERS = TOOL_RUNNERS` -> 1 FAILURE), so this cannot come back silently. `--self-test` 0 failures; the real tree still reports 86 test scripts, 44 tool self-tests and 45 python tests. --------- Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
Fixes #1901. #1824's row asks whether every
tests/*.shis referenced by something that can run it; the same defect one directory over is invisible to it, and it is intools/*.py. A tool that ships--self-testis claiming it can be verified without its real inputs — if nothing runs that flag, the claim is unbacked: the arm is maintained in the same commits as the logic and never executes anywhere.The rule
The blanket question ("is this file referenced?") is too noisy — 16 of 60 tools are referenced by nothing and most are legitimately CLI-only. So the row keys on the declaration:
need= everytools/*.pywhose argparse declares--self-test, parsed from anadd_argument()call, never grepped out of a docstring;have= a ctest row, a workflow step, or another test that names the file — those are all places a tool actually runs (pin_watch's--self-testis aci.ymlstep;q4nx_diffis exercised throughtests/q4nx_diff_grid_test.py).Measured, then fixed rather than excused
tools/whisper_encoder_baseline.pyis now registered — its self-test is model-free by construction (window arithmetic and the mel shape on synthetic input) and passes in 0.2 s — so the row starts 9/9 backed rather than green only because of an allow-list.Note:
mtp_head_ref.py, which the issue counted, does not declare--self-test(it is a reference implementation imported by other tools); the parse-based rule drops it correctly, which is why this reports one rather than two.Three things the real tree taught the check
Each was found by running it, not by reading it:
[^)]*row scan truncates at the first)inside${...}and reported eight registered tools as unbacked — the scan now balances parentheses;add_python_testhelper rather than a literaladd_test(...)— both are scanned;--self-testrow": a workflow step and a sibling test are equally real runners.Verified
ctest -Nlistswhisper_encoder_baseline,selftest_registration,selftest_registration_self;--self-testpasses, including its negative control: a declaring tool that nothing names is reported, and a tool without the declaration is not;tools/ci/ci_targets_check.py --checkgreen (baseline 9, none new);python3 tools/copyright.py --checkrc 0.