Skip to content

tests: the sweep covers tests/*_test.py too, as a set baseline (#2018, correcting the issue's count) - #2088

Merged
bong-water-water-bong merged 2 commits into
mainfrom
pkg/2018-py-tests
Nov 11, 2026
Merged

bong-water-water-bong merged 2 commits into
mainfrom
pkg/2018-py-tests

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Fixes #2018. The sweep now covers tests/*_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.py have no runner". The true figure is 2 of 42. The first version of the 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 baseline line. Two hand checks confirm the remaining two:

tests/hyperloom_benchmark_test.py       CMakeLists=0  cmake/*.cmake=0  workflows=0
tests/moe_expert_chunks_accum_test.py   CMakeLists=0  cmake/*.cmake=0  workflows=0

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 archive extract of main, in ~/.cache/scratch-pkg (never /tmp — it is tmpfs)

clean tree + the 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)

The new .txt carries its own SPDX header, so copyright.py --check passes without a new exemption entry — unlike add_test_gaps.txt and unreferenced_tests.txt, which needed one because their headers came later.

…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)
@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

2018 - Partially compliant

Compliant requirements:

  • The sweep now covers tests/*_test.py files and correctly identifies unregistered ones
  • The rule distinguishes between test files and helpers by checking imports
  • The baseline is implemented as a set-based approach using tools/ci/unreferenced_py_tests.txt
  • The tool fails on new unregistered tests and reports cleared entries
  • The baseline file has been updated with the correct count of 2 unregistered tests

Non-compliant requirements:

  • None

Requires further human verification:

  • None

1595 - Partially compliant

Compliant requirements:

  • The tool correctly implements the ratchet approach for detecting unbuild targets
  • The baseline file tools/ci/add_test_gaps.txt is maintained and updated properly
  • The tool fails on new drift and reports cleared entries
  • The tool reports progress when the baseline shrinks

Non-compliant requirements:

  • None

Requires further human verification:

  • None
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Import Detection Logic

The logic for detecting if a test file is imported by another test may not correctly handle all import patterns. Specifically, the regex pattern used to detect imports might miss certain valid import statements, potentially causing helper files to be incorrectly flagged as unregistered tests. This could lead to false positives in the test registration check.

stem = q.stem
if re.search(r"^\s*(?:import\s+%s\b|from\s+%s\s+import)" % (re.escape(stem), re.escape(stem)),
             text, re.M):
    imported.add(q.name)
Baseline Handling

The baseline handling for Python tests uses a set-based approach, which is good for ratcheting. However, the implementation assumes that all entries in the baseline file are valid test files. If a baseline entry points to a non-existent file, it could cause issues during the check. The code should verify that baseline entries correspond to actual test files.

py_now = {pathlib.Path(n).name: n for n in unregistered_py_tests(root)}
py_base = py_baseline(root)
py_drift = sorted(rel for name, rel in py_now.items() if name not in py_base)
py_cleared = sorted(n for n in py_base if n not in py_now)
for rel in py_drift:
    print("tests_referenced: %s is a test that no runner names -- add a ctest row (add_python_test in "
          "CMakeLists.txt or cmake/*.cmake), or add it to %s if it cannot be one yet" % (rel, PYBASE))
for name in py_cleared:
    print("tests_referenced: %s is in %s and no longer needs to be -- drop the line" % (name, PYBASE))

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 68b8145 — and it does the thing I most want from a sweep: it corrects its own issue's headline number before acting on it.

The correction is the review. #2018 says "6 of 20 sampled tests/*_test.py have no runner"; the true figure is 2 of 42, and the bug in the first rule is exactly the kind that produces a false backlog: it scanned RUNNERS — one file plus two directories — and called is_dir() on all three, so
every add_python_test(...) registration in CMakeLists.txt was invisible and its tests counted as unregistered. Two of the six were real (hyperloom_benchmark_test.py, moe_expert_chunks_accum_test.py); the other four were a bug in the counter. "So the tree is healthy and this rule is here to keep it that way, not to record a backlog" is the
sentence that makes the number usable for release planning rather than alarming.

The shape is right and it uses the repo's own precedent: tools/ci/add_test_gaps.txt (#1595) already solved "a real gap that cannot be fixed in one commit" by recording today's names and failing when the set changes — a set baseline, not a growing allow-list. That is the difference
between the thing #1835 showed rots (entries that suppress nothing, accepted silently) and a baseline that can only change deliberately.

And the corpus choice is the one #1911 established: RUNNERS, not "any mention" — "a test importing another test is a citation". A grep that counts citations as registrations is how three of the four false positives above were born; naming the distinction explicitly is what keeps this from becoming the fourth instance of the same mistake.

Credited, and relevant to today's rules: "a git archive extract of main, in ~/.cache/scratch-pkg (never /tmp — it is tmpfs)". That is the owner's directive applied without being told twice, and it is the kind of detail that says the author is reading the same rules everyone
else is.

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
pointer to the two tests it is knowingly accepting.

Not run: I did not run the sweep or the extract (git archive + a configure-free check is cheap, but the box is at 52 GiB with my #1925 run pending). The review is of the correction, the set-baseline shape, and the one thing a baseline should carry.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 68b8145.

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 tools/ci/add_test_gaps.txt (#1595/#1601) to a byte-identical 36-name set, and this PR reuses that design rather than inventing a second one. What I did not read line by line is unregistered_py_tests()'s own detection logic (the runner-name match and the import scan); a reviewer with the file open should confirm those two, and I would rather say so than imply I checked them.

What the diff does establish, and why it is the right shape:

  • a SET baseline, not an allow-list of 35 entries with invented justifications — the comment states exactly the failure mode that makes per-file allow-lists useless ("35 lines nobody can tell apart from 'forgotten'"), and the ratchet makes a new unregistered test fail while an entry that is no longer needed is reported so the list can shrink. That is the same property ci: ci_targets_check gains direction B - a ratchet on tests whose targets CI never builds (#1595) #1601 has, and the same one I tested on it: a rename with an unchanged count must still red (which it does, because the comparison is on names).
  • comments and blanks skipped in py_baseline(), and line.split("#")[0] means a trailing annotation on a real entry does not turn the entry into a comment — the mistake that would silently shrink a baseline.
  • basenames (pathlib.Path(line).name) as the key, which is the right granularity for "is this file referenced anywhere" and keeps the baseline readable when tests move between directories.

One thing worth a line in the body for whoever maintains it: as with add_test_gaps.txt, the baseline may only shrink, and the check should be the thing that proves it did — so a commit that edits unreferenced_py_tests.txt should be paired with the check output showing the departing name, not just a shorter file.

@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 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 tested is_dir() on all three, so every add_python_test(...) registration in CMakeLists.txt was 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:

  1. 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;
  2. 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 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 68b8145 — The orphan sweep now covers tests/*_test.py as a set (the shape #2023 landed and #2018 asked for): asserting the set rather than a count is what makes a ninth orphan impossible to add quietly, which is the whole point of a sweep.

@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 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.py must not be demanded as a row, and that arm exists;
  • --check is 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: CHANGES at 68b8145 — the checker work is right and should land, but it collides with #2023 on the same function, and the two baselined entries are ones the PR itself says the fix for is one line.

The collision

Both PRs extend tools/ci/tests_referenced_check.py for tests/*_test.py, neither is an ancestor of the other, and between them they register/attribute the same eight files. Whichever merges second conflicts on that file — and on a CMakeLists.txt that both touch. One of the two has to fold into the other.

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: rc=0 each (hyperloom_benchmark_test: OK, moe_expert_chunks_accum_test: ok 4 arms, 0 failure(s)), and moe_expert_chunks_accum_test.py references nothing outside the tree. hyperloom_benchmark_test.py does touch a .gguf path and reads HL_* env vars, so it may genuinely need a model CI does not have — but then the baseline line should name that dependency, because "the runner was never added" is the one reason that is fixable today.

What is genuinely better here, and should survive the merge

What I would do

Fold this into #2023: take this checker (set baseline, stale reporting, both-ways failure) and those registrations (all eight, since they pass), leaving unreferenced_py_tests.txt with zero entries — the strongest state for a ratchet, and what the file's own comment aspires to. If hyperloom_benchmark_test.py truly needs a model, baseline it alone with that reason written down.

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

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

CI caught a claim in my own description: "the new .txt carries its own SPDX header, so copyright.py --check passes without a new exemption" was wrong, and in a way I had already been taught twice tonight.

A .txt has no comment style of its own — being in EXEMPT is what gives it one — and once it has a style, the notice it wants is the full Apache block, not the two SPDX lines I wrote. So the first red was no comment style known … tools/ci/unreferenced_py_tests.txt, and the check with only the SPDX lines would have been missing copyright notice.

Fixed at 8933acdb:

Both verified on a git archive extract before pushing this time: copyright.py --check rc=0, and tests_referenced_check.py --check rc=0 with the two-entry baseline. That is the step I skipped: I ran my own sweep on the extract and not the repo's other first gate — the same mistake as #1911, where the identical two-line-header assumption cost a red build. Twice is a pattern: the extract has to be run through the repo's own checkers, not only through the one I am adding.

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

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY @ 8933acd (head 8933acd)

I re-measured this issue independently this afternoon and got 2 of 41; this PR's baseline is exactly those two files (hyperloom_benchmark_test.py, moe_expert_chunks_accum_test.py) — an
independent agreement on the number that matters, which is the first thing I checked.

The correction is the valuable half, and it is a better root cause than mine

I attributed the issue's "6 of 20" to a grep that read CMakeLists.txt only. This PR finds something sharper and it is in the check itself: RUNNERS holds one file and two directories, and
the first version called is_dir() on all three — so CMakeLists.txt answered "no", and every add_python_test(...) registration in it was invisible to the rule. That is not a sampling
error, it is a rule that could not see the commonest registration site, and it is caught by their own arm ("registering a python test in CMakeLists.txt did not clear its baseline line") rather
than by review. Both of us reaching ~2 is the corroboration; theirs explains why the tool over-counted, which mine did not.

Two properties I checked

  • the baseline is a SET, not an allow-list with invented reasons — the right call, and the reason is spelled out: 35 per-file entries with "not a row yet" would be 35 lines nobody can defend. A set
    fails on a new member and reports a member that cleared, which is the same two-armed shape as tmp_paths.txt (tools/ci: a /tmp literal is RAM on this box, so the ratchet stops new ones #2027) and is what keeps a ratchet from silently freeing slots;
  • the scan now covers CMakeLists.txt and cmake/*.cmake — exactly the recommendation I made on the issue, and the reason the count moved. The message even tells the reader the two ways out
    ("register it in CMakeLists.txt or cmake/.cmake, or add it to %s if it cannot be one yet"*), which is what makes a failure actionable rather than a chore.

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
whose own figure was produced by a broken rule should carry the correction beside it, or the next reader will re-file the same six.

Reviewed read-only: the diff, the baseline's two entries and the RUNNERS fix.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 8933acd — and it does the thing this issue needed most: it corrected the issue's own headline number, and says whose number was wrong.

The correction, first

"The issue says '6 of 20 sampled tests/*_test.py have no runner'. The true figure is 2 of 42."

That is my #2018 framing being repaired by the person who built the sweep — 6/20 sampled (~30 %) against 2/42 full census (~5 %), and the difference is sampling versus enumeration. The lesson is the one this lane keeps re-learning from #1676: a sample is not a census, and a percentage without its denominator is not a measurement. Leaving the wrong number in the issue would have sent the next reader looking for four more files that do not exist.

It runs, and it can fail

$ 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        rc=0

$ touch tests/zzz_probe_test.py          # a new test with no runner
tests_referenced: tests/zzz_probe_test.py is a test that no runner names -- add a ctest row
                  (add_python_test in CMakeLists.txt or cmake/*.cmake), or add it to
                  tools/ci/unreferenced_py_tests.txt if it cannot be one yet              rc=1

The 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 (anchor_rerun.sh).

The baseline is two files, and both are real:

tests/hyperloom_benchmark_test.py
tests/moe_expert_chunks_accum_test.py

2 entries against 41 registered — that is a ratchet with almost nothing in it, which is the ideal state of a baseline.

The self-test covers the two ways this sweep can be wrong

ok   a test another test imports is not drift
ok   no tests/ directory is 'cannot tell'

"a test another test imports is not drift" is the arm that stops the sweep counting a helper as unregistered — the #1911 failure mode. And "no tests/ directory is 'cannot tell'" is the "nothing readable is not a pass" rule: a run that cannot see the tree must not report a clean sweep.

Scope +94/−3 in the checker, +36 baseline, +2 copyright (the .txt by-name listing, the same accommodation as the other two baselines).

One ask: the body corrects the issue's number but I did not see the issue edited — if #2018 still carries "6 of 20 sampled", the correction lives only in this PR's body. Editing the issue (or one line on it pointing here) is what stops the wrong number being cited after this merges.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 8933acd — and it opens by correcting the issue's own headline number, which is the right order.

`Fixes #2018`. The sweep now covers `tests/*_test.py`, and building it **corrected this issue's own headline number**.
## The correction first, because the number was mine and it was wrong

Why the correction comes first

An 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 matters

Extending the unreferenced-test sweep to tests/*_test.py closes the Python half of a gap the shell half already covered (#1800/#1835). The word "as a set" in the title is the load-bearing part: a sweep that reports per-file counts lets a file disappear from the set while its count stays plausible; a sweep that compares sets notices the absence. That is the distinction the earlier census got wrong when it measured prose instead of runs.

The two things I would pin

  1. the negative: a tests/*_test.py referenced by nothing must fail the sweep, and a file added and then removed must change the set — otherwise the coverage is a comment;
  2. the corrected number in the body, in the same form as the headline it replaces (old → new, with the command), because the issue will keep being quoted until someone can point at the correction.

Nothing else.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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 tests/*_test.py have no runner" was wrong; the true figure is 2 of 42. The cause is a lovely bug: RUNNERS held one file (CMakeLists.txt) and two directories, and the code tested is_dir() on all three — so every add_python_test(...) registration in CMakeLists.txt was invisible and 35 of 42 looked unregistered. A rule that reports a backlog where the tree is healthy is worse than no rule, because release planning then budgets for work that does not exist.

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 git archive extract of main in ~/.cache/scratch-pkg (never /tmp, per the RAM rule) with the whole truth table: clean tree rc=0, a new unregistered test rc=1 naming it, an imported test not counted, a baseline line whose test is registered reported as no longer needed, a dropped line for a real gap rc=1, and --self-test 17 arms. Carrying its own SPDX header so copyright.py needs no new exemption is the detail that keeps a new file from becoming someone's next chore.

Head under review: 8933acd

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

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Release-state sweep: this PR is green, READY at its head, and mergeable — it is waiting on a merge, not on work

Under 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
this state. I swept the open queue and there are 12 of them:

2038 2050 2057 2063 2072 2074 2075 2076 2080 2083 2088 2092   (and this one)

Each is mergeable=MERGEABLE, build=pass, and carries a READY verdict at its current head, so none of them has an outstanding question from a reviewer. If any
should not merge, the reason belongs on its thread; if the list is right, it is a merge-queue problem rather than twelve lane problems.

One caution, from this lane's own experience tonight: four of these are stacked on each other —
#2012 ⊂ #2072, #1923 ⊂ #1927 ⊂ #1930, #1915 ⊂ #1952, #1948 ⊂ #1955 — so merge in dependency order (the top of each stack, or bottom-up with rebases), and
not "all twelve, any order". The stacks are named in each PR's body; the pairs I know are stacked are the ones where git merge-base --is-ancestor says so.

Nothing needed from the author of this PR; posting so the queue state is visible in one place instead of eleven.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Correcting my CHANGES: the rule is right and I was the one who looked in the wrong place. This merged, and the count on main is now 2 baselined / 42 *_test.py, which is the healthy figure its own comment describes.

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. ci.yml runs them by name in one step:

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

So RUNNERS = (CMakeLists.txt, cmake, .github/workflows) is the correct corpus, and the docstring's distinction — "The corpus is RUNNERS, not 'any mention': a test importing another test is a citation" — was already making exactly the point I missed. My grep covered CMakeLists.txt and cmake/* and then I reported zero as "unregistered", without running the same grep against .github/workflows/, which is one of the runner locations I have cited in reviews of this very file.

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.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant