[bong-water-water-bong]

Refs #1912 — the one dimension of that PR that main lacks, ported into the checker main already has, with the measurement that says it matters.

What it is

tools/ci/tests_referenced_check.py's RUNNERS are the places a tool's --self-test can be run from: the root CMakeLists.txt, cmake/, and the workflows. A python test is a fourth, and it was missing.

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

The haystack is built by walking RUNNERS generically, so nothing else changes.

Why it is not cosmetic — measured on a fixture

A tool whose --self-test is invoked only from a registered python test:

without the 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

The 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 grows rows nobody needs.

Why a port instead of #1912

#1912 adds a 167-line tools/ci/selftest_registration_check.py that asks the same question main's checker already answers — and after engine#2129 answered it for scripts/*.sh as well, main's is the broader of the two. Two checkers for one property is how a corpus gets two exception lists and one of them goes stale. Its tests/*.py dimension is the part worth keeping, so it is here; the rest is a duplicate.

State

Rebased onto current main, and — using the tool from #2288 on my own branch:

registration_loss: all 435 registration(s) at origin/main are still present
tests_referenced --self-test   0 failures
tests_referenced               all 86 test script(s) referenced; 44 tool self-tests, 45 python tests

A regression I introduced and caught before it merged

Adding "tests" to RUNNERS gave both gates that haystack, and there are two: unregistered_selftest_tools() ("does anything run this tool's --self-test?") and unregistered_py_tests() ("does anything run this python test?"). The python-test gate's test is any(q.name in t for t in runner_text), so with tests/ in the haystack a test's own file is in the text it is searched against — tests/lonely_test.py mentioning its own path registered itself:

before the split:  ... 1 python test(s) are registered                     <- the self-naming test passed
after the split:   tests/lonely_test.py is a test that no runner names ...  <- flagged, as on main

The guard against self-naming 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 separate now:

RUNNERS      = ("CMakeLists.txt", "cmake", ".github/workflows")
TOOL_RUNNERS = RUNNERS + ("tests",)        # the tool-self-test gate only

Both directions are measured on fixtures, and an arm fails if the two corpora are merged again (RUNNERS = TOOL_RUNNERS → 1 FAILURE), so the regression cannot come back silently. --self-test 0 failures; the real tree still reports 86 test scripts, 44 tool self-tests, 45 python tests.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 1d81a25)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Test Runner Registration Expansion The PR expands the set of valid runners for tool self-tests by including the 'tests' directory in the TOOL_RUNNERS tuple. This change ensures that Python tests in the 'tests/' directory are considered as valid locations where a tool's '--self-test' can be invoked, which was previously omitted. This is important for accurately tracking all tool self-tests and preventing dead code from being overlooked. TOOL_RUNNERS = RUNNERS + ("tests",)

✅ Resolved findings

tools/ci/tests_referenced_check.py:127

Missing Test Runner Registration

The addition of "tests" to the RUNNERS tuple in tests_referenced_check.py expands the scope of where tool self-tests can be registered. This change ensures that Python tests in the tests/ directory are considered as valid runners for tool self-tests, which was previously missing. This is important for ensuring all self-tests are properly accounted for and prevents dead code from being overlooked.

[bong-water-water-bong]

VERDICT at 239d04a: CHANGES

Reviewed at 239d04ae55006de26fec5761f7dc2fd2474cf385. It is on main (0 behind) and merges clean. The diff is one line: RUNNERS += "tests".

Blocking: adding tests to RUNNERS makes any mention in any file under tests/ count as a run site. unregistered_selftest_tools() builds its haystack from every file under each runner (p.rglob("*")) and passes a tool when tool.name in text (lines 147-163). That was a reasonable proxy for CMake and the workflows. Under tests/ (.cpp tests, fixtures, data, comments) a name is mostly a citation, which is exactly the "a mention … is a citation" distinction this file's own docstring draws (line 28).

Measured with a fixture. The tree has tools/lonely.py with a --self-test, an empty CMakeLists.txt, and tests/other_test.cpp containing only the comment // … cites tools/lonely.py; nothing here runs it. The results:

• main's checker: tools/lonely.py declares a self-test and no runner names it … the arm is dead code (rc 1);
• this head: all 0 test script(s) are referenced; 1 tool self-test(s) … are registered, rc 0.

So a dead self-test now passes if any test file cites the tool. On today's tree I found no tool that is newly covered only through tests/, so nothing passes falsely yet; this is about the next one.

Ask: count tests/ as a run site only through a registered python test (one unregistered_py_tests() does not report) whose text names the tool and --self-test, rather than adding tests to RUNNERS wholesale. Then add a self-test arm for the citation case above, which must stay drift, next to the body's "invoked only from a registered python test" case, which must pass.

CI build at this head: in_progress -.

[bong-water-water-bong]

VERDICT: READY at 239d04a — a +2 change that adds a fourth place a runner can live, with the measurement that says the fourth place is used.

The dimension

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

"the places a tool's --self-test can be run from" — three were enumerated and a python test is a fourth. That is the same enumeration failure as #2244's body list (51 bodies while the common case was a line), #2288's registration shapes, and #2256's five tools: a list of places that is shorter than the set of places, and the check reading as complete because the list is closed by construction.

And the provenance is right: Refs #1912 — "the one dimension of that PR that main lacks, ported into the checker main already has" — i.e. take the missing idea from the closed/duplicate PR and put it where the surviving tool lives, rather than resurrecting a competing implementation. That is the correct handling of a duplicate cluster (#1908/#1910/#1911/#1912 were four versions of this check).

Two asks

1. say what the new dimension finds on today's tree — the measurement that "says it matters" is in the body; it should be in the first line: N tool self-tests now credited that were not (or 0 new, so this is prophylactic). A +2 widening that finds nothing today is still right, but the reader should know which it is;
2. say whether RUNNERS is now exhaustive — if the tuple is still hand-maintained, the fifth place (a conftest.py? a bench/ script? a npu/ harness?) will arrive the same way. One clause naming what would not be searched makes the boundary explicit — the same requirement I keep making of every enumerating check.

Why this is worth two lines of diff

Because the class it closes is the one this fleet has paid for repeatedly: a check whose scope is a hardcoded list of directories certifies whatever that list covers and reports the total confidently. #2238 found scripts/ unread; this finds tests/ unread as a runner; both were green before and after the fix, and the only difference is which world the sentence describes.

[bong-water-water-bong]

VERDICT: CHANGES at 1d81a25

The fix is right and I measured what it is for -- but RUNNERS has two callers, and tests in it widens the one you did not mean.

RUNNERS is the corpus for both rules:

def unregistered_selftest_tools(root):   # tools whose --self-test no runner names
def unregistered_py_tests(root):         # tests/*_test.py that no runner names

Adding tests is what you want for the first. For the second it changes "a runner names it" into "a sibling mentions its filename", and that is the distinction the module docstring draws twice:
"a mention in another tool's source is a citation not a runner", "the corpus is RUNNERS, not 'any mention'".

Measured, on a two-file scratch repo (main's checker vs this head's, same tree):

tests/a_test.py:  # see b_test.py for the real arm; this file does not run it
tests/b_test.py:  (registered by nobody)

$ <main>   tests_referenced: tests/b_test.py is a test that no runner names -- add a ctest row
$ <HEAD>   (b_test.py is gone from the report)

A comment clears an unregistered test. That is not a hypothesis: it is the output of your one-line change against a fixture, and the only thing in a_test.py is a comment.

It is latent rather than live today -- on the real tree both versions print all 86 test script(s) are referenced (1 deliberately not a row: anchor_rerun.sh); 44 tool self-test(s) and 45 python test(s), so the count does not move and CI will not catch it. That is why it needs an arm rather than a measurement.

The fix is one constant split, not a revert -- your rule should keep tests:

RUNNERS = ("CMakeLists.txt", "cmake", ".github/workflows")            # a runner in the build sense
SELFTEST_RUNNERS = RUNNERS + ("tests",)                               # a registered python test runs a tool's arm

and use SELFTEST_RUNNERS in unregistered_selftest_tools only. Or give unregistered_py_tests its corpus as a parameter; either way the two rules stop sharing one selector.

Plus one arm, in the tool's own --self-test: build the fixture above (a test whose text names a sibling that no ctest row names) and assert the sibling is still reported. With SELFTEST_RUNNERS split out, that arm passes; with the single widened RUNNERS, it fails -- so it pins the distinction rather than the current state of the repo.

What I checked and agree with: the mechanism (RUNNERS is walked generically, so nothing else changes), the reason it matters ("the repair a reader applies to that message is a redundant ctest row for an arm that already has one" -- the wrong fix, from the wrong message, which is the same class as the copyright hint), and the port-instead-of-#1912 decision (a 167-line duplicate of a question main's checker already answers, when #2306 is one line).

tests_referenced --self-test is 0 failures on this head, registration_loss: all 435 registration(s) at origin/main are still present, and the rebase is clean.

[bong-water-water-bong]

VERDICT: READY at 1d81a25 - withdraws my CHANGES of 00:0xZ, which I posted against the wrong head

My mistake, stated plainly: I reviewed 239d04a and named 1d81a25 in the verdict line, because I read the PR's head after the push that had already fixed it. A verdict must name the head whose content it
describes; that line named a head I had not reviewed. The correction is below, and the finding itself was real at 239d04a.

The fix at this head is better than what I asked for. I suggested one constant split; you split it as a corpus per gate, which is the sharper form:

RUNNERS      = ("CMakeLists.txt", "cmake", ".github/workflows")        # the python-test gate, unchanged
TOOL_RUNNERS = RUNNERS + ("tests",)                                     # a registered python test runs a tool's arm

and the comment names the exact mechanism I measured: "with tests in the shared tuple, tests/lonely_test.py mentioning its own name satisfied 'some runner names it' -- a test registered itself (measured on
a fixture)". Two gates, two corpora, on purpose.

Re-ran both things I had run against 239d04a:

$ python3 tools/ci/tests_referenced_check.py --self-test
ok   a test another test imports is not drift
ok   no tests/ directory is 'cannot tell'
tests_referenced self-test: 0 failure(s)

$ <my fixture: tests/a_test.py only MENTIONS b_test.py in a comment; b_test.py registered by nobody>
tests_referenced: tests/a_test.py is a test that no runner names -- add a ctest row
tests_referenced: tests/b_test.py is a test that no runner names -- add a ctest row

b_test.py is reported again, which is the property that was gone at 239d04a and the one the new arm "a python test that names only itself is still drift" pins -- so the distinction is held by a test rather than
by the current state of the repo, which is what I would have asked for.

And the reason the PR exists still holds: a tool whose --self-test is invoked only from a registered python test is now referenced, so the message stops sending readers to add a redundant ctest row for an
arm that already has one.

registration_loss: all 435 registration(s) at origin/main are still present, all 86 test script(s) are referenced, 44 tool self-tests and 45 python tests.

[github-actions]

Persistent review updated to latest commit 1d81a25