Repository navigation
chore: render coverage numbers into the job summary - #577
runpod-Henrik wants to merge 8 commits into
Conversation
The coverage report was only readable by downloading the artifact. Write a Test Results + Coverage Summary table to $GITHUB_STEP_SUMMARY so the numbers show up on the run page, matching what the ai-api component workflow does. Adds --junitxml so the run's test counts can be reported alongside coverage; pytest-results.xml is gitignored. Stdlib only, so there is no extra install step, and `if: always()` means the summary still renders when tests fail — which is when it is most useful. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review feedback from #373. Three defects, all reachable because this step runs under `if: always()`: * the coverage-XML parse was unguarded while the junit parse beside it was. A report truncated by a timeout, OOM or crashed xdist worker made ET.parse raise, so the summary step exited non-zero and stacked a spurious failure on top of the real one. Verified: the old script exits 1 on a truncated report, the new one exits 0 and says so in the summary. * hits/lines attributes were parsed with bare int(), so a malformed value raised rather than degrading. * the status cell treated `0 failures` as passing even when no tests ran at all. A suite dying at import reports errors>0 with tests possibly 0, so the check is now `no failures AND at least one test`. Also switch the artifact upload to `if-no-files-found: warn`. With `error` a run that never produced coverage.xml — pytest erroring at collection, before pytest-cov writes anything — failed the upload step too, red-flagging the job and masking the root cause. This upload hangs off the PR-gating test job, so it should not be able to fail a PR on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Branch coverage was never collected, so the job summary could only ever show line coverage. Adds --cov-branch. Line coverage is unchanged at 94.3% (3299/3499), so the weekly coverage trend, which reads line coverage, is unaffected. The Cobertura report now carries branch data, which the summary renders as its own row: 88.1% (796/904). One thing to be aware of: --cov-fail-under in pytest.ini gates on coverage.py's total, which now blends lines and branches. That total moves 94.28% -> 93.00% against the 90% gate, so the margin narrows from 4.28 to 3.00 points. It passes, but the gate now measures something slightly different than it did. Happy to lower it to ~88 to preserve the original strictness, or to skip branch collection here, if the tighter margin is unwelcome. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replaces the inline summary script with runpod/internal-workflows/.github/actions/coverage-summary, which now owns both the summary rendering and the artifact upload. The same ~90-line script had been copy-pasted into six repos, and review found the same bugs in every copy — four rounds of a single class where "passed" was the default state and each new way of losing a report had to be patched out of it separately. The shared version computes it from positive evidence instead, and has 59 unit tests plus a smoke job behind it. Pinned by commit rather than tag, so a change to the action cannot reach this repo until someone bumps the SHA. Behaviour was proven on runpod/github-image-builder first: green run, artifact uploaded, and the weekly collector parsed it back at the expected number before the remaining repos followed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A public repository cannot consume an action from a private one, and
runpod/internal-workflows is private. Every matrix leg failed at "Set up
job" with:
Unable to resolve action `runpod/internal-workflows`, not found
This reverts the migration to the shared action for this repo. The inline
script is restored, including all the hardening from review.
The shared action still applies to the private repos — github-image-builder
is migrated and green, main-ui and RunPod follow. Sharing it here needs
internal-workflows to be public, or the action published somewhere public.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replaces the ~90-line inline heredoc summary with the shared action, now that there is a host this repo can actually consume. The first attempt at this pointed at a private repo and failed at `Set up job` on every matrix leg with "Unable to resolve action, not found", so no tests ran at all and it was reverted. A public repository cannot resolve an action from a private one, and this repo is public. The action now lives in runpod/coverage-summary-action, and the same commit is already green in three private consumers plus this repo's sibling. The action owns the coverage artifact upload, so the separate "Upload coverage report" step is gone -- that is the same artifact, not a second one. Note for whoever revisits the gate: --cov-branch narrowed the margin against --cov-fail-under=90 in pytest.ini from 4.28 to 3.00 points. Unchanged here, but it is thinner than it looks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tifact # Conflicts: # .github/workflows/ci.yml
|
Review notes (no requested changes): the Makefile now emits both files the action consumes ( |
|
|
||
| test-coverage: | ||
| uv run pytest --cov=runpod --cov-report=term-missing --cov-report=xml | ||
| uv run pytest --cov=runpod --cov-branch --cov-report=term-missing --cov-report=xml --junitxml=pytest-results.xml |
There was a problem hiding this comment.
--cov-branch changes what the existing --cov-fail-under=90 in pytest.ini measures: the total becomes (lines+branches covered)/(lines+branches). The 94.3% in the description is line-only; combined is usually several points lower and could drop under 90 and fail make test-coverage on every leg without any test change. Even if it stays above, the gate's meaning changes silently. Was this intended? If so, worth calling out in the description and re-baselining the threshold.
| coverage-file: coverage.xml | ||
| results: pytest-results.xml | ||
| artifact-name: coverage-${{ matrix.python-version }} | ||
| if-no-files-found: warn |
There was a problem hiding this comment.
if-no-files-found went from error to warn, and the comment explaining why was removed. The weekly coverage report relies on every successful run publishing coverage-<ver>. If a later change stops coverage.xml from being written while tests pass, the run goes green with no artifact and the weekly report silently reads stale or no data. Can we keep error here?
| # it deliberately. | ||
| - name: Coverage summary | ||
| if: always() | ||
| uses: runpod/coverage-summary-action@65b35a32ecfd9c2f0dc2352394eca1decfba4915 |
There was a problem hiding this comment.
The composite action pins itself by SHA, but internally uses actions/upload-artifact@v4 (tag, not SHA), which undercuts the "cannot reach us until we bump it deliberately" comment. It's also a downgrade from the @v7 this workflow used before, and Node-runtime deprecations on v4 would need a fix in coverage-summary-action, not here.
| coverage-file: coverage.xml | ||
| results: pytest-results.xml | ||
| artifact-name: coverage-${{ matrix.python-version }} | ||
| if-no-files-found: warn |
There was a problem hiding this comment.
The action's default overwrite: true is inherited without comment, so a re-run or a future duplicate artifact name silently replaces coverage instead of failing. Worth setting explicitly (and deciding which behavior we want for the weekly consumer).
| # and pinning means a change to the action cannot reach us until we bump | ||
| # it deliberately. | ||
| - name: Coverage summary | ||
| if: always() |
There was a problem hiding this comment.
if: always() also runs on cancelled jobs and jobs that failed before tests (setup/uv install), producing a "No coverage report" summary and missing-artifact warning next to the real failure. if: ${{ !cancelled() }} keeps the tests-failed case and skips the noise.
| if-no-files-found: error | ||
| format: cobertura | ||
| coverage-file: coverage.xml | ||
| results: pytest-results.xml |
There was a problem hiding this comment.
Test status in the summary comes only from JUnit, so a job that fails solely on --cov-fail-under shows "Passed" with 0 failures while the job is red, and the summary doesn't mention the threshold. Could the summary surface the fail-under result (or the threshold vs. actual)?
| # | ||
| # Pinned by commit rather than tag: a tag can be repointed at new code, | ||
| # and pinning means a change to the action cannot reach us until we bump | ||
| # it deliberately. |
There was a problem hiding this comment.
This runs on all 5 matrix legs with the same default title "Coverage Report", so the run page shows five near-identical tables and uploads five artifacts where the weekly report needs one. Suggest gating on a single canonical version (e.g. matrix.python-version == '3.12') or putting the version in title:.
|
|
||
| test-coverage: | ||
| uv run pytest --cov=runpod --cov-report=term-missing --cov-report=xml | ||
| uv run pytest --cov=runpod --cov-branch --cov-report=term-missing --cov-report=xml --junitxml=pytest-results.xml |
There was a problem hiding this comment.
Nit: --cov=runpod --cov-report=xml --cov-report=term-missing duplicates what pytest.ini addopts already sets, and the new flags were added to the duplicate. With the threshold in pytest.ini and --cov-branch here, the interaction is easy to miss. Suggest keeping coverage config in one place.
This branch has not been deployed
3 participants
What
Follow-up to #576, which published
coverage.xmlas an artifact. That made the number collectable but still only readable by downloading the artifact. This writes a Test Results + Coverage Summary table to$GITHUB_STEP_SUMMARYso it shows up on the run page, matching what the ai-api component workflow already does.Changes
Makefile: add--junitxml=pytest-results.xmltotest-coverage, so the run's test counts can be reported alongside coverage.github/workflows/ci.yml: aCoverage summarystep that renders the tables.gitignore:pytest-results.xmlStdlib only — no extra install step.
if: always()so the summary still renders when tests fail, which is when it's most useful.Rendered output
Verified by running
make test-coveragelocally and executing the step's own script:Plus a collapsible per-file table, least-covered first, so the summary points at what to test next rather than just reporting a number.
Verification
The step was validated by parsing the workflow YAML, extracting the
runblock, and executing that exact block withGITHUB_STEP_SUMMARYset — so what was tested is the heredoc the runner will use, not a copy of it. The embedded Python was also syntax-checked viaast.parse.No production code touched — CI config, one Makefile flag, one gitignore line.
🤖 Generated with Claude Code