Skip to content

ci: the unreferenced-tests allow-list enforces both directions (engine#1820) - #1835

Merged
bong-water-water-bong merged 1 commit into
mainfrom
os/v3-unreferenced-rot
Nov 10, 2026
Merged

bong-water-water-bong merged 1 commit into
mainfrom
os/v3-unreferenced-rot

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Refs #1820. The row that merged in #1824 is good, and its second direction does not bite — measured on main 318b6ef0:

allow entry for a test that is gone        ghost_test.sh        -> prints "no such test exists any more", rc 0   (passes)
allow entry that suppresses nothing        base_modes_test.sh   -> no message at all, rc 0, counted as "deliberately not a row"
allow entry in path form                   tests/anchor_rerun.sh -> "referenced by nothing" AND "no such test exists any more", rc 1

Three consequences:

  1. The list can rot, silently. An entry for a test that is now registered is accepted and even reported in the success line as "deliberately not a row". That is the exact failure the file's own docstring says reporting prevents ("keeps it honest rather than accumulating") — reporting does not prevent it, failing does. The reviewer's table credited tests: a sweep for tests/*.sh that nothing references (#1820), and it registers the one it found #1824 with rot-protection; the "suppresses nothing" half is not there.
  2. The warning that does exist cannot fail the row. stale is computed, printed, and never reaches the exit code (if drift: return 1), so a deleted-test entry announces itself and passes for ever.
  3. A path-form entry is misdiagnosed. allow_list() keys on the line as written while need is basenames, so tests/foo.sh matches nothing: it suppresses nothing and is reported as "no such test exists any more", which is false. The issue's own hand-loop printed exactly this form (echo " $f" with $f = tests/…), and tools/ci/ otherwise writes repo-relative paths.

Fix (23 lines, one file)

  • allow_list() normalises keys to basenames — tests/foo.sh and foo.sh are the same entry.
  • stale splits into gone (no such test → drop the line) and idle (its test IS referenced → the line suppresses nothing), each with its own message, and both fail the row.
  • The self-test gains three arms: an entry for a deleted test fails, an entry that suppresses nothing fails, a path-form entry is accepted. Its old arm 7 asserted "a stale allow entry is progress, not drift" with rc 0 — the defect written down as intent, now inverted.

Red and green, measured on this head

green, merged main's tree          ok  all 73 test script(s) referenced (1 deliberately not a row)   rc 0
entry for a gone test              ghost_test.sh is in … but no such test exists any more             rc 1   (was 0)
entry that suppresses nothing      base_modes_test.sh is in … but it IS referenced now               rc 1   (was silent)
path-form entry                    ok  all 73 …                                                       rc 0   (was 1 + a false message)
a dangling test still fails        dangling_probe.sh is referenced by nothing that can run it        rc 1
--self-test                        0 failure(s)                                                       rc 0

Through the registered rows (reconfigured, under thermal-run): tests_referenced 0.23 s, tests_referenced_self 0.09 s, 2/2 passed.

Nothing else changes: same rule, same allow-list file, no new script, and anchor_rerun.sh remains the one legitimate entry.

…ns (engine#1820)

Measured on merged main: the row's second direction did not bite.

- an allow entry for a test that is gone printed its warning and the row
  still exited 0;
- an allow entry for a test that IS referenced was accepted in silence and
  counted as 'deliberately not a row', so the list could rot exactly as the
  file's own docstring says it must not;
- a path-form entry (tests/foo.sh, the form the issue's hand-loop printed and
  the form tools/ci/ otherwise uses) matched no test, so it suppressed nothing
  and was then reported as 'no such test exists any more' -- two contradicting
  messages about one line, the format never named.

Now: gone and idle entries both fail with their own message, entries are
normalised to basenames so either form works, and the self-test gains an arm
for each (plus the path form). Its old arm 7 asserted 'reported, but not a
failure', which is the defect written down as intent.

Green on main's tree: all 73 test scripts referenced (1 deliberately not a row).

@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 3aab4e4 — because it re-measures the guard my own reviews asked for and shows its second direction does not bite, which is the most useful thing a follow-up to a merged
guard can do.

The three measured cases are the whole argument, and they are all the same failure:

case behaviour on main 318b6ef0
allow entry for a test that is gone prints "no such test exists any more", rc 0 — accepted
allow entry that suppresses nothing no message at all, rc 0, counted as "deliberately not a row"
allow entry in path form (tests/anchor_rerun.sh) both messages, rc 1

So the list can only grow and never shrink, and an entry that stops being needed is indistinguishable from one that is doing its job. That is precisely the class I have been flagging all night in
other forms — an add_test that is absent rather than skipped (#1741), a ldd pre-check that cannot see a dlopen failure (#1777), a pass that cannot be told from silence (#1791). A guard whose
second direction is unexercised is one-directional by accident.

Two asks.

  1. Make the "suppresses nothing" case fail, or say why it cannot. For each allow entry, the checker can ask the same question it asks of unregistered tests in reverse: is this name
    already registered, or does it not exist?
    If yes → the entry suppresses nothing, and that is a stale allow-list entry, which should be an error (or at minimum a ::warning:: with the entry
    named) rather than the current silence. This is the arm that makes the list rot-proof.
  2. Normalise path vs bare name in both directions. Case 3 shows a tests/… prefix producing two messages and rc 1 — a confusing state, and the same normalisation that fixes it also makes
    ask 1 implementable.

A note on what this says about my own asks: I asked for an allow-list to exist (#1741, #1806) and did not ask what keeps its entries honest. The docstring in the merged version says reporting
"keeps it honest rather than a rubber stamp" — and case 2 shows reporting alone does not, because silence is the success state. That is worth one line in the file, in the same spirit as the three cases
above.

Not run: I did not run the checker (it needs a configure to have a ctest list). The review is of the three measured cases and of what the second direction still needs.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY 3aab4e4

Both fixes are right, and the second is the one that matters.

The basename normalisation removes a two-contradicting-messages bug: a tests/foo.sh entry matched no test, so it suppressed nothing
and was reported as "no such test exists any more" — the same line described as both dead and non-existent, with the format never named.
Normalising to PurePosixPath(...).name is the least surprising rule, and it is the one the allow-list's own entries use.

The idle arm is the tightening a ratchet needs. Splitting stale into gone (entry names a test that no longer exists → failure) and
idle (entry names a test that is referenced → "drop the line, it suppresses nothing" → also a failure) fixes the failure mode of every
allow-list: an entry that outlives its exception, warned about forever and never removed. The old arm's name said the quiet part out loud —
"a stale allow entry is progress, not drift" — and progress that nothing forces is how a list ends up suppressing something nobody chose.

Verified on the real tree, which is the transition that matters:

$ python3 tools/ci/tests_referenced_check.py --check     # origin/main      rc=0
$ python3 tools/ci/tests_referenced_check.py --check     # this head        rc=0
tests_referenced: all 73 test script(s) are referenced (1 deliberately not a row: anchor_rerun.sh)
$ python3 tools/ci/tests_referenced_check.py --self-test #                 0 failure(s)

So the tightening does not turn main red, and the self-test covers the new arms. The summary line also shows anchor_rerun.sh as the one
deliberate non-row — which is #1806's header note showing up where it should.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 3aab4e4 — and the measured justification is the part that makes it more than tidying.

# a path-form entry matched no test, so it suppressed nothing AND was reported as "no such test exists any
# more", two contradicting messages about one line and the format never named.
out[pathlib.PurePosixPath(name.strip().rstrip("/")).name] = reason.strip()

Normalising to a basename fixes a silent-nothing bug: a tests/foo.sh entry was inert (it suppressed no test) while the same line was reported as stale — the check complaining and not working at the same time, which is the worst of both.

The new idle list is the better half: an allow-listed test that is found and referenced is no longer needed in the list, and reporting it enforces the other direction. An allow-list that only ever grows is a place where exemptions go to be forgotten; one that reports its own idle entries expires them.

Both directions, both reported, and neither can be satisfied by deleting a line. Nothing to change.

@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 3aab4e4: READY

Both directions of the allow-list, which is what a ratchet needs. The key normalisation is the fix that makes the rest work — pathlib.PurePosixPath(name.strip().rstrip("/")).name means an entry written as tests/foo/ matches the test named foo, so the list cannot be silently bypassed by a trailing slash — and then gone (allow-listed but no longer needed) and idle (allow-listed and its test is present but not running) are reported separately.

Those are different mistakes: a stale entry is debt someone should delete, an idle one means the exemption is hiding a test that exists. Reporting them apart is what lets a reader act on either without guessing which one the tool means.

@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 3aab4e41 — ran the self-test at this head (0 failure(s)), and the two new arms are the ones that keep the allow-list from rotting.

"An allow entry for a test that IS referenced fails: it suppresses nothing" is the direction that most allow-lists never enforce, and it is the one that matters — a list that only grows is indistinguishable from a list that has been abandoned. Pairing it with "an allow entry for a deleted test fails" makes the file a ledger rather than a shrug.

The basename normalisation with the measured reason is the kind of detail I would otherwise have to request: a path-form entry under the old code "suppressed nothing AND was reported as 'no such test exists any more'", i.e. one line producing two contradicting messages about itself. Naming the format in the error closes it.

no tests/ directory is 'cannot tell' rather than a pass is the correct default for a check that would otherwise silently do nothing — same principle as #1828's inconclusive exit code, which I asked to be distinct.

@bong-water-water-bong
bong-water-water-bong merged commit 466d5a1 into main Nov 10, 2026
8 of 9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the os/v3-unreferenced-rot branch November 10, 2026 11:06
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.

1 participant