Skip to content

fix(coverage): dedupe Cobertura line and branch arithmetic (#441, #478) - #538

Merged
drmoisan merged 3 commits into
epic/build-ci-coverage-gate-fidelity-integrationfrom
bug/cobertura-coverage-arithmetic-441
Aug 11, 2026
Merged

fix(coverage): dedupe Cobertura line and branch arithmetic (#441, #478)#538
drmoisan merged 3 commits into
epic/build-ci-coverage-gate-fidelity-integrationfrom
bug/cobertura-coverage-arithmetic-441

Conversation

@drmoisan

Copy link
Copy Markdown
Owner

Summary

  • Corrects the Cobertura post-processing arithmetic in scripts/vscode/Invoke-MSTestWithCoverage.Helpers.ps1, which counted every source line twice and emitted a per-file line-rate blended from two different denominators.
  • Get-CoberturaCoverageSummary now counts each source line exactly once per class, deduplicating by line number with max(hits). Branch counters are deduplicated on the same basis.
  • Verified against a generator-parity oracle: run over the committed raw dotnet-coverage document, the corrected code now reproduces that document's own root attributes exactly — 79957 / 56124 / 23109 / 13472, where it previously reported lines-valid 161086.
  • Adds a new pure helper Get-CoberturaClassLineSummary plus six regression fixtures; four were demonstrated failing against the unmodified script before any production change, per the repository's bugfix workflow.
  • Scope is deliberately narrow: exactly two source files changed, no coverage threshold re-tuned, and no edit to CLAUDE.md or .claude/rules/.

Why

Two compounding defects lived in the same rate-recomputation path.

Issue #441 — descendant-axis double count. Get-CoberturaCoverageSummary selected over the XPath descendant axis .//lines/line. In the Cobertura documents this pipeline produces, each <class> carries its <line> nodes twice — once nested under each <method>, and once again as a class-level rollup. The descendant axis matched both sets, so every line was counted twice. The inflation is not uniform across assemblies, because duplication is not uniform across classes.

Issue #478 — blended merge denominator. Merge-CoberturaClassesByFilename unioned the class-level <lines> of same-filename classes correctly, but never merged the corresponding <methods> subtrees. It then recomputed line-rate over the same descendant axis, which saw the correct union plus only the primary class's method-level lines. The emitted per-file line-rate matched neither denominator.

Root-cause analysis identified a single defective selection rather than the two sites named in the issue text. The line numbers cited in the GitHub issues (:98, :167) are function declaration lines, not selections. The verified situation was:

Site Expression Verdict
Helpers.ps1:122 $cls.SelectNodes('.//lines/line') The one and only defective selection
Helpers.ps1:219 $classNode.SelectNodes('./lines/line') Already correct (child axis) — the union builder, must not change
Helpers.ps1:270-273 delegation to Get-CoberturaCoverageSummary Indirect; how the :122 defect reached the merged per-class rate
Invoke-MSTestWithCoverage.ps1 No line-axis selection at all; no change required

Editing :219 instead of :122 would have destroyed the correct union and left both defects in place.

Every coverage gate and every committed coverage baseline in the repository consumes the affected figure, and epic #136 gates each of its fifteen children on a per-file line rate.

What Changed

Core fix — scripts/vscode/Invoke-MSTestWithCoverage.Helpers.ps1 (+132/-~34, 357 → 455 lines)

  • New pure function Get-CoberturaClassLineSummary: takes a single [System.Xml.XmlElement]$ClassNode, performs no I/O, mutates nothing in the source document, and returns LineMap, TotalLines, CoveredLines, TotalBranches, CoveredBranches. Deduplicates by line number with max(hits) and resolves condition-coverage per distinct line.
  • Get-CoberturaCoverageSummary now delegates to the helper instead of selecting over .//lines/line. Zero occurrences of .//lines/line remain in the file.
  • The $classSummaryXml synthetic-document delegation in Merge-CoberturaClassesByFilename is replaced by a direct helper call. Zero occurrences of $classSummaryXml remain.
  • The correct union builder is untouched: its 52 lines are byte-identical to the pre-change file (verified by cmp before and after formatting).

Tests — tests/scripts/vscode/Invoke-MSTestWithCoverage.Helpers.Tests.ps1 (+246/-0, 222 → 468 lines)

Six new It blocks appended inside the existing Describe, plus direct unit tests for the new helper. Zero deletions — no pre-existing block was modified.

Fixture Covers Expect-fail
F1 #441 line counts (lines-valid 3, lines-covered 2) yes
F2 #441 branch counts (branches-valid 2, branches-covered 1) yes
F3 #478 merged per-file line-rate = '0.6', five ascending lines yes
F4 max(hits) dedup across two constructors yes
F5 rollup-absent guard (class with <methods> but no class-level <lines>) no — passes before and after by design
F6 structure preservation (merged class retains <methods>, per-line hits preserved) no — passes before and after by design

Every fixture uses an inline single-quoted here-string, creates no file on disk, uses no mock, and passes -ProjectNames explicitly for determinism.

Docs and evidence

  • spec.md acceptance criteria checked off (20/20) and the plan checklist completed (85/85 tasks).
  • 34 evidence artifacts under docs/features/active/2026-08-10-cobertura-coverage-arithmetic-441/evidence/, plus the three review artifacts (policy-audit, code-review, feature-audit).
  • Six promoted potential-bug entries under docs/features/potential/promoted/ corresponding to the follow-up issues filed below.
  • Agent-memory notes recorded by the executing and reviewing agents.

Architecture / How It Fits Together

ConvertTo-KoverageCoberturaXml is the pipeline entry point. It filters .Test packages, merges same-filename classes, and rewrites coverage attributes. Control flow after this change:

ConvertTo-KoverageCoberturaXml
  ├─ package filtering
  ├─ Merge-CoberturaClassesByFilename
  │    ├─ class-level <lines> union (child axis; UNCHANGED)
  │    └─ Get-CoberturaClassLineSummary  ← replaces the $classSummaryXml delegation
  └─ Get-CoberturaCoverageSummary
       └─ Get-CoberturaClassLineSummary  ← replaces the .//lines/line selection

Both call sites now share one pure, separately unit-tested counting function, so the root summary and the merged per-class rate can no longer disagree. Get-CoberturaClassLineSummary is the seam: it is host-neutral, does no I/O, and is exercised directly by unit tests covering all three condition-coverage precedence branches and the zero-input boundary.

Verification

Completed

Toolchain (PowerShell), single clean pass with no restart:

  1. mcp__drm-copilot__run_poshqc_formatok:true, 0 files changed (SHA-256 identical before and after for both in-scope files).
  2. mcp__drm-copilot__run_poshqc_analyze plus direct Invoke-ScriptAnalyzer per file — 0 new findings. The one pre-existing PSUseSingularNouns finding on Get-CoberturaLineConditionCoverageParts persists and moved line 146 → 140; the baseline is keyed on (ScriptName, RuleName, Severity, Message) with Line recorded as an observation only, so a line-number move on an otherwise-identical finding is not a new finding.
  3. Invoke-Pester19 passed / 0 failed (8 pre-existing + 6 fixtures + 5 helper unit tests).

Arithmetic verification:

Measurement Pre-change Post-change Ground truth
Oracle lines-valid 161086 79957 79957
Oracle lines-covered 113219 56124 56124
Oracle branches-valid 46218 23109 23109
Oracle branches-covered 26944 13472 13472
Package-filtered lines-valid 110849 62345
Package-filtered line-rate 0.856453 0.850317

Fail-before evidence: F1-F4 failed against the unmodified script with FailedCount 4 / PassedCount 10, reporting 6/4, 4/2, '0.75' and 3/2 respectively. Reproduced independently during review.

Coverage of the changed production file: LINE 146/165 = 88.48% → 183/202 = 90.59%. New code (Get-CoberturaClassLineSummary) is 39/40 = 97.50% against the >= 90% new-code floor.

Feature review returned 0 blocking findings (0 FAIL, 0 blocking-PARTIAL); all 20 acceptance criteria independently verified PASS. Five non-blocking findings were recorded and dispositioned.

Recommended

Invoke-Pester ./tests/scripts/vscode/Invoke-MSTestWithCoverage.Helpers.Tests.ps1 -Output Detailed
Invoke-ScriptAnalyzer -Path ./scripts/vscode/Invoke-MSTestWithCoverage.Helpers.ps1

Backward Compatibility / Migration Notes

  • Reported coverage figures will drop, and that drop is the correction, not a regression. Any consumer comparing a post-merge figure against a pre-merge baseline will see a lower number because the inflated denominator is gone. Committed coverage baselines captured before this change were computed over the doubled denominator and are no longer comparable.
  • No public function signature changed. Get-CoberturaClassLineSummary is additive.
  • No coverage threshold was re-tuned, lowered, or relaxed. git diff --name-only edf3d34c -- CLAUDE.md .claude/rules coverage.config returns empty.
  • Handoff: the corrected repository-wide line rate for the committed -424 sample is 85.0317% against the uniform 85% floor — a margin of 0.03 pp. This is recorded as a fact for sibling feature Bug: conflicting-coverage-thresholds-across-policy-docs #494 (coverage threshold policy reconciliation, wave 2), which owns threshold decisions. This PR deliberately does not act on it.

Risks and Mitigations

Risk Mitigation
The corrected denominator moves a repo-wide figure to within 0.03 pp of the 85% floor Recorded as an explicit handoff to #494 rather than resolved here; threshold ownership is out of scope for this bugfix
Editing the wrong selection site would destroy the correct union AC-8 pins the union builder at :217-268 as byte-identical, verified by cmp both before and after formatting
Merged classes still carry only the primary class's <methods> Deliberate and documented — sibling classes routinely share (name, signature) pairs, so naive merging breaks per-method consumers. Fixture F6 pins the current behavior; filed as #530
Rollback Revert the three commits on this branch; the change is additive and self-contained within two files

Review Guide

Suggested order:

  1. scripts/vscode/Invoke-MSTestWithCoverage.Helpers.ps1 — the new helper and the two rewritten call sites. This is the whole substantive change.
  2. tests/scripts/vscode/Invoke-MSTestWithCoverage.Helpers.Tests.ps1 — fixtures F1-F6 and the helper unit tests. 0 deletions, so the diff is purely additive.
  3. docs/features/active/2026-08-10-cobertura-coverage-arithmetic-441/spec.md — the 20 acceptance criteria.
  4. evidence/regression-testing/fail-before-f1-f4.*.md and evidence/qa-gates/postchange-generator-parity.*.md — the fail-before and oracle-parity proofs.
  5. policy-audit, code-review, feature-audit — the review verdicts.

The remaining ~45 files are evidence artifacts, promoted potential-bug entries, and agent-memory notes. They are additive and mechanically uninteresting.

Note for reviewers: policy-audit.2026-08-10T23-35.md row 2.5 records a FAIL against artifacts/pester/powershell-coverage.xml, which reports zero covered lines repo-wide. That is a verdict on an invalid measurement by the bundled capture tool, not on this branch — independently confirmed as LINE covered 0 / missed 16075 across all 1227 JaCoCo counters at a head whose suite passes. Filed as #536 and dispositioned non-blocking.

Follow-ups

Six issues filed through the promotion lifecycle. None is fixed by this PR.

Threshold reconciliation for the corrected figures is owned by sibling feature #494 in wave 2.

GitHub Auto-close

Note: this PR targets the epic integration branch epic/build-ci-coverage-gate-fidelity-integration, not the default branch, so GitHub will not fire these keywords on merge. They record intent for the epic's eventual integration-to-main pull request.

drmoisan and others added 3 commits August 10, 2026 23:47
Phase 6 of the approved plan requires the four follow-up candidates to be
filed through the MCP promotion lifecycle. The executing atomic-executor
session did not expose new_potential_bug_entry / potential_to_issue, so it
correctly took the plan's sanctioned POSTING BLOCKED branch and left AC-20
unchecked rather than fabricating issue numbers. The orchestrator session
does expose those tools, so the branch is resolved here instead of deferred:

- #529 package-level line-rate / branch-rate never recomputed after filtering
- #530 merged Cobertura class retains only the primary class's <methods>
- #531 Invoke-MSTestWithCoverage.ps1 discovery lacks a \.claude\ exclusion
- #532 agent memory records an incorrect Cobertura dedup generalization

Re-verified that none of the four is fixed by this change: the diff still
touches exactly two source files and all four follow-up targets are
byte-identical to edf3d34.

Also commits the plan's permitted checklist residual (P7-T23).

AC-20 checked off in spec.md; 20/20 acceptance criteria delivered.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Feature review of bug/cobertura-coverage-arithmetic-441 returned
blocking_count = 0: zero FAIL and zero blocking-PARTIAL findings, with all
20 acceptance criteria independently verified PASS. The remediation loop was
not entered.

Five non-blocking findings were recorded. The two Minor ones are dispositioned
by filing follow-up issues through the promotion lifecycle rather than by
widening a bugfix whose plan is 85/85 complete:

- #537 NF-1: the max(hits) update assignment at Helpers.ps1:220 is exercised
  by no test, so the dedup rule is pinned only for first-entry-wins ordering.
  New-code coverage is 39/40 = 97.50% against a >= 90% floor, so no gate fails.
- #536 NF-2: the bundled run_poshqc_test coverage capture writes
  artifacts/pester/powershell-coverage.xml with zero covered lines repo-wide.
  Verified independently: LINE covered 0 / missed 16075 across all 1227 JaCoCo
  counters, contradicted by the direct-Pester capture at the same head. The
  branch changes no coverage-capture tooling, so the defect is pre-existing.

The policy audit's row 2.5 FAIL is a verdict on that invalid measurement, not
on this branch's coverage. All change-scope coverage rows pass: changed file
90.59%, new code 97.50%, no regression on changed lines.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@drmoisan
drmoisan merged commit fb257cd into epic/build-ci-coverage-gate-fidelity-integration Aug 11, 2026
drmoisan added a commit that referenced this pull request Aug 11, 2026
441 merged via PR #538 (fb257cd), worktree removed. 457 launched: its only
dependency edge is 441, which is durably confirmed merged, so wave 1 opens
while sibling 512 remains in flight in wave 0.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
drmoisan added a commit that referenced this pull request Aug 15, 2026
441 merged via PR #538 (fb257cd), worktree removed. 457 launched: its only
dependency edge is 441, which is durably confirmed merged, so wave 1 opens
while sibling 512 remains in flight in wave 0.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@drmoisan
drmoisan deleted the bug/cobertura-coverage-arithmetic-441 branch August 15, 2026 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant