Repository navigation
Fixes #3067 - #3075
Fixes #3067#3075
Conversation
…tree-guards backstop covers CrossAppGuardCiGateTests anchored every cross-app path on the app directory, so Darling.Tests' four reads of Lite.Tests were invisible: Lite.Tests is a sibling of Lite, not a child. The anchor is now every root a SKU owns, on both populations, and each arm is floored on finding at least one reference under the other SKU's test tree. The four references it now sees are not filter-reachable and are exempted rather than named in the darling filter: that filter decides what gets compiled and published, and darling-tree-guards already runs the whole Darling suite on exactly the changes in question. The exemptions carry their bound and are honoured only while that backstop is verifiably intact.
ReviewScope: this PR touches only CI test infrastructure ( Traced through the new pieces by hand against the current repo state:
No correctness bugs, no Lite/Darling parity drift, and no security concerns found (pure repo-local file/text scanning, no external input). The PR's own "red-proof" mutation-testing table is thorough and I didn't find a gap it missed — in particular the documented limitation (bare directory names with no separator, e.g. in Nothing blocking from this pass. |
#3075 rewrote CommentFilterAdoptionTests' CLASS summary (the build.yml filter paragraph). This branch edits s_bounded's own doc comment and the map below it, so the two do not overlap and every count derived here is unaffected: 4 COLLECTS + 1 stated bound + 1 SQL + 1 demonstrator = 7 keys, re-counted after the merge rather than assumed.
CrossAppGuardCiGateTestsanchored every cross-app path on the app directory, andLite.Testsis a sibling ofLiterather than a child. SoDarling.Tests' reads of Lite's test project matched nothing: the arm opened 454 C# files, found 15 references, and not one of them came from the other SKU's test tree. Nothing distinguished that from health.The issue names two such reads. There are four, all confirmed against a fresh
dev(987c005):Lite.Tests/QueryHighDopStaleMaxDopParityTests.csDarling/Darling.Tests/QueryHighDopStaleMaxDopParityTests.cs:479Lite.Tests/ParameterSensitivityFiringSignatureParityTests.csDarling/Darling.Tests/ParameterSensitivityFiringSignatureParityTests.cs:336Lite.Tests/AnalysisPassTokenThreadingTests.csDarling/Darling.Tests/CommentFilterAdoptionTests.cs:99Lite.Tests/LiteSidebarDotRendersTheCardStatusTests.csDarling/Darling.Tests/CommentFilterAdoptionTests.cs:106The first two are the
twinconstants the issue describes. The other two are keys ofCommentFilterAdoptionTests' bounded set —<project>/<filename>labels which, for a file sitting directly underLite.Tests, happen to spell a real repo-rooted path. That class does open both files, so requiring reachability of them is not a false positive; it is the strongest instance, because it enumerates the whole tree.The reverse arm is genuinely fine, and that is measured rather than reasoned: with
otherApp=Darling,Darling/Darling.Testssits insideDarling/, and the Lite arm finds 7 references under it (including #3059's linked compile) — all reachable via thelitefilter'sDarling/Darling.Tests/**/!(*.md)entry. Widening that arm's anchor to the SKU pair leaves its found set byte-identical at 22 references, because the second root is subsumed by the first.The exit taken: (c)
The anchor now matches every root a SKU owns, on both populations (the C# matchers and
RepoRooted), derived from the two arms' own constants rather than spelled a third time. The four references it now sees are not reachable bydarling, and they are exempted with a stated bound rather than named in a filter.build.ymlis not touched by this PR at all, so the inline prohibition at lines 48-52 is not engaged.The measurement, and a premise that does not hold
The issue frames the (a)-vs-(c) decision as "how often would a
Lite.Tests-only change be forced to stand up thedarling-pgcluster". The answer is zero, and it cannot be otherwise:darling-pgdeclares its owndarlingfilter (build.yml:672-678) naming onlyDarling/**/!(*.md),.github/workflows/build.ymland.github/workflows/nightly.yml.darling-linuxdoes the same (build.yml:853-859). There are three independentdarling:filter blocks in that file with three different pattern sets, and the one the guard reads — the first, at line 162 — gates only thebuildjob's steps. No entry added there can start a TimescaleDB cluster. That claim is also written intoCommentFilterAdoptionTests' doc comment as the reason #3059 declined the entry; this PR corrects it.So here is the countable version, over the 100 pull requests merged to
devbefore this branch (#2919 through #3073, file lists from the API):Lite.Tests/**at allLite.Tests/**withoutDarling/**darlingviaLite/**/*.cs,coreorroot; 1 ran it indarling-tree-guards)Lite.Tests/**/!(*.md)to the build job'sdarlingfilterdarling-pgcluster stands up todaydarling-pgafter addingLite.Tests/**to the build job's filter6% is not the large fraction the issue's decision rule was looking for, so cost is not what decides this. Coverage is: (a) buys zero additional guard execution. All 6 of those PRs already ran both twin guards. Adding the entry would change only where the suite ran, for 1 PR in 100, and would pay for it by making a Lite test-file edit compile the Darling Viewer and publish both Darling artifacts — the build job's
darlinggate also controlsBuild Darling(line 315),Publish Darling Service(377) andPublish Darling Viewer(381). Changing what CI compiles and publishes in order to satisfy a test asserting a property CI already satisfies by another means is the wrong direction; the test is what should learn about the other means.Against (b). A blanket exemption is defensible on the facts —
darling-tree-guardsruns the fullDarling.Testssuite whenever thedarlingfilter did not fire, so filter-reachability of anythingDarling.Testsreads is unconditionally redundant, not just these four. What sinks it is that the redundancy rests on an unpinned property.TheDarlingSuite_RunsWhereTheAreaFiltersDoNotReachasserted the invocation as a substring, which a-classfilter appended to it would still satisfy. Under (b) the arm would be gone, that narrowing would be silent, and nothing would notice. (c) keeps the arm live for the next reference and turns the bound into something checked.Against (a) beyond the numbers: two of the four references are whole-tree enumerations, and the only filter entry that reaches a tree enumeration is
Lite.Tests/**— the widest possible one, for the categorybuild.yml's own comment (lines 965-980) assigns todarling-tree-guards. Of the two mechanisms that file uses for markdown reachability, this is thedarling-side one: exclude and lean on the backstop, not thelite-side one of naming the file because a pin parses it.What changed
SkuTrees— one SKU as the named pair of trees it owns. Named rather than positional because two different things are read off it and reading the wrong one is silent both ways.Rootsis sorted longest-first; that changes no answer today (a required separator already rejectsLite.Tests/X.csfor theLiteroot, and .NET's alternation backtracks) and exists so a rewrite to a non-backtracking matcher cannot silently start letting the shorter root absorb the token under test.CSharpPaths(text, other)— the C# stage extracted fromScanso the pin is fed the shipped matcher, the wayMsBuildPathsalready is.RepoRooted— the same widening on the MSBuild side, which had the identical gap: a linked compile of..\..\Lite.Tests\X.csout ofDarling/Darling.Testsnormalised correctly and was then thrown away.DarlingTestsBackstopped— the four entries, each with its bound, compared for set equality against what the filter could not reach, and honoured only whileWholeTreeBackstopIsIntact(yaml)holds.TheDarlingSuite_RunsWhereTheAreaFiltersDoNotReach— the run assertion is now whole-line equality, and it also asserts the predicate the exemptions are gated on agrees, so the two cannot drift.CommentFilterAdoptionTests' doc comment — corrected: thedarling-pgclaim, the job's name (darling-tree-guards, notwhole-tree-guards), a frozen file count removed, and the note thatCrossAppGuardCiGateTests"cannot see this reference either way", which is no longer true.Red-proof
Run on macOS by compiling the actual
.csunder test into a throwaway net10.0 xunit v3 harness staged inside the gitignoredLite.Tests/bin/. Every run deletes the dll, builds, asserts the build succeeded and the dll was recreated as a step distinct from running, prints its mtime, then runs — so a failed build cannot leave a stale binary printing a clean summary. Baseline before the change:Total: 9, Failed: 0, Not Run: 0. After:Total: 10(the new pin, visible as a counted delta),Failed: 0, Not Run: 0, Skipped: 0.Mutations were applied only after committing, and every run verifies the tree restored clean and that the mutation actually changed a file.
Roots = { appDir }— revert the anchornullLite.Tests/Fixtures/SystemHealth→Lite.Tests), plus a newly unreachableLite.Tests (directory)AppDirinstead ofTestsDir, alonetwinconstant inDarling.TestssourceLite.Tests/…read to aDarling.Testsfilebackstopped: null-- -class "…"to the tree-guards run lineTrueand the new whole-line formFalse— the cheapest way that pin passed while the thing it guards was broken, made redRepoRootedroot check →AppDironlyRepoRootedroot check →StartsWith(root), no separatorLite.TestsExtra/Nope.csno longer nullDarling.Testscannot run on macOS, so the two guards there that readLite.Testswere compiled into a second harness and run:CommentFilterAdoptionTests+DocCommentHygieneTests,Total: 42, Failed: 0, Not Run: 0. That harness was itself red-proofed — inserting a.StartsWith("//")site intoLite.Tests/CrossAppGuardCiGateTests.csmadeCommentFilterAdoptionTestsfail naming exactly that file, so the clean run is a real pass rather than a sweep that read the wrong root.Not fixed here
Darling/Darling.Tests/ControlPlaneReloadDurabilityTests.cs:346enumeratesLite,Lite.TestsandPerformanceMonitor.Commonas bare directory names with no separator. No anchor keyed on a root plus a separator can see those, before or after this change, and that is symmetric — the same shape hides a"Darling"read from the Lite arm. It is a third spelling of an invisible cross-app read, distinct from #3063's population and from this anchor, and widening to bare tokens would pull in every prose mention ofLiteandDarlingin the tree. The new known-answer pin records the limitation explicitly (bareDirectoryis in the fixture and absent from the expected set) rather than leaving it implied. Filed as #3076.Fixes #3067