test(features): gate every per-language test on the grammar it names - #1479
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1479 +/- ##
=======================================
Coverage 98.02% 98.03%
=======================================
Files 359 359
Lines 95916 95916
Branches 95485 95485
=======================================
+ Hits 94022 94031 +9
+ Misses 1213 1205 -8
+ Partials 681 680 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
dekobon
added
the
gate-narrowing-intended
The `--compare` escape hatch could never work. `github.event.pull_request .labels` is a snapshot taken when the run was queued, so a label applied in response to the failing run is invisible to it — and re-running the job replays the same stale payload, so the remedy the failure message documents had no way to take effect. Measured on #1479: the label was applied, the job re-ran, and the step failed identically. The step now queries the PR's labels through the API at run time, which is correct regardless of ordering and survives a re-run. `lint` gains a job-scoped `pull-requests: read` for it, matching the file's convention that jobs needing more than the read-only default escalate explicitly. Worth noting the mechanism had never been exercised: the label itself did not exist until this PR needed it, and the condition that consumed it was wrong. An escape hatch nobody has opened is a design sketch.
Independent review (fable): 13 findings, 12 fixed in
|
`mk_langs!` generates every `*Parser` alias, `*Code` tag and `LANG` variant unconditionally; only `get_ts_language`'s arms are cfg'd. So `check_metrics::<PythonParser>(…)` compiles with `python` off and then panics in `Tree::new`. Nothing caught that: `make pre-commit` builds only the default and `--all-features` flavours, and the feature-matrix legs compiled the tests without running them. `utils/check-test-lang-gates.py` derives the marker an item needs from the languages its body reaches, reading the language table out of the `mk_langs!` invocation rather than keeping a copy of it. `--fix` writes the markers, `--show` prints the derivation. It checks three directions, because each is blind to the next. **Too narrow** — the item is admitted into a build lacking a grammar it names, and panics. A test needs `all(…)` of what it names; a helper or fixture table needs `any(…)` of its users; a sweep needs only `any(…)` of the rows it iterates, since `is_enabled` skips the rest, but still needs whatever parser it pins through a type parameter. **Too wide** — the gate requires a feature the body never uses, keeping the item out of builds it could have run in. Nothing else can see this: too wide panics, too narrow just drops the test and the leg stays green. Sixteen gates in the tree are deliberately wider than their bodies and say so with a `// test-lang-gates: hand-written(<features>) — <why>` marker; a marker that stops being load-bearing fails too, so the list stays a census rather than accumulating. **Neither** — when a marker and the derivation agree and are both wrong, only history says so. `--compare <ref>` scans the tree at `ref` as well (`git archive`, no cargo), derives which single-language builds compile each test in each, and fails on one that still exists but stopped being built somewhere. The derivation rules that took measuring: - A `LANG` being *compared* is an identity test, not a parse — the enum exists without its grammar. Covers `==`, `matches!` (every alternative, not just the first), `assert_eq!` (read with balanced parens, so a variant behind a call in the expected value is still a comparison) and receiver position, except the two methods that hand back the grammar itself. - Deliberately *not* extended to the later arms of a `match`, where `|` groups a dispatch table a sweep legitimately derives from. - A sweep over the whole enum that filters on `is_enabled` *and* asserts non-vacuity needs `any(<every language>)`: it fails rather than skips with none enabled, and its fixtures come from a `LANG`-parameterised helper whose arms are not attributed to callers. - `c-family-helpers` is spelled differently in the two crates, and getting it wrong is silent. 81 self-tests, including the repository check that the derivation reproduces all 167 gates humans had written by hand — which is what earned it the right to generate the rest. Refs #1472, #1413, #1478
Measured across `src/`, `big-code-analysis-ast/src/` and `tests/`: about 2,950 tests named a grammar without gating on it, so `cargo nextest run --no-default-features --features rust -p big-code-analysis` failed in the thousands and a partial-feature build could not be used to verify anything — the misleading-red class #1171 fixed for the corpora. The markers are derived and written by `check-test-lang-gates.py`, not hand-placed, and sit on the attribute stack alongside `#[test]`. No test changed its name, its assertions, or whether it runs under `--all-features`: `cargo nextest list --all-features --workspace` is byte-identical before and after, which is the safety net for a mechanical pass this size and the reason per-`fn` attributes were used rather than new wrapping modules, which would have renamed every test path and destroyed it. Four shapes the derivation cannot see carry a hand-written marker instead, sixteen items in all: a corpus walk choosing a language per file from a glob, a non-vacuity anchor, a `mod` declaration with no body to read, and a grammar picked out of a `"foo.rs"` extension string. Three source changes come with it: - `suppression_test.rs`'s `analyze_lang` took the language from the path's extension, which hid it from the scan and from the reader; it now takes a `LANG` at the call site. - The two library roots and the five integration-test crate roots relax `unused_imports` on partial builds only. Per-language gating makes "is this import live" a function of the enabled feature set, which no `cfg` on the import can express; `all-languages` is on by default and under `--all-features`, so the builds that matter still police it. - `assert_csharp_fixture_spells` is gated to match its callers, the "also noticed" item in #1472. One latent defect surfaced: six `Ccomment` / `Preproc` tests would have been gated out of every default build by the root crate's `c-family-helpers` not being what its ast-crate namesake enables. Refs #1472, #1413
`check-feature-gates.py` classified a `mod` as test-bearing only when it carried its own bare `cfg(test)`, so a union-gated `mod` inside an already-`#[cfg(test)]` parent was never checked at all. Latent while every subject was top-level, and about to stop being latent: gating the per-language tests creates exactly those nested gated modules. The naive repair breaks `subject_matcher`, which anchors on the bare name — admitting any `mod` makes a subject called `tests` match every `::tests::` path. Subjects now carry their full module path and match on that. Two smaller defects in the same scan, both found while testing it: a brace on a later line made a declaration read as a body and pass vacuously, and `splitlines()` disagreed with the line indices everywhere else in the file, raising `IndexError` on a trailing form feed. Fixes the third item of #1472.
The legs ran `cargo check --all-targets` and nothing else, so the "minimal grammar set" guarantee from #252 was compile-only and a regression that shows up solely in a partial build could land unseen. They now run `cargo nextest run`, which is what makes the per-language gating regress-testable rather than a one-off pass. Three binaries are excluded by exact name: their fixtures live in the `tests/repositories/` submodules this job does not check out. Exact matchers, because `not binary(corpus)` is a substring match that excluded nothing from the CLI package — 1,412 tests before and after — and 19 of them read a real source file from the DeepSpeech corpus. The `lint` job also gains `check-test-lang-gates.py --compare` against the base branch head, the one direction the marker checks cannot reach. It reads the `gate-narrowing-intended` label **live** rather than from `github.event.pull_request.labels`: that payload is a snapshot from when the run was queued, so a label applied in response to the failing run is invisible to it, and a re-run replays the same stale copy — the documented remedy could never have taken effect. The job escalates to `pull-requests: read` for that, per this file's convention. Closes #1285.
`check-test-lang-gates` and its self-tests join `make pre-commit` and `make ci`, and the pre-commit hooks. Unlike `check-feature-gates`, both arms go in: this one is a pure source scan with no cargo invocation, so it costs about three seconds. `check-test-lang-gates-compare` is deliberately left out of both. It needs a base revision to mean anything and a working tree mid-edit has none, so CI supplies the PR's base; by hand it is `make check-test-lang-gates-compare COMPARE_REF=origin/main`. The variable is spelled out rather than a bare `REF`, which `?=` would pick up from an exported environment variable of that name.
`.claude/rules/testing.md` gains the per-test rule under the existing fixture-table section: what the derivation reads, the four shapes it cannot see and must be told about by marker, the two directions the gate checks, and `--compare` for the third. Also the trust boundaries, which matter more than the happy path — the sweep pin covers a parser named as a *type* and nothing else (#1480), and the `allow(unused_imports)` carve-out is scoped to partial builds so the full build still polices every import. `AGENTS.md` lists both gates and the out-of-band comparison; `CHANGELOG` records the change under Unreleased. It is test- and CI-only, so nothing for `STABILITY.md`. The counts are given as approximate on purpose. They move with the derivation rules themselves — every refinement that stops reading something as a use lowers them — and an earlier draft of this section quoted a figure taken before the comparison rules landed.
`pgrep -f` matches the full command line, so a loop passed as `-c` text
— which is how the assistant's shell tool issues everything — finds its
own argv:
until ! pgrep -f 'make pre-commit' >/dev/null; do sleep 30; done
Ten of these accumulated in one session, seven waiting on a gate that
had written `BCA_GATE: pass` hours earlier. They match each other too,
so killing one does not release the rest. Silent in the way the rest of
this file is about: it reads as "still running", which is what the truth
looks like until it isn't.
Prefer a condition that is not a process — and bound it, because an
unbounded wait on a job that dies is the same hang by another route.
Where it must be a process, `[m]ake` breaks the self-match. `$$` does
not: the command substitution and subshell inherit the argv too, so
filtering one PID can never empty the list.
Every snippet in the section was run before being written down,
including one extracted back out of the committed file. The first draft
was not, and both of its unrun examples were wrong.
Unrelated to the rest of this branch; it is the tooling lesson from
doing the work.
ed26071 to
1cdeb63
Compare
Lesson 6 said to derive an assertion from an external source rather than from the code's own output, and framed it around snapshots. The same mechanism cost the most on #1478: `over_gated` checked ~3,200 generated markers against the derivation that generated them, so it could only catch hand edits; and a triage computed from that derivation certified 34 over-gated tests as legitimate. The Lesson paragraph now covers any expected value that is produced rather than asserted, and asks for a named oracle outside the model. The sub-example records which oracles actually found the defects — prior hand-written gates, the previous revision, a fresh-context review — and that the history comparison was the one check the narrowing label switched off.
1 participant
Closes #1472, #1413, #1285, #1478.
Per-language tests named a concrete parser —
check_metrics::<PythonParser>(…)— without gating on that grammar.
mk_langs!generates every*Parseralias,*Codetag andLANGvariant unconditionally, so such a test compiles withpythonoff and then panics inTree::new. Measured onmain: 3,025 testsnamed a grammar without gating on it, and
cargo nextest run --no-default-features --features rust -p big-code-analysisfailed in thethousands. A partial-feature build could not be used to verify anything — the
same misleading-red class as the missing-corpora case #1171 fixed.
What landed
utils/check-test-lang-gates.pyderives the marker an item needs from thelanguages its body reaches, reading the language table out of the
mk_langs!invocation rather than keeping a copy. 3,500 gated items are checked on every
make pre-commit;--fixwrites markers,--showprints the derivation.The
feature-matrixCI legs now run the suite instead of only compiling it— every target but the three corpus-dependent binaries, whose fixtures live in
a submodule that job does not check out. That is the ratchet #1285 asked for,
and it is what makes the gate regress-testable.
The gate checks both directions. Too narrow is the silent one: too wide
panics on the leg that lacks the grammar, too narrow just drops the test and a
leg running two hundred fewer tests looks exactly like a green run. Sixteen
gates in the tree are deliberately wider than their bodies justify — a corpus
walk picking a language per file, a non-vacuity anchor, a
moddeclaration —and each says so with a marker naming which features and why:
A gate that grows a feature nobody can account for fails, and so does a marker
that stops being load-bearing — otherwise the sixteen stop reading as a census.
--compare <ref>covers the residue. Everything above compares a markeragainst the derivation; when the two agree and are both wrong, only the
previous revision says so — and that is computable statically. It scans the
tree at
reftoo (git archive, no cargo, no second build), derives whichsingle-language builds compile each test in each, and fails on a test that
still exists but stopped being built somewhere. The
lintjob runs it per PRagainst the merge base. A deliberate narrowing looks identical and is waved
through with the
gate-narrowing-intendedlabel — not an in-source marker,which would be permanently stale the moment this lands.
check-feature-gates.py(#1472 item 3) no longer misses a union-gatedmodnested inside an already-
#[cfg(test)]parent — latent before this branch, andabout to stop being latent because the pass creates nested gated modules.
Two derivation rules, both verified by revert
A comparison is not a use.
lang == LANG::Goasks which variant a value is;the enum is generated unconditionally, so it parses nothing. Reading it as a
requirement is what conjoined
feature = "go"ontocontainers_emit_npm_and_npaand dropped the positive half of the #1197 contract from every build without Go.
Deliberately not extended to the later arms of an or-pattern —
| LANG::Xisalso how a dispatch table groups its arms, and excluding those cost eleven items
their whole C-family union.
A sweep still needs the parsers it hardcodes.
is_enabledfiltering earns arow set only
any(…); a parser named through a type parameter cannot beskipped by any runtime filter, so it stays required even inside a sweep, and it
propagates through helpers exactly as
needsdoes.Defects this found
tests/api/main.rsgatedmod parser_reuseonall(rust, typescript)whenonly three of its five tests need TypeScript. The other two had been dropped
from every Rust-only build — before test(abc): the test module is ungated, so a minimal-feature build reports ~2,600 spurious failures #1472, then blessed by a marker written
during it. That leg now runs two tests it did not before.
Ccomment/Preproctests would have been gated out of every defaultbuild by an asymmetry between the root and ast crates'
c-family-helpersfeatures. Handled by
feature_atom, which spells the atom per crate.Verification
was allowed to generate anything. If it could not reproduce what careful
humans wrote over twelve months, the mechanical pass would have baked the
error in 2,835 times.
cargo nextest list --all-features --workspaceunchanged — the set, notthe count. The only difference against the baseline is ten
#[ignore]dentries the two listings count differently; nothing was lost.
comparison spelling.
make pre-commit:BCA_GATE: pass.Review notes
A fresh-context review of the over-gate check returned twelve findings, four of
which changed a verdict and three of which were invisible from the gate's own
output — it reported clean either way. The over-gating rule collapsed from a
rule-plus-two-exclusions into one;
over_gatedlearned to skippubitems;hardcodedlearned to propagate through helpers; and--fixlearned to write amarker that satisfies the check that asked for it (it previously stacked four
#[cfg]s on a sweep-that-pins and exited 2). Five of the nineteen markers thefirst cut accepted were artefacts of those defects — hence sixteen.
The last two commits are unrelated to the gate: a
.claude/rules/shell.mdentryfor a
pgrep -fwait loop that matches its own command line and never exits,which leaked ten stuck waiters during this work, plus the review-driven
correction to it.
Not covered
The diff is dominated by ~3,235 mechanical
#[cfg]attribute lines across 65.rsfiles. Worth a.git-blame-ignore-revsentry if this repo starts keepingone.