Coverage collection is unreliable, and blocks the coverlet 10 upgrade #14

Activity

1. iancooper

   added

   bugSomething isn't workingSomething isn't working

   on Aug 29, 2026
2. iancooper commented on Sep 6, 2026

   

   iancooper

   on Sep 6, 2026

   MemberAuthor

   More actions

   Correction: coverlet 10 does not count more branches

   Symptom 2 is stated in this issue as "a real behavioural change: coverlet 10 identifies branches the
   6.x line did not". That is wrong, and the difference matters for how we decide what to do.

   branches-valid is unchanged in every comparison:

   	Specs 6.0.4	Specs 10.0.1	Core.Tests 6.0.4	Core.Tests 10.0.1
   lines-valid / covered	4204 / 3983	4204 / 3983	2426 / 2426	2426 / 2426
   branches-valid	1499	1499	553	553
   branches-covered	1425	1409	553	548

   The per-condition IL offsets are unchanged too — AdvancedCircuitBreakerTResultSyntax.cs:33 reports
   jump offsets 12 and 43 under both versions; only their coverage moves, 100% → 50%. Coverlet 10
   finds the same branches. What changed is hit attribution: 6.0.4 credited the "taken" leg of a
   two-way branch whenever the branch instruction was reached, regardless of which leg actually ran.
   Coverlet 10 instruments both legs (coverlet PR
   #1865, "Investigate and fix branch
   coverage").

   6.0.4 is demonstrably wrong, and one line proves it.
   src/Paramore.Fences.Core/Utils/Pipeline/DelegatingComponent.cs:64 carries hits="1" in both
   reports — the line executed exactly once — yet 6.0.4 reports its two-way branch as 100% (2/2). One
   execution cannot take both legs. Coverlet 10's 50% (1/2) is correct.

   So our current branch-coverage numbers are inflated. The true figures are:

   Project	Reported under 6.0.4	Actual
   Paramore.Fences.Specs	95.06%	93.99%
   Paramore.Fences.Core.Tests	100%	99.09%
   Paramore.Fences.Extensions.Tests	100%	96.52%

   Note the third row: Extensions.Tests fails under coverlet 10 as well. #6 aborting at Specs hid
   that, so the blast radius is wider than this issue records. Testing.Tests still passes at
   100 / 100 / 100.

   What the 19 changed sites actually are

   Reproduced locally by copying the repo and flipping only Directory.Packages.props; 8/8 identical
   runs, so this is not symptom-1 noise.

   18 of 19 are compiler-generated delegate-cache checks — the <>c.<>9__x ?? (<>c.<>9__x = new …)
   null-check the C# compiler emits when it caches a lambda or method group. The uncovered leg is the
   cache-hit path, reachable only on a second invocation within the same generic instantiation. Two of
   them (Simmy/Utils/GeneratorHelper.cs:24, Simmy/Fault/FaultGenerator.cs:65) show hits="2", but
   each hit is a different generic instantiation with its own static cache field — so both are cache
   misses, and coverlet 10's 1/2 is again right. Covering these would mean writing tests whose only
   purpose is to call a method twice.

   The remaining one is a genuine branch: src/Paramore.Fences/CircuitBreaker/RollingHealthMetrics.cs:79,
   the short-circuit leg of a while (_windows.Count > 0 && …). Line 78 already carries
   // stryker disable once all : no means to test this — we have already judged that path untestable.

   What this means for the acceptance criteria

   "No threshold is lowered merely to accommodate a collection defect" — adjusting the thresholds here
   does not violate that. This is not a collection defect and not a loss of coverage; it is the discovery
   that the previous numbers were over-credited by a tool bug. The tests cover exactly what they covered
   before. Correcting an inflated baseline is the opposite of lowering a gate to hide a problem.

   On that reading, #6 can be taken, with the thresholds set to the true figures and
   Extensions.Tests included in the change.

   One correction on symptom 1's suggested remedy

   The "worth investigating" list proposes coverlet.collector (--collect:"XPlat Code Coverage") as
   the more robust path, on the theory that the datacollector is flushed through the datacollector
   protocol before the host exits. That theory doesn't hold. Coverlet issue
   #1983 — a full RCA of this exact
   EndOfStreamException — reproduces it using coverlet.collector 10.0.1 with
   --collect:"XPlat Code Coverage" on a multi-targeted net8/9/10 project, and states that
   ModuleTrackerTemplate.cs and the whole data-collection hot path are byte-for-byte identical between
   the two integrations. The datacollector adds an in-proc collector that usually flushes earlier, which
   narrows the window materially — but it is a mitigation, not a fix, and it also introduces an
   out-of-process reader that can open the hits file mid-write.

   The confirmed mechanism is that coverlet's hits file is flushed only by an AppDomain.ProcessExit
   hook, written non-atomically, while vstest allows the test host 100 ms to shut down before calling
   TerminateProcess. Upstream measured that flush at 988 ms for a single assembly. Two cheaper levers
   than the version swap: raise VSTEST_TESTHOST_SHUTDOWN_TIMEOUT, and make the failure visible at all —
   coverlet's Hits file … not found is logged at verbose, which is why the 0% run
   (32841065953) produced no
   diagnostic whatsoever.

   Still open

   The 99.95% run (33262613610)
   is not the same failure as the 0% and stream-exception runs, and should not be assumed closed by
   fixing them. The hits file is a length-prefixed array read in a tight loop — any short read throws, so
   there is no code path yielding a valid-but-partial result. The log shows coverlet read that file
   cleanly, no warning, and then reported 99.95%. The arithmetic (2425/2426, 552/553) says exactly one
   line and one branch went missing. That remains unexplained.
3. iancooper commented on Sep 25, 2026

   

   iancooper

   on Sep 25, 2026

   MemberAuthor

   More actions

   Upstream Polly has dropped coverlet — see #34

   The first upstream sync (#28) surfaced App-vNext/Polly@1a80392b, "Update to xunit v3" (App-vNext/Polly#3131). It is being ported as its own issue, #34. It bears directly on this one:

   • coverlet is removed entirely. coverlet.msbuild and the ReportGenerator package are gone. Coverage now comes from Microsoft.Testing.Extensions.CodeCoverage, running under Microsoft.Testing.Platform v2, not vstest.
   • Each target framework writes its own coverage file (artifacts/coverage/<project>/coverage.<tfm>.xml). That rules out, by construction, one of the "worth investigating" threads above: target frameworks racing over a shared output path.
   • Thresholds survive but move. Projects keep <Threshold>, but enforcement moves out of eng/Test.targets into a cake.cs task (__VerifyCoverageThresholds) that checks each per-framework file.
   • Upstream re-baselined its numbers under the new tool. Polly.Specs went from 94,94,91 to 96,95,93: up, not down. That is a different counter again, so the "true figures" table in the correction above (measured under coverlet 10) won't carry over as-is.

   What this means here

   • The hits-file / ProcessExit / 100 ms vstest shutdown mechanism diagnosed above is specific to coverlet under vstest. If Port upstream xunit v3 / Microsoft.Testing.Platform v2 migration (App-vNext/Polly@1a80392b) #34 lands, symptom 1 as described here should no longer be reachable. That's not the same as confirming the new tool is reliable. The acceptance criterion "coverage figures reproducible across all three target frameworks" still has to be demonstrated under it.
   • Bump coverlet.msbuild from 6.0.4 to 10.0.1 #6 (coverlet 10.0.1) would be superseded rather than merged. Worth deciding before spending more effort on coverlet-specific mitigations such as VSTEST_TESTHOST_SHUTDOWN_TIMEOUT.
   • The coverage-policy question stays open whichever tool we use. Re-baselining thresholds under a new counter should follow this issue's rule: no threshold lowered to hide a collection defect, and each change justified by the tool's counting, the way the coverlet 10 correction above was.
   • The unexplained 99.95% run can't be investigated further once coverlet is gone. If it matters, it needs looking at before Port upstream xunit v3 / Microsoft.Testing.Platform v2 migration (App-vNext/Polly@1a80392b) #34 lands.

   Suggest #14 and #34 are decided together: #34 changes what #14 is about.