From 3542d6f717f276ddc77ff50c655b04d9ade9fb88 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 21:42:07 -0700 Subject: [PATCH] fix(tooling): a leaf is not an identity, and a convenience flag was a bypass Both blockers from the adversarial review of #83, both in the mode this PR rewrote, both producing a pass while the gate did not do what its prose said. 1. cargo_test_names reduced every listed path to its last segment. Six leaf names collide in this workspace and two are cited in evidence, so a heading was satisfiable by a test in another module answering another question -- renaming ui::project::tests::a_load_that_fails_is_not_a_rebase, the one its entry describes, left the checker at exit 0 with the ui::host namesake answering for it. Paths are kept whole; a bare leaf resolves only when unique, and an ambiguous one is REFUSED with both candidates named. _RUST_NAME accepts `::`, so a qualified heading is a claim rather than prose, and a citation is matched on the leaf so qualifying a heading cannot orphan its own test. The two colliding entries are qualified. Neither half suffices alone: rename one of a colliding pair and the leaf becomes unique again, which is why qualification is the fix and the refusal is what forces it. 2. --rust-tests called cargo_test_names with one argument, so the --ignored subtraction never ran, and it printed a byte-identical success line. It now requires --rust-tests-ignored (one alone exits 2) and the line names its source. Also, from the same review: a missing context/test-evidence/ exited 0 while 170 citations pointed at nothing (early return, now gone); the doc-site "runs last" check accepted a trailing `echo` that merely NAMED the generator, so it asks for an invocation; bundle_freedom gains an arm for a transitive `import "../app.js"`, which reaches the bundle while naming nothing forbidden; and two prose overclaims are corrected -- the prose-heading count is a convenience rather than a guard (git diff is the mechanism), and the ratio rule says "above the run MEDIAN", which is defensible where "above the others" is false against second place. scripts/check-ci-graph.py makes claim 6's rule a gate: pages-deploy must `needs` every other job in ci.yml. The rule was stated in the last commit and a zz-new-check: job added during review was still ungated with nothing noticing. README now states the rule and lists no jobs, the second copy being what went stale twice. Every fault re-injected and observed; figures in context/test-evidence/tooling.md rather than only here. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- .github/workflows/ci.yml | 6 ++ README.md | 6 +- context/code/tooling.md | 65 ++++++++++-- context/design-principles.md | 6 +- context/test-evidence/command.md | 5 +- context/test-evidence/tooling.md | 62 +++++++++++ context/test-evidence/tui.md | 9 +- scripts/build-doc-site.mjs | 4 +- scripts/check-ci-graph.py | 124 ++++++++++++++++++++++ scripts/check-comment-runs.py | 173 ++++++++++++++++++++++++------- web/tests/bundle_freedom.test.js | 14 +++ 11 files changed, 418 insertions(+), 56 deletions(-) create mode 100755 scripts/check-ci-graph.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 26ceaf4e..06be797d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -146,6 +146,12 @@ jobs: run: python3 scripts/check-comment-runs.py - name: Test-evidence headings name real tests run: python3 scripts/check-comment-runs.py --check-evidence + # `pages-deploy` needs every other job, and saying so was not enough: a + # job added during review of PR #83 was ungated and nothing noticed. + - name: The deploy gate refuses what it claims to + run: python3 scripts/check-ci-graph.py --self-test + - name: Every check job gates the deploy + run: python3 scripts/check-ci-graph.py # Too slow on GitHub Actions # diff --git a/README.md b/README.md index c748b673..75c4f69d 100644 --- a/README.md +++ b/README.md @@ -182,9 +182,9 @@ improvise script setup.txt -f model.improv The browser page is at ****, built and published from `main` by the `build` / `pages-deploy` jobs in `.github/workflows/ci.yml`. A deploy only happens after **every** check job in -that workflow has passed — `test`, `wasm`, `js`, `browser`, `harness-lint`, -`kb` and `build`. Naming a subset is how that list went stale before: three of -the seven were listed and the rest read as ungated. +that workflow has passed. The list is deliberately not repeated here — naming a +subset is how this sentence went stale before, and `scripts/check-ci-graph.py` +now holds `pages-deploy`'s `needs` to the workflow's own job keys. The site **root** is the landing page — `docs/index.html`, with the recorded terminal casts — and it links to the app. `web/assemble.sh` puts the two diff --git a/context/code/tooling.md b/context/code/tooling.md index 267fa70d..6a2b1981 100644 --- a/context/code/tooling.md +++ b/context/code/tooling.md @@ -56,6 +56,27 @@ not be forced to have one; what is checkable is a *claim*, and both directions are claims. The failure they close is the same one a dead ticket ID is: a pointer that reads as a promise the thing exists. +### A leaf is not an identity + +The names cargo lists are **module paths**, and the checker keeps them whole. +Six leaf names collide in this workspace — two of them cited in evidence — so +reducing a path to its last segment made a heading satisfiable by a test in +another module answering another question. Renaming +`ui::project::tests::a_load_that_fails_is_not_a_rebase`, the one its entry +actually describes, left the checker at *579 entries, all name a test that +runs*, exit 0: the surviving `ui::host::tests` namesake answered for it. + +So a bare leaf resolves only when it is **unique**, and an ambiguous one is a +**refusal** naming the candidates — not a warning, because a warning re-creates +the problem for anyone who does not read it. The heading is then written with +its module path, which is also what makes the entry say which test it is about. + +Note the two halves are both needed and neither is sufficient. The refusal +forces qualification but cannot survive a rename on its own: rename one of a +colliding pair and the leaf becomes *unique*, silently resolving to the +survivor. Qualification is what closes that, and the refusal is what makes +somebody write it. + ### The names come from cargo, not from the source A heading is diffed against `cargo test --workspace -- --list`, minus the names @@ -81,9 +102,20 @@ Demonstrated before the fix, by injecting all four shapes into `src/draw.rs` and entries, all name a test that exists*, exit 0. The current one names all four. What this costs: `--check-evidence` now needs a toolchain and a compiled -workspace, about three seconds warm. `--rust-tests FILE` takes a saved listing -for a caller that has one, and a missing `cargo` says so rather than raising. -The comment-cap check is unaffected and needs neither. +workspace, about three seconds warm. A missing `cargo` says so rather than +raising. The comment-cap check is unaffected and needs neither. + +`--rust-tests FILE` takes a saved listing for a caller that has one, and it +**requires `--rust-tests-ignored FILE`**: a first version took one file, called +`cargo_test_names` with one argument, and so skipped the ignored rule entirely +while printing the same success line as the honest path. A flag that is +strictly more permissive and indistinguishable in the log is a bypass, not a +convenience — so the pair is mandatory and the success line **names its +source**. CI passes neither and runs cargo itself. + +A **missing** `context/test-evidence/` is not an early return either. It used to +be, and it exited 0 while 170 tests cited a directory that was not there; the +walk now yields nothing and every citation is reported unstated, which is loud. Note what the self-test can and cannot carry here. The listing *reader* is testable from fixtures — a leaf name taken off a module path, an `--ignored` @@ -110,12 +142,17 @@ honest — no mechanism could, because the question is whether a sentence is tru Two things make that tolerable. The unsafe direction is the one that fails loudly: prose that *looks* like an identifier is classified as a Rust claim and reported as an orphan, so the error you can make by accident is the noisy one. -And prose headings are **counted and printed** beside the claims, so a -retirement moves a number in the gate's own output instead of passing in -silence. The live instance — PR #69 demoting a heading while PR #72 restored its -coverage under a different name — is what says the reviewer is the mechanism: -`git diff context/test-evidence/` shows a `### identifier` line becoming prose, -and that diff is the thing to read. +And **the mechanism is `git diff context/test-evidence/`** — a demotion shows +up there as a `### identifier` line becoming prose, in the diff every reviewer +already reads. The live instance, PR #69 demoting a heading while PR #72 +restored its coverage under another name, is what says a reviewer rather than a +gate has to catch it. + +The printed count of prose headings is a **convenience, not a guard**: nothing +stores the previous count and nobody diffs two CI logs, so a retirement moves a +number no one is watching. Saying otherwise would claim a detectability it +cannot support, which is the class of defect §7's adjacent-check section is +about. ## 3. The others @@ -126,3 +163,13 @@ shape as this file's subject: a guard, plus a suite that makes the guard refuse what it claims to refuse. Each is wired into `.github/workflows/ci.yml` in the same job as the thing it guards, because a check that only runs locally is a convention. + +`scripts/check-ci-graph.py` is the workflow's own gate: `pages-deploy` must +`needs` every other job in `ci.yml`. The rule "every check job" was written down +(improvise-x7cq) and a job added during review of PR #83 was still ungated with +nothing noticing, which is what moved it from a sentence to a check. It reads +the job keys and the `needs:` list off indentation, tracking which job a +`needs:` belongs to — taking the first one seen would read another job's list as +the deploy's and pass. `README.md` therefore states the rule and deliberately +**does not list the jobs**: a second copy with nothing holding it to the first +is what went stale, twice. diff --git a/context/design-principles.md b/context/design-principles.md index b96bd51e..5a05f35a 100644 --- a/context/design-principles.md +++ b/context/design-principles.md @@ -345,8 +345,10 @@ worse than no claim at all. Three consequences: - **If you cite a figure, name the machine you took it on.** A number with no machine is a property of the suite, which it is not. - **The portable signal is the ratio, not the seconds.** One binary an order of - magnitude above the others is the thing to look at, and it is the same - binary on every machine it has been measured on: `improvise-io`'s lib, which + magnitude above the run's **median** is the thing to look at — not above + second place, which it need not be: on one machine `improvise-io` is 2.2× + the runner-up and ~15× the median. It is the same binary on all three + machines it has been measured on: `improvise-io`'s lib, which is proptest-dominated by the grammar-walking `.improv` generator and the round-trip properties over it. Constraining that generator's strategy is the open work (improvise-8shu); lowering the case count below 256 is not (see diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index ec26b005..2eee0b89 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -1711,7 +1711,10 @@ command that mutates the document in place, and a view-only one, are both diffs the question `apply_model` asks is whether the generation moved, not which variant arrived. -### a_load_that_fails_is_not_a_rebase +### ui::project::tests::a_load_that_fails_is_not_a_rebase + +Qualified because `ui::host::tests` has a test of the same name: this is the +`apply_model` one, and a bare leaf was satisfiable by either. A `LoadModel` naming a document the storage does not hold leaves the document alone, so it is a diff too. This is why `apply_model` asks the generation rather diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 622c03ac..b67733f6 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -54,6 +54,61 @@ Fault injected, 2026-09-21: moved `web/tests/bundle/wasm_surface.test.js` out of the tree. The test failed with "web/tests/bundle/ is empty — the split lost its other half" rather than passing. Restored; green again. +### "no test in web/tests/ imports a module that reaches the bundle" + +The companion to the lexical check above, and the reason that one is not +enough: `import "../app.js"` needs the bundle *transitively* and names nothing +forbidden, so the `js` job breaks loudly but unattributed. `app.js` and +`worker.js` are the two entry points that load the bundle; `shell.js` and +`serial.js` do not, and `serial.test.js` imports one legitimately. + +Fault injected, 2026-09-21: appended `import "../app.js";` to +`web/tests/grid_plan.test.js`. The lexical test stayed green; this one failed +naming the file. Reverted. + +### Note — the checker's own blockers, found by review of PR #83 + +Two gates in `scripts/check-comment-runs.py` produced a pass while not doing +what their prose said. Both were in the mode the same PR rewrote, which is the +part worth keeping: writing the doctrine down does not exempt the writer. + +**A leaf name is not an identity.** Six leaf names collide in this workspace and +two were cited in evidence. Renaming +`ui::project::tests::a_load_that_fails_is_not_a_rebase` — the one its entry +describes — left the checker printing *all name a test that runs*, exit 0, with +the heading answered by the `ui::host::tests` namesake. Now an ambiguous leaf is +refused with both candidates named, and the two entries are qualified. +Re-verified both ways, 2026-09-21: the refusal fires on the unqualified +headings and names exactly those two; with the heading qualified, the same +rename is reported as an orphan rather than absorbed. + +**`--rust-tests` was strictly more permissive and said the same words.** It +called `cargo_test_names` with one argument, so the `--ignored` subtraction +never ran, and the success line was byte-identical to the honest path's. +Injecting `### eyeball_the_rendered_hints_and_pages` orphaned on the default +path and passed on the flag's. Now the flag requires +`--rust-tests-ignored` (one alone exits 2) and the success line names its +source. + +**A missing evidence directory exited 0.** The early return preceded the +citation scan, so deleting `context/test-evidence/` reported nothing while 170 +citations pointed at a directory that was not there. Verified 2026-09-21 by +moving the directory aside: exit 1, *170 tests cite context/test-evidence and +have no entry there*. + +### Note — the stamp gate's discrimination, measured + +`scripts/assemble-stamp-test.sh` separates three outcomes rather than two, which +is what makes it evidence rather than a smoke test. Measured 2026-09-21 over its +nine cases: the real script passes 9; deleting the final `test -z "$missing"` +(the PR #69 injection, which the inline version survived at exit 0) passes 4 and +fails 5; a script that does nothing passes 2; one that always fails passes 0. + +The PR body first cited "4 of 9" for the middle row, from a run taken when the +suite had eight cases. A figure is what the next reader checks, so it is +recorded here where it can be re-measured rather than only in a message nothing +can audit (design-principles §6). + ### Note — the stamp gate matches literally `scripts/assemble-stamp.sh` greps with `-F`. As a regex, the `.` in @@ -78,6 +133,13 @@ Faults injected, 2026-09-21, each observed: | a new `context/code/newarea.md` | `context/code/newarea.md is neither published nor a named omission` | | `context/plan.md` moved out of the tree | `context/plan.md is listed as omitted but does not exist` | | a line appended below the generator in `web/assemble.sh` | `web/assemble.sh must invoke this generator LAST`, naming the offending command | +| a trailing `echo "see scripts/build-doc-site.mjs"` | the same, once the check asks for an INVOCATION rather than a mention — `.includes` had accepted it (review, PR #83) | + +`scripts/check-ci-graph.py` is the third, and it is a script rather than a +stanza because its own refusals need checking: adding a `zz-new-check:` job to +`ci.yml` reports *pages-deploy does not need `zz-new-check`* and exits 1, where +before the rule was prose and nothing noticed. Its five self-test graphs include +the one a naive parse gets wrong — another job's `needs:` read as the deploy's. The second arm is the one that would not get written first, and it is the point: without it, deleting or renaming an omitted file leaves an entry asserting a diff --git a/context/test-evidence/tui.md b/context/test-evidence/tui.md index f4e8df67..69547db0 100644 --- a/context/test-evidence/tui.md +++ b/context/test-evidence/tui.md @@ -384,10 +384,13 @@ the move onto a projected cache, one pinning the *drawing* of them. The pair is the shape — a cache equivalence alone would not notice a changed renderer, and a rendered fixture alone would not notice the cache carrying different data. -### the_cache_carries_what_the_widget_used_to_read_off_the_model +### ui::tile_bar::tests::the_cache_carries_what_the_widget_used_to_read_off_the_model -Two tests share this name, in `src/ui/tile_bar.rs` and -`src/ui/formula_panel.rs`. The tile bar used to read +### ui::formula_panel::tests::the_cache_carries_what_the_widget_used_to_read_off_the_model + +Two tests share this leaf name, which is why both headings are qualified: one +entry describes the pair, and an unqualified heading would have been satisfied +by whichever of them the checker happened to see. The tile bar used to read `view.tileable_cats(&model.category_names())` and `view.axis_of(cat)` off the model directly; the formula panel used to read `model.formulas()[i].raw`. Each now reads its cache, and a divergence is a rendering change nobody asked for. diff --git a/scripts/build-doc-site.mjs b/scripts/build-doc-site.mjs index 14045806..67dd049c 100644 --- a/scripts/build-doc-site.mjs +++ b/scripts/build-doc-site.mjs @@ -133,7 +133,9 @@ const OMITTED = new Map([ .split("\n") .map((l) => l.trim()) .filter((l) => l && !l.startsWith("#")); - if (!cmds[cmds.length - 1].includes("build-doc-site.mjs")) { + // An INVOCATION, not a mention: `echo "see scripts/build-doc-site.mjs"` after + // a real command satisfied `.includes` while running nothing (review, PR #83). + if (!/^(node|sh)\s.*build-doc-site\.mjs/.test(cmds[cmds.length - 1])) { console.error("doc-site: web/assemble.sh must invoke this generator LAST;"); console.error(`doc-site: its last command is instead: ${cmds[cmds.length - 1]}`); process.exit(1); diff --git a/scripts/check-ci-graph.py b/scripts/check-ci-graph.py new file mode 100755 index 00000000..c3fc9911 --- /dev/null +++ b/scripts/check-ci-graph.py @@ -0,0 +1,124 @@ +#!/usr/bin/env python3 +"""pages-deploy must `needs` every other job in the workflow. + +The rule that does not go stale is "every check job"; a subset is what let +README name three of seven while the rest read as ungated (improvise-x7cq). +Stating it was not enough -- a new job was added during review of PR #83 and +nothing noticed, so the rule is a gate. See context/code/tooling.md. + +`--self-test` checks the checker: each case is a graph this must refuse. +""" + +import re +import sys + +WORKFLOW = ".github/workflows/ci.yml" +DEPLOY = "pages-deploy" + +_JOB = re.compile(r"^ ([A-Za-z0-9_-]+):$") +_NEEDS = re.compile(r"^ needs:\s*\[(.*)\]\s*$") + + +def read_graph(text): + """(all job names in order, the names `pages-deploy` needs). + + Both come off indentation, which is what a workflow's own shape is. A job + key is two spaces; `needs:` is four INSIDE the job it belongs to, so the + current job is tracked rather than the first `needs:` taken. + """ + jobs, needs, current = [], None, None + in_jobs = False + for raw in text.split("\n"): + if raw.startswith("jobs:"): + in_jobs = True + continue + if not in_jobs: + continue + m = _JOB.match(raw) + if m: + current = m.group(1) + jobs.append(current) + continue + m = _NEEDS.match(raw) + if m and current == DEPLOY: + needs = [n.strip() for n in m.group(1).split(",") if n.strip()] + return jobs, needs + + +def verdict(jobs, needs): + """The complaints, as strings. Empty means the graph is sound.""" + if DEPLOY not in jobs: + return [f"{DEPLOY} is not a job in this workflow"] + if needs is None: + return [f"{DEPLOY} has no `needs:` list, so nothing gates the deploy"] + expected = {j for j in jobs if j != DEPLOY} + missing = sorted(expected - set(needs)) + unknown = sorted(set(needs) - expected) + out = [f"{DEPLOY} does not need `{j}`, so a failing {j} cannot stop a deploy" + for j in missing] + out += [f"{DEPLOY} needs `{j}`, which is not a job in this workflow" for j in unknown] + return out + + +# (name, workflow text, the complaints it must draw). +SELF_TEST = [ + ("a job nobody gates is named", + "jobs:\n test:\n x: 1\n zz-new-check:\n x: 1\n" + " pages-deploy:\n needs: [test]\n", + [f"{DEPLOY} does not need `zz-new-check`, so a failing zz-new-check cannot stop a deploy"]), + + ("the complete graph is accepted", + "jobs:\n test:\n x: 1\n wasm:\n x: 1\n" + " pages-deploy:\n needs: [test, wasm]\n", []), + + ("a needs naming a job that does not exist is caught", + "jobs:\n test:\n x: 1\n pages-deploy:\n needs: [test, ghost]\n", + [f"{DEPLOY} needs `ghost`, which is not a job in this workflow"]), + + ("a deploy with no needs at all is caught", + "jobs:\n test:\n x: 1\n pages-deploy:\n x: 1\n", + [f"{DEPLOY} has no `needs:` list, so nothing gates the deploy"]), + + # `needs:` belongs to the job it is indented under. Taking the first one + # seen would read ANOTHER job's list as the deploy's and pass. + ("another job's needs is not mistaken for the deploy's", + "jobs:\n test:\n x: 1\n browser:\n needs: [test]\n" + " pages-deploy:\n x: 1\n", + [f"{DEPLOY} has no `needs:` list, so nothing gates the deploy"]), +] + + +def self_test(): + failures = [] + for name, text, want in SELF_TEST: + jobs, needs = read_graph(text) + got = verdict(jobs, needs) + if got != want: + failures.append(f" {name}: expected {want}, saw {got}") + if failures: + print("ci-graph self-test FAILED:", file=sys.stderr) + print("\n".join(failures), file=sys.stderr) + return 1 + print(f"ci-graph self-test: {len(SELF_TEST)} graphs judged") + return 0 + + +def main(): + if "--self-test" in sys.argv[1:]: + return self_test() + jobs, needs = read_graph(open(WORKFLOW, encoding="utf8").read()) + if len(jobs) < 2: + print(f"error: {WORKFLOW}: found {len(jobs)} jobs; the parse is wrong", + file=sys.stderr) + return 2 + complaints = verdict(jobs, needs) + if complaints: + for c in complaints: + print(f"ci-graph: {c}", file=sys.stderr) + return 1 + print(f"ci-graph: {DEPLOY} needs all {len(jobs) - 1} other jobs") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/check-comment-runs.py b/scripts/check-comment-runs.py index 2878ca30..8f0b9432 100755 --- a/scripts/check-comment-runs.py +++ b/scripts/check-comment-runs.py @@ -253,7 +253,7 @@ EVIDENCE_DIR = os.path.join("context", "test-evidence") EVIDENCE_CITE = "test-evidence" JS_ROOTS = [os.path.join("harness", "tests"), os.path.join("web", "tests")] -_RUST_NAME = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") +_RUST_NAME = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*(?:::[A-Za-z_][A-Za-z0-9_]*)*$") _QUOTED_NAME = re.compile(r'^"(.+)"$') # Only a RUNNABLE LEAF test. `.skip`, `.todo`, `.describe` and `.only` are all # excluded: the first three name no executed assertion, and `.only` disables @@ -273,9 +273,9 @@ def evidence_claims(text): identifier (Rust) or a quoted string (JS). Anything else is prose about the section: `Helper - x`, `Note - y`, a file's own scope. That shape rule is the whole discrimination, and rewriting a claim as prose retires it with - nothing checking the prose is honest (improvise-9za9). The prose headings - are therefore COUNTED and reported, so a retirement moves a printed number - rather than passing in silence. See context/code/tooling.md. + nothing checking the prose is honest (improvise-9za9). The mechanism is the + diff a reviewer reads, not the printed prose count, which nothing stores. + See context/code/tooling.md. """ for n, raw in enumerate(text.split("\n"), 1): s = raw.strip() @@ -295,20 +295,23 @@ _CARGO_LISTED = re.compile(r"^(\S+): test$") def cargo_test_names(listing, ignored_listing=""): - """The leaf names cargo says it will RUN, from `cargo test -- --list`. + """The FULL PATHS cargo says it will run, from `cargo test -- --list`. - Scraping source cannot tell a test from text that looks like one, so the - heading set is diffed against this instead (improvise-rm4s). See - context/code/tooling.md. + Paths and not leaves: six leaf names collide in this workspace, so a leaf + is not an identity. See context/code/tooling.md and improvise-rm4s. """ - def leaves(text): + def paths(text): return { - m.group(1).split("::")[-1] + m.group(1) for m in (_CARGO_LISTED.match(line.strip()) for line in text.split("\n")) if m } - return leaves(listing) - leaves(ignored_listing) + return paths(listing) - paths(ignored_listing) + + +def _leaf(path): + return path.split("::")[-1] def _cargo_listing(extra): @@ -410,31 +413,53 @@ def js_files(roots): def evidence_verdict(claims, defined, citing): - """(orphans, unstated) -- the two directions, keyed by ("rust"|"js", name). + """(orphans, unstated, ambiguous), keyed by ("rust"|"js", name). `orphans` is evidence naming a test nobody can find. `unstated` is the reverse obligation (improvise-lyyc): a test that POINTS AT the evidence file must appear in it, or a reader following the pointer arrives nowhere. - Only that narrower obligation, because most tests have no evidence and - should not be forced to. Note what neither direction can see: an entry can - exist and still describe its subject backwards, which is discipline rather - than a gate. + `ambiguous` is a bare leaf that two tests answer to -- a refusal and not a + warning, because a heading satisfied by a test in another module answering + another question is a pass while the gate does nothing. + Only that narrower reverse obligation, because most tests have no evidence + and should not be forced to. What none of the three can see: an entry can + exist and still describe its subject backwards. """ - orphans = {k: v for k, v in claims.items() if k[1] not in defined[k[0]]} + by_leaf = {} + for path in defined["rust"]: + by_leaf.setdefault(_leaf(path), set()).add(path) + + orphans, ambiguous = {}, {} + for key, where in claims.items(): + kind, name = key + if kind == "js": + if name not in defined["js"]: + orphans[key] = where + continue + if name in defined["rust"]: + continue + matches = by_leaf.get(name, set()) + if len(matches) == 1: + continue + (ambiguous if matches else orphans)[key] = where + + # A citation is a bare `fn` name, so it is satisfied by a QUALIFIED heading + # naming that leaf -- qualifying a heading must not orphan its own test. + claimed = {(kind, _leaf(name)) for kind, name in claims} unstated = {} for kind, names in citing.items(): for name in names: - if (kind, name) not in claims: + if (kind, name) not in claimed: unstated.setdefault((kind, name), []).extend(sorted(names[name])) - return orphans, unstated + return orphans, unstated, ambiguous -def check_evidence(roots, rust_listing=None): - """Both directions over context/test-evidence/. See evidence_verdict.""" - if not os.path.isdir(EVIDENCE_DIR): - print("no context/test-evidence/ yet: nothing to check") - return 0 +def check_evidence(roots, rust_listing=None, rust_ignored=None): + """Both directions over context/test-evidence/. See evidence_verdict. + A MISSING evidence directory is not an early return: it used to be, and it + exited 0 while 170 citations pointed at nothing (review of PR #83). + """ claims, prose = {}, 0 for dirpath, _, filenames in os.walk(EVIDENCE_DIR): for f in sorted(filenames): @@ -449,9 +474,12 @@ def check_evidence(roots, rust_listing=None): claims.setdefault((kind, name), []).append(f"{path}:{n}") if rust_listing is None: + source = "cargo" rust_names = cargo_test_names(_cargo_listing([]), _cargo_listing(["--ignored"])) else: - rust_names = cargo_test_names(open(rust_listing, encoding="utf8").read()) + source = f"{rust_listing} + {rust_ignored}" + rust_names = cargo_test_names(open(rust_listing, encoding="utf8").read(), + open(rust_ignored, encoding="utf8").read()) defined = {"rust": rust_names, "js": set()} citing = {"rust": {}, "js": {}} @@ -464,9 +492,10 @@ def check_evidence(roots, rust_listing=None): for name in c: citing["js"].setdefault(name, []).append(path) - orphans, unstated = evidence_verdict(claims, defined, citing) - if not orphans and not unstated: - print(f"test-evidence: {len(claims)} entries, all name a test that runs; " + orphans, unstated, ambiguous = evidence_verdict(claims, defined, citing) + if not orphans and not unstated and not ambiguous: + print(f"test-evidence: {len(claims)} entries, all name a test that runs " + f"(names from {source}); " f"{sum(len(v) for v in citing.values())} tests cite the file and all have one; " f"{prose} prose headings claim nothing") return 0 @@ -474,6 +503,16 @@ def check_evidence(roots, rust_listing=None): print(f"{len(orphans)} test-evidence entries name no test:\n", file=sys.stderr) for (kind, name), where in sorted(orphans.items()): print(f" [{kind}] {name} ({', '.join(where)})", file=sys.stderr) + if ambiguous: + by_leaf = {} + for path in defined["rust"]: + by_leaf.setdefault(_leaf(path), set()).add(path) + print(f"\n{len(ambiguous)} test-evidence entries name a leaf that is not unique; " + "write the module path:\n", file=sys.stderr) + for (kind, name), where in sorted(ambiguous.items()): + print(f" [{kind}] {name} ({', '.join(where)})", file=sys.stderr) + for path in sorted(by_leaf[name]): + print(f" -> {path}", file=sys.stderr) if unstated: print(f"\n{len(unstated)} tests cite {EVIDENCE_DIR} and have no entry there:\n", file=sys.stderr) @@ -713,17 +752,17 @@ fn g() {} ] -# (name, listing, ignored listing, the names that RUN). +# (name, listing, ignored listing, the paths that RUN). # # Three source shapes defeated the old scrape -- a test in a string literal, one # behind a cfg feature, one behind cfg(target_arch = "wasm32") -- and an ignored # test defeats a listing read without its `--ignored` twin (improvise-rm4s). SELF_TEST_LISTING = [ - ("a leaf name is taken off the module path", - "help::tests::a_real_test: test\n", "", ["a_real_test"]), + ("the module path is kept, because a leaf is not an identity", + "help::tests::a_real_test: test\n", "", ["help::tests::a_real_test"]), ("an ignored test is one that does not run", - "a::runs: test\na::skipped: test\n", "a::skipped: test\n", ["runs"]), + "a::runs: test\na::skipped: test\n", "a::skipped: test\n", ["a::runs"]), ("cargo's prose is not a test name", " Running unittests src/lib.rs (target/debug/deps/x-1)\n" @@ -813,9 +852,58 @@ SELF_TEST_HEADINGS = [ ("a heading is classified three ways, prose included", '#### Helper — `addressed`\n### a_rust_name\n#### "a js name"\n', ["prose", "rust", "js"]), + + ("a module-qualified heading is a Rust claim, not prose", + "### ui::project::tests::a_load_that_fails_is_not_a_rebase\n", ["rust"]), ] +# (name, evidence markdown, cargo listing, orphans, ambiguous). +# +# Six leaf names collide in this workspace and two were cited, so a heading was +# satisfiable by a test in another module answering another question -- renaming +# the one an entry describes left the checker at exit 0 (review of PR #83). +SELF_TEST_AMBIGUITY = [ + ("a leaf two tests answer to is refused, not silently resolved", + "### a_load_that_fails_is_not_a_rebase\n", + "ui::host::tests::a_load_that_fails_is_not_a_rebase: test\n" + "ui::project::tests::a_load_that_fails_is_not_a_rebase: test\n", + [], ["a_load_that_fails_is_not_a_rebase"]), + + ("qualifying it resolves it, and to the one it names", + "### ui::project::tests::a_load_that_fails_is_not_a_rebase\n", + "ui::host::tests::a_load_that_fails_is_not_a_rebase: test\n" + "ui::project::tests::a_load_that_fails_is_not_a_rebase: test\n", + [], []), + + ("a qualified heading naming a module that does not have it is an orphan", + "### ui::nowhere::tests::a_load_that_fails_is_not_a_rebase\n", + "ui::host::tests::a_load_that_fails_is_not_a_rebase: test\n", + ["ui::nowhere::tests::a_load_that_fails_is_not_a_rebase"], []), + + ("a unique leaf still resolves unqualified", + "### a_lonely_test\n", "m::tests::a_lonely_test: test\n", [], []), +] + + +def _ambiguity_self_test(): + failures = [] + for name, md, listing, want_o, want_a in SELF_TEST_AMBIGUITY: + claims = {} + for kind, cname, n in evidence_claims(md): + if kind != "prose": + claims.setdefault((kind, cname), []).append(f"fixture:{n}") + defined = {"rust": cargo_test_names(listing), "js": set()} + orphans, _, ambiguous = evidence_verdict(claims, defined, {"rust": {}, "js": {}}) + got_o = sorted(k[1] for k in orphans) + got_a = sorted(k[1] for k in ambiguous) + if got_o != sorted(want_o): + failures.append(f" ORPHANS: {name}: expected {sorted(want_o)}, saw {got_o}") + if got_a != sorted(want_a): + failures.append(f" AMBIGUOUS: {name}: expected {sorted(want_a)}, saw {got_a}") + return failures + + def _listing_self_test(): failures = [] for name, listing, ignored, want in SELF_TEST_LISTING: @@ -830,7 +918,7 @@ def _listing_self_test(): def _evidence_self_test(): - failures = _listing_self_test() + failures = _listing_self_test() + _ambiguity_self_test() for name, md, rs, js, listing, want_orphans, want_unstated in SELF_TEST_EVIDENCE: claims = {} for kind, cname, n in evidence_claims(md): @@ -841,13 +929,15 @@ def _evidence_self_test(): defined = {"rust": cargo_test_names(listing), "js": jd} citing = {"rust": {k: ["fixture.rs"] for k in rc}, "js": {k: ["fixture.js"] for k in jc}} - orphans, unstated = evidence_verdict(claims, defined, citing) + orphans, unstated, ambiguous = evidence_verdict(claims, defined, citing) got_o = sorted(k[1] for k in orphans) got_u = sorted(k[1] for k in unstated) if got_o != sorted(want_orphans): failures.append(f" ORPHANS: {name}: expected {sorted(want_orphans)}, saw {got_o}") if got_u != sorted(want_unstated): failures.append(f" UNSTATED: {name}: expected {sorted(want_unstated)}, saw {got_u}") + if ambiguous: + failures.append(f" AMBIGUOUS: {name}: unexpected {sorted(k[1] for k in ambiguous)}") return failures @@ -875,7 +965,8 @@ def self_test(): print(f"checker self-test: {len(SELF_TEST_CASES)} violations caught, " f"{len(SELF_TEST_CLEAN)} legitimate shapes allowed, " f"{len(SELF_TEST_EVIDENCE)} evidence verdicts, " - f"{len(SELF_TEST_LISTING) + len(SELF_TEST_HEADINGS)} listing readings") + f"{len(SELF_TEST_LISTING) + len(SELF_TEST_HEADINGS)} listing readings, " + f"{len(SELF_TEST_AMBIGUITY)} name resolutions") return 0 @@ -889,7 +980,11 @@ def main(): ap.add_argument("--check-evidence", action="store_true", help="every context/test-evidence heading must name a test that exists") ap.add_argument("--rust-tests", metavar="FILE", - help="a saved `cargo test --workspace -- --list`; the default runs it") + help="a saved `cargo test --workspace -- --list`; the default runs it. " + "Requires --rust-tests-ignored: one file alone would skip the " + "ignored-test rule entirely") + ap.add_argument("--rust-tests-ignored", metavar="FILE", + help="the matching `cargo test --workspace -- --list --ignored`") ap.add_argument("--self-test", action="store_true", help="check the checker against the bypasses it has been fixed for") args = ap.parse_args() @@ -913,7 +1008,11 @@ def main(): return 2 if args.check_evidence: - return check_evidence(roots, args.rust_tests) + if bool(args.rust_tests) != bool(args.rust_tests_ignored): + print("error: --rust-tests and --rust-tests-ignored come together; one alone " + "silently drops the ignored-test rule", file=sys.stderr) + return 2 + return check_evidence(roots, args.rust_tests, args.rust_tests_ignored) if args.list_doctests: for path in sorted(rust_files(roots)): diff --git a/web/tests/bundle_freedom.test.js b/web/tests/bundle_freedom.test.js index 8a06b9dc..cddcc0d8 100644 --- a/web/tests/bundle_freedom.test.js +++ b/web/tests/bundle_freedom.test.js @@ -33,6 +33,20 @@ test("no test in web/tests/ reaches for a built bundle", () => { ); }); +test("no test in web/tests/ imports a module that reaches the bundle", () => { + // The check above is lexical and DIRECT-only: `import "../app.js"` needs the + // bundle transitively and names nothing forbidden. app.js and worker.js are + // the two entry points that load it; shell.js and serial.js do not. + const siblings = readdirSync(dir).filter((f) => f.endsWith(".test.js")); + const reaches = /^\s*import\b[^;]*["']\.\.\/(app|worker)\.js["']/m; + const offenders = siblings.filter((f) => reaches.test(code(f))); + assert.deepEqual( + offenders, + [], + `importing ../app.js or ../worker.js executes the bundle loader: ${offenders}`, + ); +}); + test("the subdirectory that MAY use a bundle is not the one being checked", () => { // Reading it proves the exemption is a real directory rather than a // convention nobody kept, and that this check is scoped to the flat listing.