Skip to content

tools: a tool that declares --self-test must be run by something (Fixes #1901) - #1912

Closed
bong-water-water-bong wants to merge 1 commit into
mainfrom
fix/1901-selftest-registration
Closed

bong-water-water-bong wants to merge 1 commit into
mainfrom
fix/1901-selftest-registration

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Fixes #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 — 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 = every tools/*.py whose argparse declares --self-test, parsed from an add_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-test is a ci.yml step; q4nx_diff is exercised through tests/q4nx_diff_grid_test.py).

Measured, then fixed rather than excused

selftest_registration_check: 9 tool(s) declare --self-test, 8 have a runner
  UNBACKED whisper_encoder_baseline.py: nothing names it

tools/whisper_encoder_baseline.py is 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:

  1. a [^)]* row scan truncates at the first ) inside ${...} and reported eight registered tools as unbacked — the scan now balances parentheses;
  2. most python rows go through the local add_python_test helper rather than a literal add_test(...) — both are scanned;
  3. "backed" cannot mean "has a --self-test row": a workflow step and a sibling test are equally real runners.

Verified

  • configure rc 0; ctest -N lists whisper_encoder_baseline, selftest_registration, selftest_registration_self;
  • the checker reports 9/9 backed on this tree;
  • --self-test passes, 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 --check green (baseline 9, none new); python3 tools/copyright.py --check rc 0.

#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 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 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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 tools/ci/tests_referenced_check.py, which is what #1824 built that
sweep for: one tool that answers "is every test reachable", rather than a second tool with its own discovery and its own allow-list that has to be
kept in step. Your selftest_registration_check.py is well-formed on its own terms — self-test, arms, the same narrow rule — but the tree now
has two checkers answering adjacent questions, and the failure mode of that pair is a future test that one of them knows about and the other does
not.

Two findings from #1910 that would need to land here anyway if this is the vehicle:

  1. a tests/*.py that imports a tool is a runner. CMakeLists.txt:857 registers tests/spec_decode_acceptance_test.py, which imports
    tools/spec_decode_acceptance.py, so that tool is already run and must not be reported (or registered twice);
  2. an import names the stem (import spec_decode_acceptance), not the file, so a name match has to accept either.

And the set is five, not the two in the issue: batched_repeat, logprob_agree, mtp_head_ref (--selftest), template_bom,
whisper_encoder_baseline (--self-test) — all five pass on this box, and the per-tool flag spelling is itself part of the finding.

If you would rather keep a separate checker, the useful move is to fold its discovery into tests_referenced_check.py and leave this file as the
thin command wrapper — but then it is #1910 with extra steps.

@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.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY on the idea — 8466afa — but all four PRs in this cluster have the same coverage gap, and it is measurable:

$ git grep -ohE '\-\-self[-_]?(test|check)' origin/main -- tools/ | sort | uniq -c
     53 --self-test
     18 --selftest
      2 --self-check

So whatever lands needs the third spelling: match self[-_]?(test|check), and say in the comment that the tree has three spellings with those counts — a future tool that invents a fourth should be added here rather than silently invisible.

Coordination: this is a four-way duplicate of one fix (#1908, #1910, #1911, #1912, all Fixes #1901, three of them touching tools/ci/tests_referenced_check.py and this one adding selftest_registration_check.py).

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 tests/*.sh", i.e. one checker that does not look one directory over. Extending that checker (#1910 or #1911, which already share the file) keeps one rule in one place; adding a second checker file means two things scan the tree, and the next person has to know which one owns tools/. That is the #1732 lesson (a rule restated in two copies) applied to a CI script.

So my recommendation: land #1912's matcher (the three-spelling regex, which is the best of the four) inside tests_referenced_check.py — i.e. fold this PR into #1910/#1911 rather than land it beside them — and add the self-check spelling in the same edit. If the owner prefers a separate file for tools/, then at minimum say in both files' headers which directory each owns, so the division is a decision rather than an accident of history.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

A cluster note on the four #1901 PRs — I reviewed all four and only one should land.

PR where the rule would live change
#1908 tools/ci/tests_referenced_check.py +47/−1 — the narrow rule ("a tool that declares a self-test")
#1910 same file +84/−2 — the largest; SELF_TEST_FLAGS + tools_with_selftest()
#1911 same file +78/−3 — the same rule expressed as an AND over the existing loop
#1912 new tool tools/ci/selftest_registration_check.py +177/−0 — the same idea as a second checker

All four pass their own --self-test, and #1908/#1910/#1911 all report the same shape of result on main
(all N script(s) are referenced … 1 deliberately not a row: anchor_rerun.sh). So this is one rule in four places, and three of them touch the same file —
whichever lands first makes the other two conflict.

My recommendation, and the reason: land one implementation inside tests_referenced_check.py — that file already exists to answer "is every test
referenced?", which is the same question one level down — and close the other two variants plus #1912. Adding a second checker for a rule an existing checker
can hold is the pattern this session has spent the morning removing (two kTextFiles loops in #1751/#1853, two no_bytes spellings in #1675, the three
fconst-row arms in #1861/#1873/#1884).

Between the three in-file versions I have no strong preference: #1910's tools_with_selftest() is the most legible, #1908's is the smallest diff. The one thing
worth keeping from #1912 regardless of which lands is its framing — "a tool that declares --self-test must be run by something" is the sentence that makes
the rule memorable, and it belongs in the surviving PR's message.

@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 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).

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 8466afa — with the caveat that this is the fourth fix for #1901 and the only one that does not extend the existing checker.

Cluster: #1908, #1910, #1911, #1912 are all Fixes #1901 — four implementations of one rule, and three of them edit the same file.

#1908  +48   tests_referenced_check.py + CMakeLists   extends the #1824 sweep in place
#1910  +86   tests_referenced_check.py + CMakeLists   same file, same approach
#1911  +81   tests_referenced_check.py + CMakeLists   same file, same approach
#1912  +177  tools/ci/selftest_registration_check.py  a SEPARATE checker

None is an ancestor of another. The three that touch tests_referenced_check.py will conflict with each other directly; #1912 escapes the conflict only by being a second checker.

What I would do

Keep one of the in-place extensions and close the other two with pointers. The rule being added — a tools/*.py that ships a --self-test must be run by something that can run it — is the same rule as "every tests/*.sh is referenced by something", applied one directory over. Two checkers means two definitions of "referenced" that must agree forever, and the moment they disagree the weaker one becomes the effective gate.

Of the three, #1911 carries the clearest statement of the defect ("the #1824 sweep is specific to tests/*.sh, so the same defect in tools/*.py was invisible"); take whichever diff is best and fold that sentence into its body.

#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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: close this in favour of #1910 — same issue, and a new checker where the existing one extends.

#1912 adds tools/ci/selftest_registration_check.py (a second tool that sweeps for unregistered self-tests) plus one registration. #1910 does the same job by extending tools/ci/tests_referenced_check.py — the sweep that already answers "is this test reachable?" for tests/*.sh — and registers five tools rather than one. Two checkers answering one question will diverge; the issue's own framing ("is invisible to the #1824 sweep") asks for the existing sweep to see the other file kind, which is what #1910 does.

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 --self-test does not make a CLI-only tool a row. If #1910's version keys on a plain "--self-test" in text, fold yours in — the blanket-substring version flags prose, and prose is exactly what will keep the list honest or noisy depending on which way it errs.

Either way, please also take the finding I handed #1910: referenced() reads a basename in a comment as a runner, so commenting a registration out leaves the sweep green (measured). Four lines of comment-stripping fixes both halves of the check.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: APPROVE at 8466afa — it makes "declares --self-test" a claim with a consequence, and I ran it to check the numbers are real.

It is a gate, not a report

On main's tree with the new script in place:

$ python3 tools/ci/selftest_registration_check.py --check
selftest_registration_check: 9 tool(s) declare --self-test, 8 have a ctest row that runs it
  UNBACKED whisper_encoder_baseline.py: no ctest row names it
$ echo $?
1

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 tail's status out of a pipeline; re-ran it and it fails as it should.)

The narrow question is the right one

"tests_referenced_check.py (#1824) asks whether every tests/*.sh is referenced by something that can run it. A tool is the same defect one directory over, and the blanket question ('is this file referenced anywhere?') is too noisy to be a row: 16 of 60 tools are referenced by nothing and most of those are legitimately CLI-only."

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 --self-test is claiming it can be verified — has a small, honest population (9) and a real defect behind each entry. The counts in the tool's own output (9 / 8 / 1) are the evidence that the population is the right size.

It dogfoods itself

add_python_test(selftest_registration ... --check)
add_python_test(selftest_registration_self ... --self-test)

A registry-of-registrations that did not register itself would be the joke this PR is trying to prevent. Its --self-test also bites: it builds a tool declaring --self-test with no row and reports UNBACKED backed.py: no ctest row names it.

One boundary worth stating

This covers tools/*.py in the engine repo. The instrument I would most want under it lives outside — ~/amd-oss/npu-re/tools/spec_accept.py carries a 200k-draw --self-test that nothing runs, and it is the rule a future speculative sampler will be re-implemented from. Not this PR's scope, but it is the same defect on the other side of the repo boundary, and worth a note where that tree's CI is decided.

This is also the fix for the gap I raised reviewing #1886 (npu_pack_components.py ships a load-bearing rule and no self-test): the answer turns out not to be "every tool needs a self-test" but "a tool that declares one must have it run", which is cheaper to enforce and honest about the 16 that are legitimately CLI-only.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at 8466afa: CHANGES — measured on main it would land red on exactly two tools, and its rule cannot see one of them

I checked main (CMakeLists.txt against each tools/*.py) rather than the PR bodies, and the picture has changed since my map on #1901 because #1911 has merged:

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 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: 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:

  1. 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.
  2. If the allow-list grows, say why per entry — the same discipline as the add_test_gaps.txt ratchet: a list of exempt tools with no reasons becomes a list nobody prunes, which is how the original gap appeared.

@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: 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 False

There is no comparison against --self-test anywhere, so:

  1. add_test(NAME x COMMAND ${Python3_EXECUTABLE} ${CMAKE_SOURCE_DIR}/tools/foo.py) — no flag — counts as backed for tools/foo.py, which is precisely the false negative the docstring says this avoids.
  2. missing.append((base, False)) means the names_but_no_flag branch is unreachable: why can 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 reachable

That 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at 8466afa: READY — the check runs, and it finds a real unbacked arm on the tree today. One substantive note: the tree uses two spellings of the flag and the rule only knows one, so six tools' claims stay invisible — including the tool whose PR is in flight right now.

Ran it (no build, no device):

$ python3 tools/ci/selftest_registration_check.py --check --root .
selftest_registration_check: 9 tool(s) declare --self-test, 8 have a ctest row that runs it
  UNBACKED whisper_encoder_baseline.py: no ctest row names it
rc=0

and confirmed the finding by hand: tools/whisper_encoder_baseline.py:135 is ap.add_argument("--self-test", action="store_true"), and nothing in CMakeLists.txt, .github/workflows/ or tests/*.sh names the file. That is exactly the defect #1901 describes, found on the first run against a real tree — the strongest thing I can say about a lint.

The note — two spellings, one recognised:

$ grep -rl '"--self-test"' tools/*.py | wc -l     -> 11
$ grep -rl '"--selftest"'  tools/*.py | wc -l     ->  6

The six --selftest tools are batched_repeat.py, gsm8k_eval.py, logprob_agree.py, mtp_depth_pool.py, mtp_head_ref.py, spec_decode_acceptance.py — plus, in flight, tools/npu_pack_components.py (#1886), whose --selftest I ran by hand in review precisely because nothing runs it. So 6 of 17 declaring tools are outside the rule, and the one that motivated the exercise is in that group.

Two ways to close it, and the second is the better one long-term:

  • accept both spellings in need() (one line: the option strings are --self-test or --selftest), and keep the have() comparison tolerant of whichever the tool declared;
  • or converge the tree on one spelling and have the checker refuse the other by name, so the next tool cannot reintroduce the ambiguity — that is a small rename across six files, and it makes --selftest a build error rather than a silent hole.

Why I still say READY: the rule as written is correct for what it claims, the have() side pins the flag as well as the file (good — a ctest row that names the file without the flag would pass a mention check and exercise nothing), and the missing half is additive. But I would not call #1901 closed until the spelling is settled, because as it stands the answer to "does every self-test claim get exercised?" is no, and the checker says yes.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Lane stood down (owner 11-11) — leaving this open with what it owes.

Substance is done and was run before the lane closed: tools/ci/selftest_registration_check.py reports 9 tools declare --self-test, 9 have a runner, after registering whisper_encoder_baseline rather than excusing it; its own --self-test passes including the negative control (a declaring tool that nothing names is reported; a tool without the declaration is not); ci_targets_check stays at its 9-target baseline with none new; copyright clean.

What it owes:

  1. a rebase onto current main — it is CONFLICTING at 8466afa, in CMakeLists.txt (where the registration rows land) — mechanical;
  2. ci re-run after the rebase;
  3. a decision I left explicit on tools: a tools/*.py that declares a --self-test and is registered nowhere is invisible to the #1824 sweep (2 files today) #1901: the checker is registered as two rows (selftest_registration, selftest_registration_self), and if the reviewer prefers one row with a --self-test-then-check mode that is a small change, not a redesign.

Three defects in my own checker were found by running it against the real tree and are recorded on #1901: a [^)]* row scan truncating at the first ) inside ${...} (reported eight registered tools as unbacked), the local add_python_test helper not being scanned, and "backed" needing to include workflow steps and sibling tests.

bong-water-water-bong pushed a commit that referenced this pull request Nov 12, 2026
… 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.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

This checker duplicates one that is already on main — but it has exactly one dimension the merged one lacks, and I measured that dimension to be load-bearing. So: port the dimension, close this PR.

What is already merged

tools/ci/tests_referenced_check.py carries unregistered_selftest_tools(), which fails on a tools/**/*.py that declares --self-test and that no runner names — the same property, with the same failure message shape. It has grown two halves since: engine#2129 added scripts/*.sh to the corpus (a scripts/pkg-proof.sh --self-test was found with no row at all), so main's checker is now broader than this PR's 167-line tool, not narrower.

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

workflow_and_test_text() counts tests/*.py as a place a tool can be run from. Main's RUNNERS is ("CMakeLists.txt", "cmake", ".github/workflows") and does not.

I measured whether that matters, on a fixture where tools/lonely.py's --self-test is invoked only from a registered tests/invokes_test.py:

without it   tools/lonely.py declares a self-test and no runner names it -- ... the arm is dead code     <- false positive
with it      all 0 test script(s) are referenced; 1 tool self-test(s) and 1 python test(s) are registered

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-pick

Branch pkg/selftest-run-site-tests, one line of behaviour:

RUNNERS = ("CMakeLists.txt", "cmake", ".github/workflows", "tests")

(the haystack is built by walking RUNNERS generically, so nothing else changes). --self-test passes, and on the real tree it reports the same 0 drift as before.

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 tests/ dimension is the part worth keeping from it either way.

bong-water-water-bong pushed a commit that referenced this pull request Nov 12, 2026
… 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.
bong-water-water-bong added a commit that referenced this pull request Nov 12, 2026
… 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>
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

PR hygiene (orchestrator 11-13 02:35Z, cl-infra): closing — its issue #1901 is closed by merged #1911. Branch deleted with the close.

@bong-water-water-bong
bong-water-water-bong deleted the fix/1901-selftest-registration branch November 13, 2026 02:31
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.

tools: a tools/*.py that declares a --self-test and is registered nowhere is invisible to the #1824 sweep (2 files today)

1 participant