Skip to content

fix: include serial tests in coverage.xml and upload it from CI - #395

Merged
justinwlin merged 2 commits into
mainfrom
ci-coverage-artifact
Oct 27, 2026
Merged

justinwlin merged 2 commits into
mainfrom
ci-coverage-artifact

Conversation

@justinwlin

@justinwlin justinwlin commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

The bug

Our coverage run is split in two passes:

uv run pytest tests/ ... -m "not serial" --cov=runpod_flash --cov-report=xml
uv run pytest tests/ ... -m "serial"     --cov=runpod_flash --cov-append --cov-report=term-missing

The second pass appends to .coverage but only re-renders the terminal report — it never re-emits the XML. So coverage.xml on disk holds the parallel run alone, and the 53 tests in serial-marked groups contribute nothing to it.

Measured locally:

Coverage Covered lines
Before the append is re-emitted 85.1% 8085 / 9503
After 85.6% 8132 / 9503

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=xml to the --cov-append pass in both test-coverage and ci-quality-github, so coverage.xml includes the serial tests. The explanatory comments sit above each target, so make no longer echoes them into the logs.
  • pyproject.toml: branch = true in [tool.coverage.run]. Every target now measures line + branch coverage against the same 65% gate, including test-coverage-serial and ci-quality-github-serial.
  • .github/workflows/ci.yml: publish coverage.xml per matrix leg as coverage-${{ matrix.python-version }} through the shared runpod/coverage-summary-action, pinned by commit, with if-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.xml does not carry that combined number. Its root has line-rate and branch-rate separately. To match the gate, a consumer must compute (lines-covered + branches-covered) / (lines-valid + branches-valid) from the root attributes. Reading line-rate alone 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 current main:

2762 passed, 1 skipped, 1 xfailed (parallel)
53 passed, 1 skipped (serial)
Coverage XML written to file coverage.xml
Required test coverage of 65% reached. Total coverage: 85.33%

coverage.xml reports 9557 lines (86.83% covered) and 3172 branches (80.83% covered). The log contains no echoed Makefile comment lines. CI on GitHub publishes a coverage-<version> artifact for each Python version.

No production code touched — CI config and Makefile flags only.

Supersedes #373 (same final diff, squashed onto current main as one commit).

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 OneAsterisk left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 carries line-rate (lines only) and branch-rate separately. The combined (lines + branches) number that the 65% gate and the terminal TOTAL use 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 reads line-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-270 The history of the failed internal-workflows attempt belongs in the commit message. The comment only needs "public repo, pinned by SHA".
  • Makefile:132-134 The 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-append plus --cov-report=xml in the serial pass writes the combined XML after the append. --cov-fail-under=0 stays on the parallel pass only, so the 65% gate is still enforced once, on the combined data.
  • make failure semantics: a failing parallel pass stops the recipe before the serial pass. The comment at Makefile:129-134 describes this correctly.
  • The action's summarize.py always exits 0, and the upload uses if-no-files-found: warn, so a missing or unparseable coverage.xml cannot fail the job or mask the real failure.
  • Inputs reach the action script through env, not interpolation. No secrets or extra permissions needed.
  • Artifact names are unique per matrix leg (coverage-<python-version>), so the default overwrite: true never collides across legs.
  • Branch mode on both passes: the first pass has no --cov-append, so a stale line-mode .coverage cannot 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.

Comment thread pyproject.toml
Comment thread .github/workflows/ci.yml Outdated
Drop the internal-workflows history from the ci.yml comment and the
reference to the external weekly report from the Makefile comment.

@OneAsterisk OneAsterisk left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 8f25af7 plus 0ae6b1a. Harmless under squash-merge, but the chore: subject will land in the squash body. Reword the line or squash locally.

Verified clean

  • git diff 8f25af7..0ae6b1a touches comments only (ci.yml, Makefile). No recipe, flag, or step changed.
  • Round 1 checks still hold: --cov-fail-under=0 only on the parallel pass, serial pass inherits the 65% gate, make stops before the re-emit if the parallel pass fails, and the action cannot fail the job on a missing XML.
  • Merge base is 4e5001e (current main at 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.

@justinwlin
justinwlin merged commit 830c813 into main Oct 27, 2026
10 checks passed
@justinwlin
justinwlin deleted the ci-coverage-artifact branch October 27, 2026 15:00
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.

2 participants