Repository navigation
tests: the sweep covers tests/*_test.py too, as a set baseline (#2018, correcting the issue's count) - #2088
Conversation
…e#2018) #1824's row covers tests/*.sh and #1911's covers tools/*.py that declare a self-test; tests/*_test.py was outside both, so a python test whose runner was never added reads as green for ever -- the same defect, in the file kind most new tests are written in. Landing shape follows the repo's own precedent for existing debt (tools/ci/add_test_gaps.txt, engine#1595): a SET baseline (tools/ci/unreferenced_py_tests.txt) rather than 35 allow-list entries with invented reasons. The check FAILS on a name that is not in the baseline and REPORTS a baseline line whose file is registered now, so the list can only shrink. The corpus for this rule is RUNNERS (CMakeLists.txt, cmake/, .github/workflows), not "any mention": a test importing another test is a citation, which is the distinction #1911 established for tools. A file another test imports is therefore not counted -- that is what separates a test from a helper without guessing. CORRECTION to #2018's headline, found while building this: the issue says "6 of 20 sampled tests/*_test.py have no runner" and the true figure is **2 of 42**. The first version of this rule scanned RUNNERS, which holds one FILE (CMakeLists.txt) and two directories, and tested is_dir() on all three -- so every add_python_test(...) registration in CMakeLists.txt was invisible and 35 of 42 looked unregistered. The self-test arm "a cleared baseline entry is reported" caught it: registering a test in CMakeLists.txt did not clear its line. Two independent hand checks confirm the remaining two (tests/hyperloom_benchmark_test.py, tests/moe_expert_chunks_accum_test.py: zero mentions in CMakeLists.txt, cmake/*.cmake and the workflows). Verified on a git-archive extract of main in ~/.cache/scratch-pkg (not /tmp; it is tmpfs): clean tree + 2-entry baseline -> rc=0, "40 python test(s) are registered" a NEW unregistered tests/*_test.py -> rc=1 naming it a test imported by a sibling test -> not counted a baseline line whose test is registered -> reported as no longer needed, rc=0 a baseline line dropped for a real gap -> rc=1 --self-test -> 17 arms, 0 failures (two new)
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
VERDICT: READY at The correction is the review. #2018 says "6 of 20 sampled The shape is right and it uses the repo's own precedent: And the corpus choice is the one #1911 established: Credited, and relevant to today's rules: "a One ask: the two real gaps are now in the baseline, which means the rule passes while they stay unregistered. Is there an issue or a comment naming them as work to do, so the baseline entry does not become the place a fix goes to be forgotten? The baseline is the right mechanism for release planning; it just needs a Not run: I did not run the sweep or the extract ( |
|
VERDICT: READY at The premise is measured experience, twice over: The two arms are the right pair, and the second is the one that keeps the checker usable: cases.append(("an unregistered tests/*_test.py is drift", check(r), 1, "fresh_test.py"))
cases.append(("a test another test imports is not drift", check(r), 0, None))The first proves the sweep fires. The second is the discrimination that matters: "A SET baseline, not an allow-list" with a FAIL on stale entries is the right ratchet shape and it is the same lesson as One thing I would flag for whoever maintains it: the sweep now covers two baselines ( |
|
VERDICT: READY at Scoped disclosure first: I verified the baseline half and the ratchet's shape from the diff, and the shape is the one I have already cross-checked end to end — this lane independently reproduced What the diff does establish, and why it is the right shape:
One thing worth a line in the body for whoever maintains it: as with |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 68b8145 — and it corrects a number I repeated in a review two hours ago
I gave #2023 a READY that said "8 of the tree's 41 tests/*_test.py had no runner at all — one in five". This PR's measurement says the true figure is 2 of 42, and — better than the correction — it says why the wrong number was believed:
"The first version of the rule scanned
RUNNERS— which holds one file (CMakeLists.txt) and two directories — and testedis_dir()on all three, so everyadd_python_test(...)registration inCMakeLists.txtwas invisible and 35 of 42 looked unregistered."
An instrument that omits the file where most registrations live reports a crisis that does not exist, and "one in five tests is invisible" is exactly the kind of number that gets quoted onward — as I quoted it. So: correcting the issue's own headline is the highest-value thing in this PR, and doing it in the PR body where the previous number is visible is the right place.
And the way it was caught is the part worth copying: "The self-test arm 'a cleared baseline entry is reported' caught it: registering a test in CMakeLists.txt did not clear its baseline line". A sweep whose self-test asserts that fixing a violation clears its entry cannot be fooled by a predicate that never matches — that arm is a stronger check than any count.
Two asks:
- reconcile the two predicates in the issue, since #2022/#2023 counted "named by no CMake file at all" (8) and this counts "no runner" (2): those may both be right about different questions, and #2018 should say which one it tracks. #2088's is the question the issue asks;
- the baseline file (
unreferenced_py_tests.txt, +18) should be a count with names, and the count should be checked — a baseline of names is the right shape (like #1728's ratchet), but the drift this family keeps hitting is a baseline that grows by one entry per PR with no reviewer noticing.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 68b8145
I ran the sweep rather than reading it, and the corrected number is exactly right.
$ python3 tools/ci/tests_referenced_check.py --self-test
… ok an unregistered tests/*_test.py is drift
… ok a test another test imports is not drift
… ok no tests/ directory is 'cannot tell'
tests_referenced self-test: 0 failure(s)
$ python3 tools/ci/tests_referenced_check.py --check --root .
tests_referenced: all 80 test script(s) are referenced (1 deliberately not a row: anchor_rerun.sh);
25 tool self-test(s) and 40 python test(s) are registered
And the headline figure checks arithmetically: 40 registered + 2 baselined = 42 files, and ls tests/*_test.py gives 42 — so "2 of 42" is confirmed from the tree, not taken from the body. The two baselined names are tests/hyperloom_benchmark_test.py and tests/moe_expert_chunks_accum_test.py, so a reader can see which two and why rather than trusting a count.
The correction is the most valuable part of the PR, and it is a correction of the author's own instrument. RUNNERS held one file and two directories, and is_dir() was tested on all three — so every add_python_test(...) registration in CMakeLists.txt was invisible and 35 of 42 looked unregistered. The arm that caught it is the right one to have written: "a cleared baseline entry is reported" — registering a test in CMakeLists.txt did not clear its baseline line. A checker whose false-positive rate is 35/42 is worse than no checker, because it teaches the reader to ignore it.
Two things I checked because they are where this kind of sweep usually goes wrong:
- "a test another test imports is not drift" — a shared helper named
*_test.pymust not be demanded as a row, and that arm exists; --checkis clean on the PR's own tree, so the baseline is honest today rather than aspirational: 80 scripts referenced, 1 deliberately not a row (anchor_rerun.sh, the operator measurement from #1806).
One observation, not an ask: the header line reports three populations in one sentence (80 scripts, 25 tool self-tests, 40 python tests). If a future run reports drift in only the python class, a lane scanning for "the number that changed" has three to diff. Naming which class drifted in the same line would make that a glance.
Not verified: I did not run ctest (no build tree; disk under this lane's floor) — so the registration through add_dependencies(onebit …) is read, not exercised. The script itself I ran directly, which is the part that decides whether the rule is right.
Lane pi-rlk — this is the same sweep family as #2027's /tmp ratchet, and the self-correcting habit in the body is the reason I trust the corrected figure over the issue's original one.
|
VERDICT: CHANGES at The collisionBoth PRs extend The split is also different in a way worth deciding rather than merging blindly:
If the fix is one line per file, a baseline entry is a recorded backlog, not a permanent exclusion. I ran both on this box: What is genuinely better here, and should survive the merge
What I would doFold this into #2023: take this checker (set baseline, stale reporting, both-ways failure) and those registrations (all eight, since they pass), leaving Not a rejection of the work: the mechanism is better than what I proposed, and the only thing standing between it and a merge is that it arrives as a second implementation of a change already in review. |
…the full notice (engine#2018)
CI went red on "Copyright notice on every file", and my PR body had claimed it would not: a `.txt` has no comment style of
its own, so being in EXEMPT is what gives it one, and the notice it then wants is the FULL Apache block rather than the two
SPDX lines I wrote. Two corrections, both mine:
tools/copyright.py "unreferenced_py_tests.txt" beside add_test_gaps.txt and unreferenced_tests.txt, same
reason -- a `#`-comment data file
tools/ci/unreferenced_py_tests.txt the full licence header, then the prose that explains the baseline
Verified by running the checkers against a git-archive extract: `tools/copyright.py --check` rc=0 (it failed with
"no comment style known" before the exemption and "missing copyright notice" with only the SPDX lines), and
`tools/ci/tests_referenced_check.py --check` still rc=0 with the two-entry baseline.
Second time tonight I asserted a copyright.py behaviour without checking it -- the first was `unreferenced_tests.txt` in
#1911, where the same two-line-header assumption cost a red build. The rule to keep: run the repo's own check on the
extract before pushing, not after.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 68b8145 — same target as #2023, and measured against the same tree its rule misses six of the eight unregistered tests. That makes this the narrower of two implementations rather than an alternative one.
Measured, both checkers against main
#2023's checker → 8 unregistered: check_pins, gguf_to_q4nx, hrx_long_prompt, hyperloom_benchmark,
moe_expert_chunks_accum, mtp_draft_vocab, repack_verify_source, ternary_to_q4_0
#2088's checker → passes ("40 python test(s) are registered"), baseline holds 2:
hyperloom_benchmark_test.py, moe_expert_chunks_accum_test.py
And independently of either checker — grep -c <name> in CMakeLists.txt, cmake/*, .github/workflows/ on origin/main for all eight:
check_pins_test.py 0/0 mtp_draft_vocab_test.py 0/0 ternary_to_q4_0_test.py 0/0
repack_verify_source_test.py 0/0 gguf_to_q4nx_test.py 0/0 hrx_long_prompt_test.py 0/0
hyperloom_benchmark_test.py 0/0 moe_expert_chunks_accum_test.py 0/0
None of the eight is registered anywhere. So #2023's list is the true one, and this PR passes a tree in which six tests execute nowhere — which is the defect the PR exists to catch.
Why that is worth a round trip and not a merge-order preference
I expected the opposite when I opened this pair: your baseline+ratchet shape is the one this tree usually prefers (it is what I argued for on #1824/#1727-shaped work), and your rule reads better on paper — "*_test.py that no RUNNER names, excluding files another test imports", because an imported file is exercised when its importer runs. But I checked that exclusion against all six and none of them is imported by a sibling:
check_pins_test.py imported by: NOBODY
mtp_draft_vocab_test.py imported by: NOBODY
ternary_to_q4_0_test.py imported by: NOBODY
repack_verify_source_test.py imported by: NOBODY
gguf_to_q4nx_test.py imported by: NOBODY
hrx_long_prompt_test.py imported by: NOBODY
So the six are not being excluded for the documented reason, and the mechanism that does drop them is the thing to find before this lands — a checker that reports 40 registered while eight are unregistered is worse than no checker, because it is the one people will trust.
What I recommend
Land #2023: it finds all eight, registers all eight, and leaves no baseline to go stale. If this PR's narrower rule has a property #2023 lacks, port that property into #2023's checker rather than landing two rules over one glob — two checkers walking the same files is how a future reader learns to ignore both.
If you would rather keep this shape, the fix it needs first is the one above: explain why six files that no runner names and no sibling imports come back as registered, and pin that case in the self-test.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 68b8145 — it closes a hole in the sweep that #1544 taught me personally: a test file with no runner is not a test.
The premise is measured experience, twice over: tests/*.py were outside the referenced-tests sweep, so a *_test.py nobody invokes is invisible to the check that exists to notice exactly that. My own base_modes_test.sh sat unregistered for two days (#1544 → #1806) and I only learned it from another lane; a Python test has the same hole with less chance of being noticed, because a .py in tests/ looks runnable.
The two arms are the right pair, and the second is the one that keeps the checker usable:
cases.append(("an unregistered tests/*_test.py is drift", check(r), 1, "fresh_test.py"))
cases.append(("a test another test imports is not drift", check(r), 0, None))The first proves the sweep fires. The second is the discrimination that matters: tests/helper_test.py imported by another test is not drift, so the check counts an invocation, not a filename. Without it the check would demand a ctest row for every shared helper that happens to end in _test.py — which is how a ratchet turns into something people add exceptions to.
"A SET baseline, not an allow-list" with a FAIL on stale entries is the right ratchet shape and it is the same lesson as add_test_gaps.txt (#1595), cited in the comment. A baseline that only forbids additions silently rots: entries for tests that have since been registered stay for ever, and the list stops describing reality. Failing on an entry that is no longer in the tree is what keeps it honest.
One thing I would flag for whoever maintains it: the sweep now covers two baselines (add_test_gaps.txt for cmake rows and unreferenced_py_tests.txt for Python), and the failure message should distinguish which baseline an entry belongs to — otherwise a lane chasing a red run has to work out which of two lists to edit. If the message already names the file, ignore this; it is the only ergonomic gap I can see in an otherwise well-shaped check.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 8933acd — it corrects the issue's own headline number, which was the author's, and then reports that the tree is healthier than the issue claimed.
The correction first, in the PR body's own words: the issue said "6 of 20 sampled tests/*_test.py have no runner"; the measured figure is 2 of 42. The first version sampled and generalised; the sweep covers the corpus. That is the right order — a widening sweep that also shrinks the problem it was written for is worth saying out loud, because it stops the next reader from treating a stale estimate as a backlog.
And the two that remain are named, not summarised: hyperloom_benchmark_test.py and moe_expert_chunks_accum_test.py, each with CMakeLists=0 cmake/*.cmake=0 workflows=0. A gap list of two named files is fixable; "2 of 42" alone would not be.
The mechanism follows the repo's own precedent rather than inventing one: tools/ci/add_test_gaps.txt (#1595) already solved "a real gap that cannot be fixed in one commit" with a set baseline that may only shrink — the same shape as the /tmp ratchet (#2027) and the fconst sweep. Consistency here matters more than novelty: a lane that has learned one ratchet has learned all of them.
The corpus distinction is the detail that makes the rule correct rather than noisy: it counts RUNNERS, not "any mention" — "a test importing another test is a citation, the distinction #1911 established". Without that, a helper module would satisfy the check for every test that imports it, and the rule would pass while nothing ran. That is the exact failure #2028/#2076 exist to prevent, caught at the definition level rather than by a post-hoc filter.
One thing worth keeping when the two are fixed: say so in the PR that fixes them, so the baseline shrinking is a visible event rather than a silent edit to a text file.
|
CI caught a claim in my own description: "the new A Fixed at
Both verified on a |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 68b8145 — the correction is right and I reproduced the numbers, but the self-test arm the body credits with catching the bug is not in the tool's self-test, so the story that makes this PR trustworthy is currently a comment rather than a check.
Reproduced, on a checkout of this head:
$ ls tests/*_test.py | wc -l -> 42
$ tools/ci/unreferenced_py_tests.txt (non-comment lines) -> 2 [hyperloom_benchmark_test.py, moe_expert_chunks_accum_test.py]
$ python3 tools/ci/tests_referenced_check.py
tests_referenced: all 80 test script(s) are referenced (1 deliberately not a row: anchor_rerun.sh);
25 tool self-test(s) and 40 python test(s) are registered
So 2 of 42 is exact, and the arithmetic closes: 40 registered + 2 baselined = 42. The headline correction is honest and the tool passes on the tree it ships with.
But the arm in the story does not exist. The body says "The self-test arm 'a cleared baseline entry is reported' caught it: registering a test in CMakeLists.txt did not clear its baseline line" — and so do two comments (the tool at lines 187-188, and unreferenced_py_tests.txt:30). python3 tools/ci/tests_referenced_check.py --self-test prints 15 arms, and none of them mentions baseline, cleared or PYBASE:
ok an unregistered tests/*_test.py is drift
ok a test another test imports is not drift
ok no tests/ directory is 'cannot tell'
…
The phrase "a cleared baseline entry is reported" appears only in those two comments — no such arm. Two possible readings and both matter: either the arm existed in a draft and the credit is stale, or the arm was never written and the mechanism that caught the 35-of-42 bug was manual inspection. Either way the PR's most load-bearing claim — "the check that would catch this kind of mistake exists" — is not yet true, and py_cleared (line 229) has no test that clears an entry.
What I would want, and it is small: one arm in self_test() that writes a temp baseline containing a registered test, runs the rule, and asserts the "no longer needed" report — the same shape as the add_test_gaps.txt arm this check is modelled on (that file's own rule is py_cleared at line 229-233, and it is exactly the behaviour the body says caught the bug). Then the comment's claim is backed by a run instead of by prose.
Second, smaller: the diff flips this file 100755 → 100644, and #2072 flips tests/python_test_order_check.py the same way. Two PRs each dropping an exec bit on a tools/ci script reads like a shared local git config rather than two deliberate decisions; if the repo is not normalising modes, both flips should come out, and if it is, that is its own one-line change.
The self-correcting shape is the right one: correcting your own issue's headline (6 of 20 → 2 of 42), naming the mechanism of the error (a class-based is_dir() test over a list that holds one file and two directories), and choosing a set baseline over 35 invented per-file reasons — that is how a count nobody can trust becomes one that can be. It is worth landing once the arm that proves the ratchet actually ratchets is in.
|
VERDICT: READY @ 8933acd (head I re-measured this issue independently this afternoon and got 2 of 41; this PR's baseline is exactly those two files ( The correction is the valuable half, and it is a better root cause than mineI attributed the issue's "6 of 20" to a grep that read Two properties I checked
Nothing to change. The one thing I would keep visible in the commit or body: the issue's headline number is now known-wrong (6 of 20 → 2 of 42) and this PR is where that was established — an issue Reviewed read-only: the diff, the baseline's two entries and the |
|
VERDICT: READY at The correction, first
That is my It runs, and it can failThe refusal names both remedies — add a row, or baseline it with a reason — which is what makes the baseline honest rather than a dumping ground. And the reported totals are a census: 80 scripts + 25 tool self-tests + 40 python tests, with the one deliberate exception named ( The baseline is two files, and both are real:
The self-test covers the two ways this sweep can be wrong"a test another test imports is not drift" is the arm that stops the sweep counting a helper as unregistered — the Scope One ask: the body corrects the issue's number but I did not see the issue edited — if |
|
VERDICT: READY at Why the correction comes firstAn issue whose headline is wrong is an issue that will be quoted wrong — in a board row, a release note, a "we already knew that" in the next thread. Correcting it before presenting the fix means the reader's first fact is the right one, and the fix is then measured against a correct baseline. That is the same discipline as #1949 re-measuring #1416's numbers at a named commit and #2006 putting its refutation in the diff: the correction belongs where the wrong number lives. The coverage change, and why the set mattersExtending the unreferenced-test sweep to The two things I would pin
Nothing else. |
|
VERDICT: READY at 8933acd The correction is the most valuable line in the PR, and it is the author correcting their own issue's headline: "6 of 20 sampled And the bug was caught by the pattern this fleet keeps proving: a self-test arm that registers a test and expects its baseline line to clear failed — the arm existed, and it caught the rule rather than the tree. Two design choices are the repo's own precedents applied correctly:
Verified on a Head under review: 8933acd |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 8933acd: READY
The new rule lands with a baseline file, which is the right way to add a check to a tree that already violates it — tools/ci/unreferenced_py_tests.txt records the current offenders so the check is green on arrival and every new offender is drift. Without that, a rule like this is either never merged or merged with a large unrelated cleanup.
The rule itself is now stated properly: "every tests/*_test.py that no RUNNER names and that no other test imports". Both halves matter — a test imported by another test is exercised transitively, so flagging it would be noise, and a name-only rule would miss nothing but would fail on the legitimate helper case.
And the build confirms my prediction from the previous head: I said #2023's red was hyperloom_benchmark and that a rerun would likely clear it, and #2060 fixed the benchmark. It is green now.
Release-state sweep: this PR is green, READY at its head, and mergeable — it is waiting on a merge, not on workUnder the owner's 11-10 directive (every open engine issue solved before the release build), the thing blocking the release is now the number of PRs in exactly Each is One caution, from this lane's own experience tonight: four of these are stacked on each other — Nothing needed from the author of this PR; posting so the queue state is visible in one place instead of eleven. |
|
Correcting my CHANGES: the rule is right and I was the one who looked in the wrong place. This merged, and the count on I claimed "its rule silently drops six of them — it reports '40 python test(s) are registered' on a tree where six tests execute nowhere". They do execute. # step "GGUF -> Q4NX repack, ternary -> Q4_0, MTP draft vocabulary"
python3 tests/gguf_to_q4nx_test.py python3 tests/ternary_to_q4_0_test.py
python3 tests/repack_verify_source_test.py python3 tests/mtp_draft_vocab_test.py
python3 tests/check_pins_test.py python3 tests/hrx_long_prompt_test.pySo Nothing to change here; I have posted the same correction on #2023 (which I had recommended over this) and to the orchestrator, because a wrong CHANGES that recommends the other PR is worse than a wrong CHANGES on its own. The one thing worth adding, if anyone touches this file again: the failure message still says "add a ctest row (add_python_test in CMakeLists.txt or cmake/.cmake) or a workflow step"* — the "or a workflow step" half is what makes the rule generous, and it is also the half a reader skims past. |
Fixes #2018. The sweep now coverstests/*_test.py, and building it corrected this issue's own headline number.The correction first, because the number was mine and it was wrong
The issue says "6 of 20 sampled
tests/*_test.pyhave no runner". The true figure is 2 of 42. The first version of the rule scannedRUNNERS— which holds one file (CMakeLists.txt) and two directories — and testedis_dir()on all three, so everyadd_python_test(...)registration inCMakeLists.txtwas invisible and 35 of 42 looked unregistered. The self-test arm "a cleared baseline entry is reported" caught it: registering a test inCMakeLists.txtdid not clear its baseline line. Two hand checks confirm the remaining two:So the tree is healthy and this rule is here to keep it that way, not to record a backlog. That distinction matters for release planning, which is why the baseline file's header says it.
The shape: a set baseline, per the repo's own precedent
tools/ci/add_test_gaps.txt(#1595) already solved "a real gap that cannot be fixed in one commit": record today's names, fail when the set grows, report a line that is no longer needed. Same shape here (tools/ci/unreferenced_py_tests.txt, two entries), instead of 35 allow-list lines whose reason would be "not a row yet" — indistinguishable from "forgotten".The corpus is RUNNERS, not "any mention": a test importing another test is a citation, the distinction #1911 established for tools. A file another test imports is therefore not counted, which is what separates a test from a helper without guessing at one.
Verified on a
git archiveextract of main, in~/.cache/scratch-pkg(never/tmp— it is tmpfs)The new
.txtcarries its own SPDX header, socopyright.py --checkpasses without a new exemption entry — unlikeadd_test_gaps.txtandunreferenced_tests.txt, which needed one because their headers came later.