Repository navigation
fix: include serial tests in coverage.xml and upload it from CI - #395
Conversation
Re-emit coverage.xml on the --cov-append pass so the serial tests are counted, collect branch coverage, and publish coverage.xml per matrix leg through runpod/coverage-summary-action. Supersedes #373.
OneAsterisk
left a comment
There was a problem hiding this comment.
Claude review, round 1
Verdict: Approved. The XML re-emit fix is correct, the action pin resolves to a public repo, and nothing here can break the PR gate.
Approval gate: 0 Critical, 0 High, 1 Medium+Low → approved. CI Quality Gates and Nix flake were still pending at review time; branch protection still enforces them.
PR description vs code
| Claim | Status | Where |
|---|---|---|
Serial pass re-emits coverage.xml in test-coverage and ci-quality-github |
✅ | Makefile:78, Makefile:146 |
Comments moved above targets so make does not echo them |
✅ | Makefile:74-75, Makefile:121-134 (no # lines inside recipes) |
branch = true, every target uses the same 65% gate |
✅ | pyproject.toml:122, gate from addopts pyproject.toml:74 |
Action pinned by commit, if-no-files-found: warn, separate test-results upload |
✅ | ci.yml:278-286; runpod/coverage-summary-action is public, 65b35a3 is main HEAD |
| "The published metric changed" to 85.33% | See Low finding. The artifact does not contain that number. | |
CI publishes coverage-<version> per leg |
Not verifiable yet, Quality Gates were pending. | |
| No production code touched | ✅ | Only ci.yml, Makefile, pyproject.toml |
Findings
Low
- The 85.33% "published metric" is not in the published artifact (
pyproject.toml:122). coverage.py's Cobertura root carriesline-rate(lines only) andbranch-rateseparately. The combined (lines + branches) number that the 65% gate and the terminalTOTALuse appears in neither. Your own verification shows it: XML says 86.83% lines and 80.83% branches, and 85.33% is only derivable as(lines-covered + branches-covered) / (lines-valid + branches-valid). If the weekly report readsline-rate, it gets a different number from the gate. Fix: state in the PR body (and in the report script) which value the report uses, and compute the combined figure from the four root attributes if it should match the gate.
Nits
.github/workflows/ci.yml:266-270The history of the failed internal-workflows attempt belongs in the commit message. The comment only needs "public repo, pinned by SHA".Makefile:132-134The comment names an external consumer (the weekly report and its "success only" filter) that lives outside this repo, so it goes stale silently if that report changes.
Verified clean
--cov-appendplus--cov-report=xmlin the serial pass writes the combined XML after the append.--cov-fail-under=0stays on the parallel pass only, so the 65% gate is still enforced once, on the combined data.makefailure semantics: a failing parallel pass stops the recipe before the serial pass. The comment atMakefile:129-134describes this correctly.- The action's
summarize.pyalways exits 0, and the upload usesif-no-files-found: warn, so a missing or unparseablecoverage.xmlcannot fail the job or mask the real failure. - Inputs reach the action script through
env, not interpolation. No secrets or extrapermissionsneeded. - Artifact names are unique per matrix leg (
coverage-<python-version>), so the defaultoverwrite: truenever collides across legs. - Branch mode on both passes: the first pass has no
--cov-append, so a stale line-mode.coveragecannot cause a "Can't combine arc data with line data" error.
Questions for the author
- Which matrix leg does the weekly report read? There are five
coverage-*artifacts per run and the numbers can differ slightly per Python version.
Reviewed by Claude (claude-opus-5-5) via the pr-review skill. Tokens: 18 in / 5,491 out / 450,477 cache read / 53,368 cache write = 509,354. Estimated cost: $1.45 at the skill's bundled Opus 5.5 rate card, 5-minute cache writes (1.25× input). Round 1 of this PR. This is automated assistance; a human owns the final call.
Drop the internal-workflows history from the ci.yml comment and the reference to the external weekly report from the Makefile comment.
OneAsterisk
left a comment
There was a problem hiding this comment.
Claude review, round 2
Verdict: Approved. The new commit (0ae6b1a) only trims comments, and it closes both round 1 nits. The PR body now closes the Low.
Approval gate: 0 Critical, 0 High, 0 Medium+Low → approved. Quality Gates (3.11) passed and published coverage-3.11. The other legs and Nix flake were still pending at review time; branch protection still enforces them.
Status of prior findings
| # | Finding | Severity | Status | Evidence |
|---|---|---|---|---|
| R1-1 | 85.33% combined metric is not in coverage.xml |
Low | Fixed | PR body now says the root has line-rate and branch-rate separately and gives the combined formula. The report script lives outside this repo. |
| R1-2 | Internal-workflows history in the ci.yml comment |
Nit | Fixed | ci.yml:266-267 at 0ae6b1a |
| R1-3 | Makefile comment names the external weekly report | Nit | Fixed | Makefile:129-132 at 0ae6b1a |
Existing feedback
- No human or bot comments on the PR. Only threads are from round 1 (above).
PR description vs code
| Claim | Status | Where |
|---|---|---|
Serial pass re-emits coverage.xml in both targets |
✅ | Makefile:78, Makefile:144 |
branch = true, same 65% gate everywhere |
✅ | pyproject.toml:122, addopts pyproject.toml:74 |
Action pinned by commit, if-no-files-found: warn, separate test-results upload |
✅ | ci.yml:268-283; 65b35a3 still resolves in the public repo |
CI publishes coverage-<version> per leg |
✅ (partial) | Run 40520696110 has coverage-3.11 (31 KB) and test-results-3.11. Other legs pending. |
coverage.xml has line-rate and branch-rate, not the combined number |
✅ | Matches coverage.py's Cobertura output |
"squashed onto current main as one commit" |
Now two commits (see nit). |
Findings
Nits
- PR body says "as one commit". The head now has
8f25af7plus0ae6b1a. Harmless under squash-merge, but thechore:subject will land in the squash body. Reword the line or squash locally.
Verified clean
git diff 8f25af7..0ae6b1atouches comments only (ci.yml,Makefile). No recipe, flag, or step changed.- Round 1 checks still hold:
--cov-fail-under=0only on the parallel pass, serial pass inherits the 65% gate,makestops before the re-emit if the parallel pass fails, and the action cannot fail the job on a missing XML. - Merge base is
4e5001e(currentmainat review time).
Questions for the author
- None.
Reviewed by Claude (claude-opus-5-5) via the pr-review skill. Tokens: 10 in / 2,538 out / 207,014 cache read / 43,295 cache write = 252,857 (before posting). Estimated cost: $1.04 at the skill's sum_usage.py Opus 5.5 rate card, 5-minute cache-write TTL. Round 2 of this PR. This is automated assistance; a human owns the final call.
The bug
Our coverage run is split in two passes:
The second pass appends to
.coveragebut only re-renders the terminal report — it never re-emits the XML. Socoverage.xmlon disk holds the parallel run alone, and the 53 tests inserial-marked groups contribute nothing to it.Measured locally:
47 covered lines were being dropped from the report. The terminal output was always correct; only the XML was wrong, which is why this went unnoticed — nothing consumed the XML until now.
Changes
Makefile: add--cov-report=xmlto the--cov-appendpass in bothtest-coverageandci-quality-github, socoverage.xmlincludes the serial tests. The explanatory comments sit above each target, somakeno longer echoes them into the logs.pyproject.toml:branch = truein[tool.coverage.run]. Every target now measures line + branch coverage against the same 65% gate, includingtest-coverage-serialandci-quality-github-serial..github/workflows/ci.yml: publishcoverage.xmlper matrix leg ascoverage-${{ matrix.python-version }}through the sharedrunpod/coverage-summary-action, pinned by commit, withif-no-files-found: warn. The test-results upload stays a separate step.The published metric changed. With branch coverage on, the total is (lines + branches covered) / (lines + branches): 85.33% locally, against 85.6% for line coverage alone. Anything comparing against older numbers should expect that step.
coverage.xmldoes not carry that combined number. Its root hasline-rateandbranch-rateseparately. To match the gate, a consumer must compute(lines-covered + branches-covered) / (lines-valid + branches-valid)from the root attributes. Readingline-ratealone gives line coverage (86.83% locally), which will not match the gate.Why upload it
We're automating the weekly Test Coverage Progress report, which currently relies on hand-collected numbers. That has produced real errors — figures reported as improvements when they were regressions, and at least one number that matched no CI run at all. With this artifact published, the report reads the number straight from the newest successful run on
main.Verification
Ran the full CI gate (
make ci-quality-github) on the branch merged with currentmain:coverage.xmlreports 9557 lines (86.83% covered) and 3172 branches (80.83% covered). The log contains no echoed Makefile comment lines. CI on GitHub publishes acoverage-<version>artifact for each Python version.No production code touched — CI config and Makefile flags only.
Supersedes #373 (same final diff, squashed onto current
mainas one commit).