Run Darling's whole-tree guards where the area filters do not reach - #3026
Conversation
Darling.Tests takes the repository itself as input in places: the ladder fixture guard derives what it expects from CHANGELOG.md's newest released heading, and the identifier sweep enumerates every tracked file carrying one of nine extensions, .md among them. The darling/core/root filters describe Darling product code, correctly - that is what decides the compile and the publish - so a change confined to markdown or to .github leaves all three false and the suite unrun. Measured over the last 100 pull requests: 92 lit one of the three, 8 lit none, 7 of those 8 changed markdown. Add a job whose gate is the complement of the build job's, read off that job's own step outcome rather than written out again as a second set of paths. A second set is free to fall out of agreement with the first, and enumerated area lists going stale is the shape #2114, #2830 and #2839 each hit in turn. Being the complement it never double-runs the suite, and it needs no upkeep as the filters change. Pin the three pieces of wiring in CrossAppGuardCiGateTests: the step must carry the id, the build job must publish its outcome, and the consuming job must actually invoke the suite. Losing any one leaves a job reporting success having run nothing.
|
Reviewed. This is CI-only ( Design checks out. Walked the complement logic end to end:
Left one inline note on a GitHub Actions edge case worth confirming (job outputs potentially not propagating when the producing No security, correctness, or parity issues found beyond that note. |
…d no outcome A job's declared outputs do not reach a downstream consumer when the producing job's conclusion is failure, so a failing build job can leave needs.build.outputs.darling-tests empty whatever its Darling step actually did. That fell through to the arm that prints "the suite has already run on this commit", which is the dishonest-log failure the job exists to close. Give the empty outcome its own arm: still skip - a red build blocks the merge on its own, and re-running the suite on a commit nothing can merge buys no decision and would cost three minutes on every failing build - but say that the suite's status is not knowable from here and name the log that holds it.
|
Reviewed the One thing worth confirming outside this diff: Minor/non-blocking nit: in the "Decide whether the whole-tree guards run here" step, |
What changed
Darling.Testsis not only Darling's test suite. Some of its guards take the repository itself as input rather than the Darling tree, andbuild.ymlgated all of them on filters that describe Darling product code.Two instances, both derived from the guards' own source.
MigrationUpgradeLadderLiveTests.TheMostRecentRelease_HasALadderFixturederives the ladder fixture it requires fromCHANGELOG.md's newest released heading — its own doc comment says "The release cut moves the heading, and this fails until the fixture beside it exists" — so a release cut committed as a CHANGELOG-only change is precisely the input it exists to fail on, and it is the input no check would run it against.FleetIdentifierScrubTestsenumerates every tracked file carrying one of nine extensions,.mdamong them, so markdown is in its own declared scope.The filters are not wrong; they answer a different question.
darling,coreandrootdecide what to compile and publish, which is what they are for. They are not a description of what a whole-tree guard reads, so the two sets differ. Measured against the last 100 pull requests, replaying each one's file list through this workflow's actual filter patterns: 92 lit one of the three and 8 lit none — 7 of the 8 changed markdown, the eighth changed.github/.So the new job's gate is the complement of the build job's, read off that job's own step rather than restated. The
buildjob now publishessteps.darling-tests.outcomeas an output;darling-tree-guardsruns the suite when it readsskipped. Nothing is enumerated, so nothing can go stale: an enumerated area list stops covering the next tree added, which is the shape #2114, #2830 and #2839 each hit in turn, and a second copy of the gate would drift from the first the same way. It also runs whenever this workflow changes, so a change to the gate is exercised by the gate — the same self-validationdarling-pgtakes.The cost, and the two shapes I did not take
The complement gate never double-runs the suite: 92 of those 100 PRs land on the already-ran side and report here without spinning up .NET at all. On the other 8 it costs one Windows runner for about three minutes, priced off run
33969850863's real step times — 35s of .NET setup, 9s restore, 93s to buildDarling.Tests, 32s for the suite.Not
'**/*.md'on thedarlingfilter. It would retire the#1712docs fast path for the only change class it serves (the fast path engages only when no area is lit, and running the guards needs the restore the fast path skips), and it would makedarlingmean "Darling code, or any markdown anywhere" for the four steps that consume it — including two publishes. Same runner cost, worse signal.Not
'**/*.md'on a new job's own filter. 52 of the last 100 PRs touch markdown and 45 of those already run the suite inbuild, so a markdown filter would double-run it on 45% of PRs — about 2.4 hours of extra Windows runner per 100 PRs on the pool this workflow's own concurrency comment identifies as the thing that serializes everyone's CI.No PostgreSQL in the new job, deliberately. The whole-tree guards need none, and standing up a throwaway TimescaleDB cluster would make a documentation change depend on the flakiest part of CI —
#1862's compression flake failed twice on CI in two unrecognizably different ways and could not be reproduced locally at all.Deliberately unchanged
lite/Lite.Testsfilter asymmetry.darlingreachesLite/**/*.csbut notLite.Tests/**, whilelitereachesDarling/Darling.Tests/**. Untouched: it is a separate question about cross-app reads,EveryCrossAppSourceRead_IsReachableByTheFilterThatGatesItsSuitealready governs it, and nothing here depends on it.buildin ~30s; the guards run in a job of their own.on:block is touched in any workflow, so nothing that can register twice registers differently.CHANGELOG.md. Entry text is below rather than committed.The required-check count goes from six to seven.
Verified
CrossAppGuardCiGateTests.cs— unmodified, so the assertions are the real ones — into anet10.0console harness with xunit.v3 3.2.2, the version this repo pins.build.ymlas it stands ondevfails on the missing step id; removing only the id fails the same way; removing only theoutputs:block fails on the published outcome; deleting the consuming job fails on "nothing reads the build job's Darling.Tests outcome"; pointing the consuming job at a different suite fails on thedotnet runassertion; and changing the gate to key onsuccessinstead ofskippedfails on that comparison. Green with the change in place.Failed: 0,Not Run: 0.FleetIdentifierScrubTestspasses on this tree and is not vacuous here: run through the same harness it reportsFailed: 0, and planting a non-allowlisted slug with a padded ordinal into a markdown file underdocs/turns both of its facts red, naming the file and line; removing it returns both to green. So the guard does reach markdown, which is what makes the gate matter.jobsresolves tobuild,darling-pg,darling-linux,darling-tree-guardswith the new job'sneeds,if,runs-on,timeout-minutesandpermissionsas written, andid: darling-testslanding on theRun Darling testsstep and no other.ViewerSidebarDotRendersTheCardStatusTests.TheGuard_RunsOnEveryTreeItScansandCrossAppGuardCiGateTests.Checkboth locate- name: Run Darling testsbyIndexOfand read forward a bounded window; the new job is appended after the build job and its step is namedRun the whole-tree guards, so neither can latch onto it.CiClusterWorkerSizingTestsparses onlyAdd-Content ... -Value "setting = value"lines, and the diff adds none.build.ymlandCrossAppGuardCiGateTests.csare both CRLF with no BOM ondevand stay that way in the working tree; both blobs are LF after staging, 0 CRLF, which is what* text=auto eol=crlfshould produce.git checkout, with the fixed file's md5 re-asserted after every one of the six cases; the working tree matched the commit afterwards.Review findings
Taken, in
3529495. Theelsearm printed "the suite has already run on this commit" for an EMPTYneeds.build.outputs.darling-testsas well as forsuccess/failure. A job's declared outputs do not reach a downstream consumer when the producing job's conclusion isfailure, so abuildthat fails — atLite.Tests, before reaching the Darling step, or at the Darling step itself — can leave that read empty whatever the step actually did, and the notice would then assert something it cannot know. That is the dishonest-log failure this job exists to close, arriving in the job's own log. The empty outcome now has its own arm: still skip, because a redbuildblocks the merge on its own and re-running the suite on a commit nothing can merge buys no decision while costing three minutes on every failing build — but it says the status is not knowable from here and names the log that holds it.Declined, in the second pass: passing
github.event_namethroughenv:for consistency. Theenv:indirection in this step carries the two filter-derived values, and that is the pattern the workflow already uses —ALL_COUNT,DOCS_COUNTandAREASin the docs fast path,DARLING_TESTSandWORKFLOWhere.github.event_nameis interpolated directly into bash in every gate-decision step in this file: lines 255 and 261 in the fast-path classifier, 690 in the Darling PG gate, 865 in the Linux gate, and 1035 here. So this step already matches the house pattern on both halves; routingevent_namethroughenv:would make it the only one that does.Raised as a checklist item, not a code change: the required-status-checks list. See "Action needed outside this diff" above — it is real, it is confirmed against the API, and it cannot be expressed in this diff.
Fixing the wording also retires the suggestion to confirm the propagation behaviour with a deliberately broken
build: both branches are now correct under either behaviour, so the question stops being load-bearing.All four arms exercised directly, by extracting the gate script from the parsed workflow and running it under each input:
release→ skip;skipped→ run;workflowfilter true → run; empty → the new not-knowable arm;successandfailure→ skip with the outcome named.bash -nclean on the extracted script.After, observed on
3529495Darling whole-tree guardsran and did the work: 2m39s wall, of which .NET setup 29s, restore 8s,Build Darling.Tests77s, andRun the whole-tree guards30s reportingDarling.Tests Total: 7546, Errors: 0, Failed: 0, Skipped: 313, Not Run: 0. A no-op report on this job costs ~15s, so the duration alone separates the two, and the test count settles it.Which arm fired is in the log, and it is the one predicted:
DARLING_TESTS: success,WORKFLOW: true,##[notice]build.yml changed, so this job's own gate is exercised by the gate.The complement arm could not fire here —buildran the suite on this PR becausebuild.ymlis in therootfilter — which is the limit recorded below.needs: buildsequenced as intended:buildcompleted 14:41:22, this job started 14:41:30.Incidental confirmation of
if: ${{ !cancelled() }}: on the superseded first run this job reportedcancelledwithstarted_at == completed_at, so it did not take a runner for a run that no longer mattered.Action needed outside this diff
dev's branch protection currently requires exactly two contexts —buildandDarling PostgreSQL tests.Darling whole-tree guardstherefore reports a result but does not yet block a merge, and neither doDarling Linux build,check-branchesorverify. That is a repo Settings change, not something a workflow file can express (there is no.github/settings.ymlor rulesets-as-code here), so it will not happen as a side effect of merging this:Worth noting alongside it: on a markdown-only change both currently-required contexts go green having executed nothing, so until this job is added the required set is satisfied by two vacuous passes.
Before, measured
Markdown-only PRs on this workflow today, by duration and by step conclusion rather than by the check mark:
CHANGELOG.mdonly,4344fcf)CHANGELOG.mdonly,fa7fd5f)33969850863)buildDarling PostgreSQL testsDarling Linux buildOn #2984's
buildjob, all 35 build / test / publish steps reportskipped; four steps ran — job setup, checkout,Detect changed paths, andClassify change for the docs fast path.Run Darling testsis one of the 35. Thatskippedis the exact value the new gate keys on, so the arm is reading a value whose real-world setting on a markdown-only diff is measured rather than assumed.Not verified
DARLING_TESTS: success. A PR that introduces this job necessarily changesbuild.yml, which is in therootfilter, sobuildrunsDarling.Testson the very commit that adds the job and publishessuccess, neverskipped. What is observed instead is the job's mechanics end to end via the self-validation arm, plus the arm's input in two measured halves:buildpublishes that step's outcome (seen here assuccess), and Record #2975's entries, and correct a claim its measurements disproved #2984's run shows the same step's conclusion isskippedon a markdown-only diff. One observation short of end-to-end, and I am not going to call it end-to-end.needs: buildadds the new job's own startup to the workflow's wall clock on the 92% of PRs where it reports without working. This run measured the pre-gate steps at 9s (job setup 1s, checkout 7s, filter 0s, gate 1s), so ~10s plus queue time — but a run where the gate says skip has not been observed, because this PR's gate says run.Whether GitHub's expression parser accepts a hyphenated job-output name.Settled by the run:needs.build.outputs.darling-testsresolved tosuccessin the job'senv:block.CHANGELOG entry, not committed — for the coordinator to place