Repository navigation
ci: the unreferenced-tests allow-list enforces both directions (engine#1820) - #1835
Conversation
…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
left a comment
There was a problem hiding this comment.
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.
- 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. - 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.
|
VERDICT: READY Both fixes are right, and the second is the one that matters. The basename normalisation removes a two-contradicting-messages bug: a The Verified on the real tree, which is the transition that matters: So the tightening does not turn main red, and the self-test covers the new arms. The summary line also shows |
|
VERDICT: READY at Normalising to a basename fixes a silent-nothing bug: a The new Both directions, both reported, and neither can be satisfied by deleting a line. Nothing to change. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
Refs #1820. The row that merged in #1824 is good, and its second direction does not bite — measured on
main318b6ef0:Three consequences:
staleis computed, printed, and never reaches the exit code (if drift: return 1), so a deleted-test entry announces itself and passes for ever.allow_list()keys on the line as written whileneedis basenames, sotests/foo.shmatches 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/…), andtools/ci/otherwise writes repo-relative paths.Fix (23 lines, one file)
allow_list()normalises keys to basenames —tests/foo.shandfoo.share the same entry.stalesplits intogone(no such test → drop the line) andidle(its test IS referenced → the line suppresses nothing), each with its own message, and both fail the row.Red and green, measured on this head
Through the registered rows (reconfigured, under
thermal-run):tests_referenced0.23 s,tests_referenced_self0.09 s, 2/2 passed.Nothing else changes: same rule, same allow-list file, no new script, and
anchor_rerun.shremains the one legitimate entry.