Repository navigation
A guard miss fails the pull request in about 6 minutes and skips every shard - #5471
Merged
Merged
Conversation
… about ten minutes (#5459) A new Guard tests job runs the Stage=Guard classes of both test suites before any shard. On a pull request the Darling PG shards, Lite shards, Linux build and whole-tree guards start only after it succeeds, the build job fails at once, and the two result jobs report the failure; a skipped matrix is now an error instead of a pass. About 140 existing guard classes carry the trait, and a census per suite keeps new ones honest. Part of #5459.
Part of #5459. Reverted by the next commit.
erikdarlingdata
marked this pull request as ready for review
October 7, 2026 14:41
…fails skips, and its loop and header are corrected (#5459) Review round 1 of #5471. M1: both censuses compare the source scan with a reflection read of the test assembly (class, method, constant, inherited and linked-file traits), apply the no-store rule to that set, and the scan now reads files a csproj links in. The guard job runs both suites with -failSkips. L1: the guard loop sets the native-command preference explicitly. L2: the darling-tree-guards header no longer promises a result on every event.
erikdarlingdata
marked this pull request as draft
October 7, 2026 15:42
erikdarlingdata
marked this pull request as ready for review
October 7, 2026 15:42
… and the workflow pins are anchored (#5459) The census reflection now reads exported types, static classes, and static and non-public test methods, as xunit v3 does, with probes for a static method, a private method and a static class. The workflow pins match an uncommented PSNativeCommandUseErrorActionPreference line and the whole -failSkips switch.
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.
Fixes nothing alone. This is change 1 of #5459 (guard classes first, fail fast).
What a PR author sees now
A pull request that breaks a guard (a stacked doc comment, an untagged census class, a bad pin) now fails in about 10 minutes instead of after the whole run. A new job, "Guard tests", runs first and runs only the classes tagged
[Trait("Stage", "Guard")]in both test suites. While it runs, the six Darling PG shards, the four Lite shards, the Linux build and the whole-tree guards wait. If it fails, they are skipped,buildfails on its second step, and the two result checks fail with "Guard tests failed". Before this, one stacked summary left every shard running to the end.The results of push, merge queue and release runs are unchanged, but those runs now wait for the guard job before the shards start (see the L3 note below). They run everything whatever the guards say (dev pushes stay the safety net and the Lite timing source).
Required checks and how each still reports
Exact names from the branch protection list:
build,Darling PostgreSQL tests,Lite tests (result),Claude review guard.build: needs[gate, guard-tests], stillif: !cancelled(). New second step "Fail when the guard tests failed" (pull request only, before checkout) fails with an::errornaming the Guard tests job. When it fires, runs-on picks ubuntu-latest, so no Windows runner starts. A release runs as before (the guard job is skipped there).Darling PostgreSQL testsandLite tests (result): needs[gate, guard-tests, <matrix>]. Arms in order: gate failed, declined (draft or merge-train, unchanged text), then new "Guard tests failed" (not success, exceptskippedon a release), then the matrix result.skippedmatrix arm used to print a notice and exit 0 (a skipped matrix read as a pass). It is now an error in both result jobs.Claude review guardis another workflow and is untouched.By-design skips stay green. I listed every
if:on the shard jobs. The shards are skipped on purpose only when: the gate declined the run (draft or merge-train label; the declined arm reports that as a failure on a pull request as it always did), the guards failed on a pull request (new, an error), and for the Lite shards a release (the Lite result job has its ownif:that excludes a release, so it never reads that skip).darling-pganddarling-linuxhave no other skip reason; their path filters skip steps, not the job. So no by-design skip reaches the new error arm.GuardStageWorkflowTests.TheShardJobs_AreSkippedOnlyForThePlannedReasonspins the exactif:of each shard, so a new skip cause is a deliberate edit.Docs-only pull requests: all four required checks still report success. The proof is the YAML pins: the guard job's filter is a pinned copy of build's
docsandallentries and every step after its decision step is gated onrun == 'true', so it succeeds having built nothing (ADocsOnlyChange_LeavesTheGuardJobGreenHavingBuiltNothing); the shards' own filters andbuild's docs fast path are unchanged; and the guard arm acceptssuccess. No separate docs-only proof run was done.Merge-ref note
pull_request runs use build.yml from the merge ref (the head merged into dev). Every open PR's next NEW run (a push or a ready event, not a re-run of an old run) picks wave 1 up, and the census then flags any untagged guard-style class that PR adds.
What changes
.github/workflows/build.yml: theguard-testsjob;needs/ifonbuild,darling-pg,darling-linux,lite-tests,darling-tree-guards(same fail-fast, so it does not start a runner) and both result jobs; the newbuildstep;GuardStageScanner.csadded to thelite_linked_shardfilter.Darling/Darling.Tests/GuardStageScanner.cs(new, linked into Lite.Tests): reads each test class's source: name, trait,live-postgrescollection, recursive repo-root enumeration,DARLING_TEST_variable reads and store helpers.GuardStageCensusTestsin each suite (rules R1 name, R2 repo-root walker, R3 no store; a pinned exemption list) andGuardStageWorkflowTests(R4 workflow pins).[Trait("Stage", "Guard")]on 99 Darling.Tests classes and 39 Lite.Tests classes (script-tagged, one line each, reviewed bygit diff --statand spot checks). Guard-trait class counts from-list classes/json -trait Stage=Guard: Darling.Tests 102, Lite.Tests 40 (each includes the new census).LockedModeRestoreCoverageTestsnow picks the build job's restore step by name ("Restore dependencies") instead of by "the only step withset -euo pipefail", because the guard job has a second one.RepoFileAdoptionTestslists the new workflow pin as an LF reader.Exemption list (Darling.Tests, 6; Lite.Tests, 0; size pinned, may only shrink):
AlertDeliveryChannelTests(walks the repo but also reaches the shared store through a helper)DarlingEmptyEnumerationInventoryTests(live-postgres collection, readsDARLING_TEST_PG)DarlingOwnedSecretsCensusTests(sets and reads its ownDARLING_TEST_OIDC_SECRET_OWNEDSET)DarlingPgRuntimeVersionPinTests(gated onDARLING_TEST_PGRUNTIME)StoreApplicationNamePinTests(readsDARLING_TEST_PGfor its scratch database)WebExceptionTextCensusTests(live-postgres collection, readsDARLING_TEST_PG)Seat tools (plan ruling 8): the seat's
(Darling|Lite)\.Tests +Total:grep will also match the Guard tests job's two subset lines; read the job name with each. Shard totals do not change in W1.Why the shards wait for the guard job
The shards list Guard tests in
needs:. We weighed running the guards next to the shards and cancelling the run when a guard fails, and keptneeds:for three reasons.needs:, a guard failure costs about 6 runner-minutes in all (run b: 6.18). With a cancel, each shard has already run for about 6 minutes when the guards fail. That run costs about 60 runner-minutes.actions: writepermission. It can also stop a required result check from ever being created, unless every result job usesalways(). In that case the pull request waits on a check that never reports.Guard job runtime
Locally (Debug, one machine): Darling.Tests guard set 1282 tests in about 36 s, Lite.Tests guard set 364 tests in about 22 s. On the proof run the job took 5 min 53 s end to end (14:42:06 to 14:47:59): Darling.Tests tests about 35 s, Lite.Tests about 14 s, the rest setup, locked restore and two Release builds.
Cost
Measured on the proof run of this change: 154.5 runner-minutes and 30 minutes wall. A comparable full run without the guard job on the same day took 148.6 runner-minutes and 21 minutes. That is +6 runner-minutes (+4%) and +9 minutes wall. Change 2 of #5459 makes the guard job the one build every shard reuses, which removes most of the wall-clock cost.
Test plan
-trait Stage=GuardDarling.Tests: Total 1282, Failed 0. Lite.Tests: Total 364, Failed 0.GuardStageCensusTestsDarling: Total 5, Failed 0. Lite: Total 5, Failed 0.GuardStageWorkflowTestsTotal 7, Failed 0; reverting one shardif:madeTheShardJobs_AreSkippedOnlyForThePlannedReasonsfail (mutation proof). The census failed with 99 MISSING lines before tagging.*Census*Darling.Tests: Total 445, Failed 0, Skipped 1.*Ratchet*: Total 15, Failed 0.CrossAppGuardCiGateTests.EveryCrossAppSourceRead_IsReachableByTheFilterThatGatesItsSuitefailed when a Darling.Tests pin named the Lite project path; the pin matches the project file name without its folder instead.needs/ifprinted and checked.Proof runs
Run a (the draft run, declined by the gate):
build,Darling PostgreSQL testsandLite tests (result)red with "CI not run" within 6 s; Guard tests, Darling PG tests, Lite tests, Darling Linux build and the whole-tree guards skipped.Run b (marked ready, with one planted stacked
<summary>in a Darling test file): started 14:41:54.DocCommentHygieneTests.NoMemberCarriesTwoStackedSummaryBlocksfailed (Darling.Tests Total 1282, Failed 1) and Lite.Tests Total 364, Failed 0. Listed classes: 102 Darling.Tests, 40 Lite.Tests.build: failed 14:48:09 on its new second step "Fail when the guard tests failed" (6 min 15 s after run start).Darling PostgreSQL testsfailed 14:48:07 andLite tests (result)failed 14:48:05 (about 6 min 10 s after run start), both with "Guard tests failed".Run c (the revert commit, pushed after run b finished, no force-push) is for the seat to watch: expect green, about +7 runner-minutes and +4-5 min wall against the 109 median.
Review round 1
GuardStageCensusTestsnow also read the tagged set off the loaded test assembly with reflection. It sees class, base class and method traits, and trait names spelled with constants, on public concrete classes with a test. A class xunit selects that the source scan does not see as tagged fails. So does a class the scan reads as tagged that xunit does not select. Rule R3 (no store) runs over the reflected set too, including thelive-postgrescollection. The scan now also reads the files a csproj compiles in through aLink, which is what hidCrossSkuSurfaceSourceTestsfrom the Darling census. Nothing was loosened and no exemption was added. Before the scanner change the new check failed on exactlyCrossSkuSurfaceSourceTests(Darling.Tests, 1 of 9 failed). After it, 9 of 9 pass in each suite. Detector tests pin each shape (method-level trait, constants, inherited trait, live collection, phantom, unscanned) with private probe classes xunit does not run.-failSkips(spelling checked against the runner's help). A tagged test that skips for lack of a store now turns the job red. Both suites skip 0 in the guard set today (Darling.Tests 1288 run, Lite.Tests 368 run, all passing with the switch).GuardStageWorkflowTestspins the switch on the single test-running line.$PSNativeCommandUseErrorActionPreference = $falseexplicitly, like the three other loops, with a comment. Pinned (set after$ErrorActionPreference, before the loop).Review round 1 test totals (Total / Failed):
-trait Stage=Guard -failSkips: Darling.Tests 1288/0, Lite.Tests 368/0.*Census*class in Darling.Tests 449/0 (1 skipped, a live class with no store here). Every*Ratchet*class 15/0.SECTION: None
ENTRY: - CI only ([#5471]) - no user-visible change.
Part of #5459.
Review round 2
GuardStageScanner.ReflectGuardClassesnow takes types fromGetExportedTypes(). It accepts a class that is not abstract or is sealed, so a static class counts. It binds test methods as Instance, Static, Public, NonPublic with FlattenHierarchy. That is xunit v3's own discovery. Each suite's probe test got three new probes. One is a static method with a method-levelStage=Guardtrait. One is a private method with the same trait. One is a static class with a class-level trait. Before the fix the probe test failed on the static method ("a static test method with a method-level trait is not seen"). With only the method binding fixed it failed on the static class ("a static class is not seen"). After both fixes it passes.GuardStageWorkflowTests, the$PSNativeCommandUseErrorActionPreference = $falsepin is now a multiline regex that needs an uncommented statement on a line of its own. The switch pin is a regex that needs-failSkipsfollowed by whitespace or the end of the line. I planted two bad lines in build.yml and then restored it. A commented-out preference line failed the pin with the new message.-failSkips-failed it too. The unchanged file passes.origin/devfirst. It had no guard file or workflow conflicts.Review round 2 test totals (Total / Failed):
-trait Stage=Guard -failSkips: Darling.Tests 1288/0, Lite.Tests 368/0.*Census*class 449/0 (1 skipped: a live class that needs a store).-failSkipscounts that skip as a failure, so these classes ran without the switch,*Ratchet*15/0,*Hygiene*90/0.*Census*55/0. There is no*Ratchet*or*Hygiene*class in Lite.Tests (Total 0).Dev sync
Merged dev; tagged the guard classes dev added since the base. The census named four:
ProcedureTrendHourlyQueryViewPinTests,ProcedureTrendQueryViewPinTests,ScratchPostgresQuiesceCensusTests(Darling.Tests) andProcedureTrendQueryPinTests(Lite.Tests). None needs a store, so each got[Trait("Stage", "Guard")]and none was exempted. Before the tags,GuardStageCensusTestsfailed 1 of 9 in each suite; after, 9/0 in each. A second merge of dev brought in no new class.Totals after the final merge (Total / Failed):
-trait Stage=Guard -failSkipsDarling.Tests 1303/0, Lite.Tests 372/0.GuardStageWorkflowTests9/0.ScratchPostgresQuiesceCensusTests4/0.*Census*Darling.Tests 453/0 (1 skipped live class), Lite.Tests 55/0.*Ratchet*15/0,*Hygiene*90/0 in Darling.Tests. Both test projects build with 0 warnings. The.githubdiff against dev is only this PR'sbuild.ymlchange, and dev's worker-crash warning step is still in both workflow files.