Refuse a Debug-measured coverage report in the ratchet #206

rob · 2026-08-21T05:59:20Z

rob commented

2026-08-21 05:59:20 +00:00

Closes task 269.

check-coverage-ratchet.cs now takes ReportGenerator's merged Cobertura report (new Cobertura reporttype in CI) as a required third argument, and refuses to run — exit 3, before any comparison — unless the generated-file paths in it name Release. Detection reads obj/<Configuration>/ from the on-disk paths Roslyn source generators (the [LoggerMessage] generator CA1848 mandates) write into, rather than inferring anything from the coverage numbers themselves. If detection is inconclusive — no generated-file path present, e.g. a report scoped to one generator-free project — it refuses too, rather than warning and proceeding. That branch is the one a contributor iterating locally on a single assembly would actually hit, and it's the exact path task 269 exists to protect, so it gets the same refusal as a confirmed Debug report, not a disclaimer.

CONTRIBUTING.md and the script's own header/usage state the Release requirement at the point of use.

Also folded in task 261's truncation trap: a comment on the --update block says baseline figures must come from the script's own output, never hand-derived from coveredlines/coverablelines, since ReportGenerator truncates rather than rounds.

Watched to fail, all against real reports:

Full-solution Debug:

*** Coverage ratchet refused: this report was measured from a Debug build. ***
Release is the only configuration this script supports. Debug emits substantially more coverable lines than Release, so every assembly reads under its baseline even though nothing regressed — rebuild and retest with --configuration Release, regenerate the coverage report, then re-run this script.

EXIT CODE: 3

PlaceMark.Domain.Tests-only Debug report (no source-generator assembly present — the inconclusive case the review found):

*** Coverage ratchet refused: could not determine the build configuration this report was measured from. ***
No generated-file path was found in the merged Cobertura report to read a configuration from — that happens when none of the instrumented assemblies carry a Roslyn source generator, which means this report was merged from something narrower than the full solution (a single project's tests, say). Regenerate a full-solution coverage report — dotnet test PlaceMark.slnx --configuration Release --collect:"XPlat Code Coverage", merged with ReportGenerator — and re-run this script against that.

EXIT CODE: 3

Full-solution Release:

PlaceMark.Api: 97.4% (baseline 97.4%)
PlaceMark.Contracts: 100% (baseline 100%)
PlaceMark.Database: 91% (baseline 91%)
PlaceMark.Domain: 100% (baseline 100%)
PlaceMark.Infrastructure: 99.1% (baseline 99.1%)
PlaceMark.WebUI: 91.6% (baseline 91.6%)

Coverage ratchet held: no assembly dropped below its recorded baseline.
EXIT CODE: 0

No warning or refusal on the Release run — CI's report is always full-solution and always includes Api/Infrastructure's generator, so this doesn't touch CI's green path (confirmed: run #852, jobs build/e2e/container-images all success at the prior head).

Closes task 269. `check-coverage-ratchet.cs` now takes ReportGenerator's merged Cobertura report (new `Cobertura` reporttype in CI) as a required third argument, and refuses to run — exit 3, before any comparison — unless the generated-file paths in it name Release. Detection reads `obj/<Configuration>/` from the on-disk paths Roslyn source generators (the `[LoggerMessage]` generator CA1848 mandates) write into, rather than inferring anything from the coverage numbers themselves. **If detection is inconclusive — no generated-file path present, e.g. a report scoped to one generator-free project — it refuses too, rather than warning and proceeding.** That branch is the one a contributor iterating locally on a single assembly would actually hit, and it's the exact path task 269 exists to protect, so it gets the same refusal as a confirmed Debug report, not a disclaimer. CONTRIBUTING.md and the script's own header/usage state the Release requirement at the point of use. Also folded in task 261's truncation trap: a comment on the `--update` block says baseline figures must come from the script's own output, never hand-derived from `coveredlines`/`coverablelines`, since ReportGenerator truncates rather than rounds. **Watched to fail**, all against real reports: Full-solution Debug: ``` *** Coverage ratchet refused: this report was measured from a Debug build. *** Release is the only configuration this script supports. Debug emits substantially more coverable lines than Release, so every assembly reads under its baseline even though nothing regressed — rebuild and retest with --configuration Release, regenerate the coverage report, then re-run this script. EXIT CODE: 3 ``` `PlaceMark.Domain.Tests`-only Debug report (no source-generator assembly present — the inconclusive case the review found): ``` *** Coverage ratchet refused: could not determine the build configuration this report was measured from. *** No generated-file path was found in the merged Cobertura report to read a configuration from — that happens when none of the instrumented assemblies carry a Roslyn source generator, which means this report was merged from something narrower than the full solution (a single project's tests, say). Regenerate a full-solution coverage report — dotnet test PlaceMark.slnx --configuration Release --collect:"XPlat Code Coverage", merged with ReportGenerator — and re-run this script against that. EXIT CODE: 3 ``` Full-solution Release: ``` PlaceMark.Api: 97.4% (baseline 97.4%) PlaceMark.Contracts: 100% (baseline 100%) PlaceMark.Database: 91% (baseline 91%) PlaceMark.Domain: 100% (baseline 100%) PlaceMark.Infrastructure: 99.1% (baseline 99.1%) PlaceMark.WebUI: 91.6% (baseline 91.6%) Coverage ratchet held: no assembly dropped below its recorded baseline. EXIT CODE: 0 ``` No warning or refusal on the Release run — CI's report is always full-solution and always includes Api/Infrastructure's generator, so this doesn't touch CI's green path (confirmed: run #852, jobs `build`/`e2e`/`container-images` all `success` at the prior head).

rob added 1 commit

2026-08-21 05:59:20 +00:00

Refuse a Debug-measured coverage report in the ratchet

CI / build (pull_request) Successful in 6m27s

Details

CI / container-images (pull_request) Successful in 3m7s

Details

CI / e2e (pull_request) Successful in 5m54s

Details

c7c09b1d5a

rob reviewed

2026-08-21 06:19:34 +00:00

scripts/check-coverage-ratchet.cs Outdated

					
				@ -35,0 +63,4 @@

					return 3;

				}

				if (buildConfiguration is null)

Confirmed live: a Cobertura report scoped to a generator-free project (built and merged PlaceMark.Domain.Tests in Debug — PlaceMark.Domain and PlaceMark.Contracts carry no [LoggerMessage] usage) hits this branch and the script proceeds silently, exit 0, on a genuinely Debug-measured report. That's the exact trap task 269 exists to close, reappearing behind a warning instead of a refusal.

Today's CI invocation is safe because the merged report is always full-solution and Api/Infrastructure's generator is always present (verified: their virtual obj/Release/net10.0/...LoggerMessage.g.cs paths do show up in the Cobertura output even though the files never touch disk, since EmitCompilerGeneratedFiles isn't set — the mechanism works as described). But nothing stops a narrower local report from reaching this branch with the warning easy to miss among other output. Given the ticket's own bar ("refusing, or printing an unmissable warning"), consider refusing (non-zero exit) when detection is inconclusive rather than assuming Release, or at least flag the limitation in CONTRIBUTING.md's local-reproduction instructions.

Confirmed live: a Cobertura report scoped to a generator-free project (built and merged `PlaceMark.Domain.Tests` in Debug — `PlaceMark.Domain` and `PlaceMark.Contracts` carry no `[LoggerMessage]` usage) hits this branch and the script proceeds silently, exit 0, on a genuinely Debug-measured report. That's the exact trap task 269 exists to close, reappearing behind a warning instead of a refusal. Today's CI invocation is safe because the merged report is always full-solution and Api/Infrastructure's generator is always present (verified: their virtual `obj/Release/net10.0/...LoggerMessage.g.cs` paths do show up in the Cobertura output even though the files never touch disk, since `EmitCompilerGeneratedFiles` isn't set — the mechanism works as described). But nothing stops a narrower local report from reaching this branch with the warning easy to miss among other output. Given the ticket's own bar ("refusing, or printing an unmissable warning"), consider refusing (non-zero exit) when detection is inconclusive rather than assuming Release, or at least flag the limitation in CONTRIBUTING.md's local-reproduction instructions.

scripts/check-coverage-ratchet.cs

					
				@ -122,0 +176,4 @@

						@"[/\\]obj[/\\](?<configuration>[^/\\]+)[/\\]",

						RegexOptions.CultureInvariant);

					var coberturaDocument = XDocument.Load(coberturaPath);

Minor, not a regression: a missing/mistyped <cobertura.xml> path crashes with an unhandled FileNotFoundException and a raw stack trace rather than the script's own clean usage error — confirmed. Matches the existing (lack of) handling for summaryPath/baselinePath though, so low priority.

Minor, not a regression: a missing/mistyped `<cobertura.xml>` path crashes with an unhandled `FileNotFoundException` and a raw stack trace rather than the script's own clean usage error — confirmed. Matches the existing (lack of) handling for `summaryPath`/`baselinePath` though, so low priority.

rob reviewed

2026-08-21 06:19:46 +00:00

rob left a comment

Verdict: mergeable

Re-derived the core claims against real reports (not the PR's pasted account), from main:

Built and merged a Debug PlaceMark.Api.Tests Cobertura report against a Release-derived baseline: refuses, exit 3, matching the PR's own evidence.
Deleted the guard and reran the identical Debug report: silently produces a false regression (Api 95.8% vs baseline 97.4%) — the exact trap task 269 describes. Confirms the guard, not something else, is what's discriminating.
Confirmed the detection mechanism itself: [LoggerMessage]-generated code's virtual path (obj/<Configuration>/net10.0/...LoggerMessage.g.cs) appears in coverlet's Cobertura output even though the generated file never touches disk (EmitCompilerGeneratedFiles isn't set) — the regex has a real signal to read, as claimed.
Checked stale-artefact risk: obj/Debug and obj/Release coexisted on disk from prior builds throughout, and each configuration's own build/test/merge only ever produced its own configuration's paths — no cross-contamination observed.
Checked CI job status (not run status) for #852 / SHA c7c09b1: build, e2e, container-images all success.
Checked the required-argument change against every caller: ci.yml passes the new third arg; CONTRIBUTING.md never hardcodes the full invocation, so it isn't broken by the new arg.
Checked merge overlap with PR #205: that PR's ci.yml change is in the unrelated e2e job's concurrency block (~line 302+), well clear of this PR's build-job hunks (~184–201). No textual conflict.

One real gap found and left inline: the "no generated-file path → warn and proceed" branch is reachable on a genuinely Debug report whenever the merged report doesn't happen to include an assembly with a source generator (reproduced with a PlaceMark.Domain-only report) — safe today because CI's own report is always full-solution, but a silent trap for any narrower local run. Worth tightening, not worth blocking the merge over, since it doesn't weaken what this PR was scoped to fix.

Verdict: mergeable Re-derived the core claims against real reports (not the PR's pasted account), from `main`: - Built and merged a Debug `PlaceMark.Api.Tests` Cobertura report against a Release-derived baseline: refuses, exit 3, matching the PR's own evidence. - Deleted the guard and reran the identical Debug report: silently produces a false regression (Api 95.8% vs baseline 97.4%) — the exact trap task 269 describes. Confirms the guard, not something else, is what's discriminating. - Confirmed the detection mechanism itself: `[LoggerMessage]`-generated code's virtual path (`obj/<Configuration>/net10.0/...LoggerMessage.g.cs`) appears in coverlet's Cobertura output even though the generated file never touches disk (`EmitCompilerGeneratedFiles` isn't set) — the regex has a real signal to read, as claimed. - Checked stale-artefact risk: `obj/Debug` and `obj/Release` coexisted on disk from prior builds throughout, and each configuration's own build/test/merge only ever produced its own configuration's paths — no cross-contamination observed. - Checked CI job status (not run status) for #852 / SHA `c7c09b1`: `build`, `e2e`, `container-images` all `success`. - Checked the required-argument change against every caller: `ci.yml` passes the new third arg; `CONTRIBUTING.md` never hardcodes the full invocation, so it isn't broken by the new arg. - Checked merge overlap with PR #205: that PR's `ci.yml` change is in the unrelated `e2e` job's `concurrency` block (~line 302+), well clear of this PR's `build`-job hunks (~184–201). No textual conflict. One real gap found and left inline: the "no generated-file path → warn and proceed" branch is reachable on a genuinely Debug report whenever the merged report doesn't happen to include an assembly with a source generator (reproduced with a `PlaceMark.Domain`-only report) — safe today because CI's own report is always full-solution, but a silent trap for any narrower local run. Worth tightening, not worth blocking the merge over, since it doesn't weaken what this PR was scoped to fix.

rob added 1 commit

2026-08-21 06:24:06 +00:00

Refuse rather than warn when the ratchet can't tell the configuration

CI / build (pull_request) Successful in 10m36s

Details

CI / container-images (pull_request) Successful in 6m35s

Details

CI / e2e (pull_request) Failing after 1h13m22s

Details

25fc498eb7

rob added 1 commit

2026-08-21 07:52:32 +00:00

Empty commit to retrigger CI on an idle runner

CI / build (pull_request) Successful in 3m52s

Details

CI / container-images (pull_request) Successful in 3m0s

Details

CI / e2e (pull_request) Successful in 5m37s

Details

3083c26738

rob reviewed

2026-08-21 08:07:27 +00:00

scripts/check-coverage-ratchet.cs

					
				@ -33,2 +49,4 @@

				var coberturaPath = args[2];

				var updateBaseline = args.Contains("--update");

				var buildConfiguration = DetectBuildConfiguration(coberturaPath);

Confirmed: this now falls inside the refusal block (exit 3) rather than the old "warn and proceed" path. Re-derived independently — built a Debug PlaceMark.Domain.Tests-only Cobertura report and ran the script against it: refuses, exit 3, matching the PR's evidence. Reverting this line's containing condition back to buildConfiguration is not null && buildConfiguration != "Release" (the pre-fix shape) against the same report drops the clean refusal and produces a confusing partial failure instead (missing-baseline noise plus a misleading "Domain has dropped below its baseline" line) — confirms this branch is what's discriminating, not incidental.

Confirmed: this now falls inside the refusal block (exit 3) rather than the old "warn and proceed" path. Re-derived independently — built a Debug `PlaceMark.Domain.Tests`-only Cobertura report and ran the script against it: refuses, exit 3, matching the PR's evidence. Reverting this line's containing condition back to `buildConfiguration is not null && buildConfiguration != "Release"` (the pre-fix shape) against the same report drops the clean refusal and produces a confusing partial failure instead (missing-baseline noise plus a misleading "Domain has dropped below its baseline" line) — confirms this branch is what's discriminating, not incidental.

rob reviewed

2026-08-21 08:07:39 +00:00

rob left a comment

Verdict: mergeable

Round 1's blocking finding is fixed: the null (inconclusive) case now sits inside the refusal block (exit 3) instead of warning and proceeding. Re-derived independently, not from the author's account:

Built a Debug PlaceMark.Domain.Tests-only Cobertura report (the known scoped/generator-free reproduction) and ran the script: refuses, exit 3, with the "could not determine the build configuration" message.
Mutated the guard back to the pre-fix shape (buildConfiguration is not null && buildConfiguration != "Release") and reran against the same report: the clean refusal disappears and it instead falls through to a confusing partial failure (missing-baseline noise for the five unscoped assemblies plus a misleading "Domain has dropped below its baseline" line) — confirms this is the discriminating line, not an untestable assertion.
git diff between c7c09b1 and this head for the script shows only this branch changed (warn+proceed → refuse) plus the required third argument and header/usage text; the Release-vs-named-Debug comparison and DetectBuildConfiguration regex logic are untouched, so round 1's other confirmations (full-solution Debug refuses, full-solution Release passes clean, the LoggerMessage virtual-path detection mechanism) still stand.
Every caller updated for the new required third arg: ci.yml passes coverage-report/Cobertura.xml and adds Cobertura to -reporttypes, and it's the only invocation in the workflow. CONTRIBUTING.md never hardcodes the full command, so it isn't broken by the arg change, and its new paragraph correctly says the script "refuses to run" (not "warns") — matches the code. No opt-out flag was added.
CI run #874 (head 3083c26): success at run level, consistent with the brief's job-level confirmation; not re-run.

Non-blocking, carried over from round 1's review comment 1975 and unaddressed here (fine to leave): a missing/mistyped <cobertura.xml> path throws a raw FileNotFoundException rather than a clean usage error, matching the existing (lack of) handling for the other two path arguments.

Verdict: mergeable Round 1's blocking finding is fixed: the null (inconclusive) case now sits inside the refusal block (exit 3) instead of warning and proceeding. Re-derived independently, not from the author's account: - Built a Debug `PlaceMark.Domain.Tests`-only Cobertura report (the known scoped/generator-free reproduction) and ran the script: refuses, exit 3, with the "could not determine the build configuration" message. - Mutated the guard back to the pre-fix shape (`buildConfiguration is not null && buildConfiguration != "Release"`) and reran against the same report: the clean refusal disappears and it instead falls through to a confusing partial failure (missing-baseline noise for the five unscoped assemblies plus a misleading "Domain has dropped below its baseline" line) — confirms this is the discriminating line, not an untestable assertion. - `git diff` between `c7c09b1` and this head for the script shows only this branch changed (warn+proceed → refuse) plus the required third argument and header/usage text; the Release-vs-named-Debug comparison and `DetectBuildConfiguration` regex logic are untouched, so round 1's other confirmations (full-solution Debug refuses, full-solution Release passes clean, the LoggerMessage virtual-path detection mechanism) still stand. - Every caller updated for the new required third arg: `ci.yml` passes `coverage-report/Cobertura.xml` and adds `Cobertura` to `-reporttypes`, and it's the only invocation in the workflow. `CONTRIBUTING.md` never hardcodes the full command, so it isn't broken by the arg change, and its new paragraph correctly says the script "refuses to run" (not "warns") — matches the code. No opt-out flag was added. - CI run #874 (head `3083c26`): `success` at run level, consistent with the brief's job-level confirmation; not re-run. Non-blocking, carried over from round 1's review comment 1975 and unaddressed here (fine to leave): a missing/mistyped `<cobertura.xml>` path throws a raw `FileNotFoundException` rather than a clean usage error, matching the existing (lack of) handling for the other two path arguments.

rob merged commit 1facc5d68e into main

2026-08-21 08:08:45 +00:00

rob deleted branch coverage-ratchet-configuration

2026-08-21 08:08:45 +00:00

rob referenced this pull request from a commit

2026-08-21 08:08:47 +00:00

Refuse a Debug-measured coverage report in the ratchet (#206)

~~rob referenced this pull request 2026-08-21 11:01:22 +00:00~~

Wait for Blazor to boot before driving the E2E sign-in flow #210

Sign in to join this conversation.

No reviewers

No labels