Repository navigation
Pre-push gate: run selected classes by full name, RED on zero tests, STOP on a whole-suite selection (#5868) - #5870
Merged
Conversation
…0, STOP on a whole-suite selection (#5868) The test map and the symbol scan name classes by their simple name, and the runner's -class filter matches only a full name: every map-selected batch matched nothing, printed Total: 0, exited 0, and the gate printed GATE GREEN. The gate now resolves each name against the build's own -list classes/json (keyed as CI's shard scripts key it), cuts batches by argument length, turns any run that reports Total: 0 into GATE RED, and stops with GATE STOP (exit 3) when one project's selection is over 400 classes. Adds a behavior test with a fake dotnet (.github/scripts/tests/pre-push-test.sh) and documents the two new lines in the lane-orders skill.
erikdarlingdata
marked this pull request as ready for review
October 15, 2026 16:39
There was a problem hiding this comment.
Verdict: LGTM
Claims
- VERIFIED — "Each selected name... is matched against the build's own
-list classes/jsonby its simple name, split on[.+/]the waytest-map.py,ci-select.py... split it" — pre-push.sh:166-199 implements the resolve step;ci-select.py:363(re.split(r"[.+/]", cls)[-1]) andtest-map.py:95(re.split(r"[.+/]", full)[-1]) use the identical regex. - VERIFIED — "A failed class listing makes the gate RED" — pre-push.sh:190-193 (
red+=(...)onlist_classesfailure outside--dry-run). - VERIFIED — "Any run whose last
Total:is 0 or missing makes the gate RED... The Guard stage is covered too" — pre-push.sh:51-57, exercised for both the Guard calls (pre-push.sh:73-74) and the class batches (pre-push.sh:236). - VERIFIED — "Whole suite is a STOP... never GREEN... the Guard stage still runs first, and a red there wins" — pre-push.sh:211-213 and the final exit sequence at pre-push.sh:249-256 (red checked before stop).
- VERIFIED — "Batches cut by length... budget
PRE_PUSH_ARG_BUDGET, default 30,000, the same as CI's shard script" — pre-push.sh:30,219-223 matches the cost formula and$argBudget = 30000inrun-darling-pg-shard.ps1:120,125. - VERIFIED — "CI does not run the file" — no
.github/workflows/*.ymlreferencespre-push.shorpre-push-test.sh. - UNVERIFIED — "The xunit v3 runner's
-classfilter matches only the full name... printsTotal: 0and exits 0" — this is live runner behavior; this review ran on a read-only tree with no dotnet build available, so the direct-run evidence in the PR body could not be reproduced here. - UNVERIFIED — the test-plan's concrete run outputs (
pre-push-test: all passed, the three mutant-copy failure counts, the realGATE STOP/GATE GREENruns with specific class counts) — all require executingpre-push-test.shand/or a real Debug build against live nightly-map data; this sandbox's safety hook blocksdeclare -A(used throughout the script and its tests), so the script could not be run here to confirm.
Riskiest lines
- pre-push.sh:195-199 — the awk step that resolves a selected simple class name to the runner's full name (the actual root-cause fix; a wrong match here silently runs the wrong class or nothing) — pinned by
pre-push-test.shcase "green" (checks nestedOuter+Innernames reach the runner and a helper class lands onunlisted:rather than running). - pre-push.sh:211-213 — the
GATE STOPthreshold computed from post-resolutionsel_ncounts — pinned bypre-push-test.shcase "stop" (401 classes → exactGATE STOPline, exit 3, no-classrun). - pre-push.sh:51-56 — turning an exit-0/
Total: 0run into a red (the original defect being fixed: a matched-nothing run used to read as a pass) — pinned bypre-push-test.shcases "zero" and "guard0".
Traps near this diff
- "Under
set -euo pipefail, a bare$(gh api …)assignment that fails kills the step" (Workflows and CI) — does not apply: pre-push.sh:29 usesset -uo pipefail(no-e), and thegh apicall at pre-push.sh:91-92 is wrapped with2>/dev/null, so a failed lookup degrades to an emptyidsrather than aborting the script. - "Ruling (#5833): tests are load-bearing... answered with a behaviour test that fails on the defect and passes on the fix" (Tests and censuses) — applies, and the diff satisfies it rather than violating it:
pre-push-test.shdrives the real gate through a fakedotnetand asserts on its exit code/output, not on source text; its header even states "CI does not run this file... These are behavior tests, not source pins," matching the ruling. - Lite/Darling parity (Repository mechanics) — does not apply to this file (dev tooling, not SKU app code), and the script itself treats both suites symmetrically throughout (
for suite in darling lite,$DARLING/$LITEhandled identically at every step changed here).
Divergence (blind read vs PR body)
None — the body's account matches the diff on every claim gradable from the tree. The one acknowledged behavior change not framed as a "fix" — class batches no longer run at all once the Guard stage, a class listing, or the STOP threshold has already decided the gate (pre-push.sh:249-250), versus the old code which ran them unconditionally — is explained inline by the new comment at pre-push.sh:249 and isn't claimed otherwise by the body; non-blocking.
Reviewer checklist
- Run
bash .github/scripts/tests/pre-push-test.shfor real (blocked here by a sandbox hook ondeclare -A) and confirm the "15 PASS" / mutant-failure counts the PR body reports. - Run
bash .github/scripts/pre-push.shonce on a real Debug build to confirm the simple→full name resolution actually matches live-list classes/jsonoutput (including a nestedOuter+Innerclass) and thatGATE STOPfires as described given the current nightly map.
This was referenced Oct 15, 2026
Closed
Test map: a one-file test, docs or script change no longer selects most of both suites (#5872)
#5877
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Claude posting for Erik Darling.
Fixes #5868.
Why: the cause, with evidence
The issue's guess (a filter past the command-line limit) is not the cause. The test map and the gate's symbol scan both name a class by its simple name (
AgAlertEvaluatorTests), and the gate passed that name straight to-class. The xunit v3 runner's-classfilter matches only the full name (Lite.Tests.AgAlertEvaluatorTests). For anything else it runs nothing, printsTotal: 0and exits 0, and the gate counted exit 0 as a pass. CI never had this problem:run-darling-pg-shard.ps1resolves the same simple names against the runner's own-list classes/json.Evidence (no suite run):
-class AgAlertEvaluatorTestsgivesexit 0; Total: 0.-class Lite.Tests.AgAlertEvaluatorTestsgivesexit 0; Total: 21, Errors: 0, Failed: 0.[Collection]change) onto a scratch branch and ran the old gate's--dry-run. It gavemap: darling SELECTED 2324,lite SELECTED 616andclasses: darling 2324, lite 616, the same as the report. Every listed name was a simple name (one of them was53).The same dry run shows a second problem. Today the nightly map selects close to the whole suite for any change: 2,324 to 2,325 Darling classes and 614 Lite classes. That held for each of this PR's three files given to
map-selectalone, including the SKILL.md docs file. The reasons were 1,562 "covers a changed file", 374 "new class" and 249 "text pattern". So for now the gate givesGATE STOPon nearly every diff. See "For the coordinator".What changes (by brief item)
Commit aa38d9c, plus a merge of origin/dev (aa4b889). Three files:
.github/scripts/pre-push.sh,.github/scripts/tests/pre-push-test.sh(new) and.claude/skills/lane-orders/SKILL.md.Reproduction and dry run.
--dry-runnow prints each batch's class count and its character length:[classes-darling] batch 1 of 4: 564 classes, 29956 characters of -class arguments. It resolves names against an existing Debug build. Without a build it says the names are not resolved. A newPRE_PUSH_SELECTION=<selection.json>replays a given selection instead of fetching the nightly map. Both the reproduction and the tests use it.Fix.
-list classes/jsonby its simple name, split on[.+/]the waytest-map.py,ci-select.pyandrun-darling-pg-shard.ps1split it. The runner gets full names, nested ones included (Outer+Inner). A name with no test class in the listing is counted on anunlisted:line and not run. These are helper classes, collection definitions and the stray53. A failed class listing makes the gate RED.Total:is 0 or missing makes the gate RED, with one line that names the batch and its class count:[classes-darling-1] batch 1 of 1 (2 classes) ran 0 tests: the filter matched nothing. The Guard stage is covered too.PRE_PUSH_MAX_CLASSES(default 400, the brief's suggested cap), the gate runs no selected class. It ends withGATE STOP: selection is the whole suite (darling N, lite M): run the brief's classes; CI decidesand exit 3. It is never GREEN. The Guard stage still runs first, and a red there wins (GATE RED, exit 1).-classarguments, not by a count of 150. The budget isPRE_PUSH_ARG_BUDGET, default 30,000, the same as CI's shard script. On the reproduction that gives 4 Darling batches of at most 29,980 characters and 1 Lite batch of 28,033.Tests. The repo had no script-test harness, so I added
.github/scripts/tests/pre-push-test.sh. It copies the gate into a throwaway git repository and puts a fakedotnetfirst on PATH. The fake lists the classes it is given, prints the Total it is told to, and records every call. Five cases:GATE RED, exit 1, and the batch line;GATE STOPline, exit 3, no-classrun, and the Guard stage still ran;CI does not run the file. These are behavior tests, not source pins.
Docs. In the lane-orders skill's gate paragraph, I added one sentence for the
ran 0 testsline (→GATE RED) and one for theGATE STOPline (exit 3, over 400 classes in one project). Brief-common's copy of the gate rules lives outside the repo and is unchanged.Test plan
bash .github/scripts/tests/pre-push-test.sh: 15 PASS,pre-push-test: all passed, both before and after the merge of origin/dev.The tests catch a reverted fix. I ran them against three mutant copies of the gate (
PRE_PUSH_TEST_GATE):The gate itself on this branch (
bash .github/scripts/pre-push.sh), result lines:build: both test projects built (Debug)[guard-darling] Darling.Tests Total: 4181, Errors: 0, Failed: 0, Skipped: 0, Not Run: 0[guard-lite] Lite.Tests Total: 1134, Errors: 0, Failed: 0, Skipped: 0, Not Run: 0classes: darling 1955, lite 474 (selected names: darling 2325, lite 614; symbols: 0 candidates)unlisted: darling 370, lite 140 ...GATE STOP: selection is the whole suite (darling 1955, lite 474): run the brief's classes; CI decides(exit 3)The brief expected GATE GREEN for this diff. The map selects the whole suite for this diff, as it did for Lite tests that count log lines no longer lose lines to a parallel class (#5863) #5865, so STOP is the correct verdict now. The brief's classes are the script tests above, and CI decides the rest.
The real gate on a small replayed selection (
PRE_PUSH_SELECTION: 2 Darling classes and 2 Lite classes, real build and runner):[classes-darling-1] Darling.Tests Total: 22, Errors: 0, Failed: 0, Skipped: 1[classes-lite-1] Lite.Tests Total: 31, Errors: 0, Failed: 0GATE GREEN: guard stage of both suites and 2+2 selected classes in 2 batches passedThe reproduction selection replayed through the fixed
--dry-run:classes: darling 1954, lite 476, 4+1 batches under 30,000 characters, thenGATE STOP(exit 3).FleetIdentifierScrubTestsreads the tracked SKILL.md and the new script:Total: 3, Errors: 0, Failed: 0.Full suite: not run (laptop rule). CI decides.
For the coordinator
map-38734757244), even a lone docs file, it selects about 2,325 + 614 classes. So every lane's gate will now sayGATE STOPinstead of the old falseGATE GREEN. CI's own selection uses the same map, so CI runs about the whole suite on these PRs too. That looks like map staleness against a fast-moving dev ("covers a changed file" ×1,562, "new class" ×374). It is in ci-select.py's map logic, outside this lane, and I did not file an issue. It is your call whether it needs one.scratch/5868-repro(the cherry-picked a5b6360, used for the reproduction), in the shared repo. It was never pushed. Delete it withgit branch -D scratch/5868-repro. My rules did not allow me to runbranch -D.Not done
Nothing from the brief's four items. The one difference from the brief: the gate on this diff ends in
GATE STOP, notGATE GREEN, for the reason above.SECTION: Fixed
ENTRY: - The pre-push gate no longer reports GATE GREEN when its selected test classes ran nothing ([#5870]) - developer tooling: the gate passed simple class names to the test runner, which matches only full names, so every map-selected class ran zero tests and the gate still passed; it now resolves names against the build's class listing, fails any run that reports zero tests, cuts batches by command-line length, and stops with GATE STOP when the selection is the whole suite.