fix: include serial tests in coverage.xml and upload it from CI - #373
fix: include serial tests in coverage.xml and upload it from CI#373runpod-Henrik wants to merge 9 commits into
Conversation
77bbea2 to
8dd2f16
Compare
deanq
left a comment
deanq
left a comment
There was a problem hiding this comment.
A few robustness notes on the coverage plumbing. The core fix (re-emitting coverage.xml after the serial --cov-append pass) is right; these are edge cases around failing/aborted runs.
| if not p.exists(): | ||
| o += [f"> No coverage report at `{cov}`."] | ||
| else: | ||
| r = ET.parse(p).getroot() |
There was a problem hiding this comment.
The coverage-XML parse here isn't guarded, unlike the junit parse above (the try/except ET.ParseError at lines 269-272). Because this step runs under if: always(), a truncated or malformed coverage.xml — a run killed mid-write by timeout, OOM, or an xdist worker crash — makes ET.parse(p) raise, so this summary step exits non-zero and stacks a spurious failure on top of the real one. Consider wrapping this in the same try/except ET.ParseError and falling back to the "no coverage report" branch.
There was a problem hiding this comment.
Confirmed and fixed. Reproduced it: the old script exits 1 on a truncated coverage.xml, the new one exits 0 and reports the parse error in the summary instead.
Your framing sent me looking at the rest of the script and I found the same class of bug in the half I had written a guard for — the junit attributes were still going through bare int(), so a malformed tests="abc" raised too. Both now go through a coercing helper. Also fixed while there: a suite dying at import reports 0 failures having run nothing, so the status cell rendered a crashed suite as ✅; it now requires that something actually ran.
| with: | ||
| name: coverage-${{ matrix.python-version }} | ||
| path: coverage.xml | ||
| if-no-files-found: error |
There was a problem hiding this comment.
if-no-files-found: error combined with if: always() means any run where coverage.xml was never produced — e.g. pytest errors at collection/import before pytest-cov writes the file — fails this upload step, red-flagging the job and masking the root cause. On main it also stops that run from counting as "successful," which (per the comment above) the weekly coverage report depends on to locate its artifact. Consider warn instead of error so a missing report doesn't manufacture a second failure or starve the weekly report.
There was a problem hiding this comment.
Changed to warn — agreed on the substance. This upload hangs off the PR-gating job, so it shouldn't be able to fail a PR on its own, and it was masking the root cause.
One correction on the second half: a failed upload can't starve the weekly report. If the upload fails there's no artifact either way, and the collector only ever reads runs whose status is success — so both paths land on "not re-measured" rather than a wrong number. The masked root cause was the real cost, which is why the change stands.
Kept as error on main-ui and RunPod, where this is a dedicated nightly whose only purpose is the artifact.
| # Re-emit the XML after appending the serial tests, or coverage.xml is left | ||
| # holding only the parallel run and undercounts. | ||
| uv run pytest tests/ --junitxml=pytest-results-serial.xml -v -m "serial" --cov=runpod_flash --cov-append --cov-report=term-missing --cov-report=xml | ||
| @echo "::endgroup::" |
There was a problem hiding this comment.
Because the parallel line above (pytest ... -m "not serial") exits non-zero on any test failure, make aborts before reaching this serial pass — so on a failing PR coverage.xml still holds only the parallel subset and undercounts, which is the exact bug this change is fixing. The re-emit only takes effect when every parallel test passes. If the goal is an accurate number even on failing runs, the parallel invocation needs to not abort the recipe (e.g. a - / || true guard) so the serial re-emit always runs. Flagging in case the undercount-on-failure case is the one that matters.
There was a problem hiding this comment.
Confirmed, and I've documented it — but I'd push back on the remedy. Guarding the parallel line with -/|| true would make the recipe's exit status come from the serial pass, so a red parallel suite could exit 0. Silently passing a failing suite is worse than an undercount on a run that already failed.
The undercount is also unconsumed: the weekly report only reads runs whose status is success, so a red run's coverage.xml is never read. The fix does what it needs to on green runs, which are the only ones that matter here. Written into the Makefile so the next reader doesn't have to re-derive it.
| # Re-emit the XML after appending the serial tests, or coverage.xml is left | ||
| # holding only the parallel run and undercounts. | ||
| uv run pytest tests/ --junitxml=pytest-results-serial.xml -v -m "serial" --cov=runpod_flash --cov-append --cov-report=term-missing --cov-report=xml | ||
| @echo "::endgroup::" |
There was a problem hiding this comment.
The parallel invocation passes --cov-fail-under=0 to suppress the partial-coverage gate, but this serial line doesn't, so it inherits --cov-fail-under=65 from pyproject addopts and gates CI on combined coverage. That's likely intended — the serial pass is where the full number exists — but the asymmetry is undocumented. Worth a short comment noting the serial pass is the real coverage gate, so it isn't later "aligned" to the parallel line and the gate quietly disabled.
There was a problem hiding this comment.
Agreed, documented. The comment now states that the serial line is the coverage gate and that aligning the two flags would quietly disable it.
The quality-gates matrix declared 3.10/3.11/3.12/3.13 but every leg ran
Python 3.11. `make dev` runs a bare `uv sync`, and uv resolves its
interpreter from `.python-version` (pinned to 3.11) rather than from the
one actions/setup-python just installed. Confirmed across four CI runs on
three different branches, the oldest from 2026-08-10: every leg reports
`Using CPython 3.11.x` and builds `.venv/lib/python3.11`.
UV_PYTHON takes precedence over the pin file. Set at job level rather
than on the install step because `make ci-quality-github` runs
`uv run pytest`, which resolves the interpreter again.
uv.lock is re-locked in the same commit because it was stale in two ways
and 3.13 could not resolve without it:
* `requires-python = ">=3.10, <3.13"` while pyproject.toml says
`>=3.10,<3.14`, so the lock never covered 3.13 at all
* `tomlkit>=0.13.0` is declared in pyproject.toml but was missing from
the runpod-flash dependency and requires-dist lists
`uv lock --check` fails against the previous lock and passes against this
one, for each of 3.10/3.11/3.12/3.13. CI did not catch the drift because
pre-check uses `--frozen` (which does not verify freshness) and `make dev`
uses plain `uv sync`, which silently re-resolves on the runner.
Verified: the full suite on a real 3.13 interpreter gives 2680 passed,
with no 3.13-specific failures.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
test_deploy_all_background was the source of a flaky failure that hit an
unrelated test, most often
FAILED tests/unit/test_regressions.py::TestREG008NonInteractiveEnvDelete
::test_undeploy_resource_force_remove_no_tty
_pickle.PicklingError: args[0] from __newobj__ args has the wrong class
deploy_all_background() starts a daemon thread and returns immediately,
and the test never joined it. The `patch.object(..., "get_or_deploy_resource")`
context was therefore lifted while the thread was still starting, so the
thread ran against the unpatched manager and registered the fixture's
MagicMock(spec=ServerlessResource) objects into the ResourceManager
singleton -- during whichever unrelated test happened to be running at
that moment.
ResourceManager._save_resources() cloudpickles its entire state on every
registration, and a MagicMock cannot be pickled: `obj.__class__` is the
spec'd class while `type(obj)` is MagicMock, which is exactly the
mismatch pickle.save_reduce rejects. The mock arrived as a dict *key*,
via _migrate_to_name_based_keys() calling `resource.get_resource_key()`
on it, so the offending key rendered as
`<MagicMock name='mock.get_resource_key()'>`.
Because it depended on thread scheduling and on xdist's dynamic worker
assignment, the victim, the worker and the matrix leg all varied per run:
the same failure appears on three unrelated branches, on legs 3.10, 3.12
and 3.13 and on workers gw0, gw2 and gw3. Forcing everything into one
process reproduced it every time; `-n 4` reproduced it in 0 of 6 runs.
threading.Thread is now stubbed, which keeps what this test actually
asserts -- the call is non-blocking and spawns a daemon thread -- and lets
nothing escape. The test previously asserted nothing at all; its own
comment said "not much we can test here without waiting for thread".
Verified: full suite single-process, 2680 passed, 0 PicklingError (the
same run reproduced the failure before this change). The two remaining
local failures are TestVersionFlag, which is colour-dependent and passes
both in CI and locally under NO_COLOR=1.
Two deeper faults are left alone here, as they are wider changes:
ResourceManager._resources / _resource_configs are class variables that
instance mutation writes through, and conftest's worker_flash_dir is
scope="session", so every test in a worker shares one resources.pkl.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Making the version matrix real (previous commit) exposed three failures on
Python 3.10 that had been masked for months. All three are test artifacts,
not product bugs, and all three reproduce on a real 3.10 interpreter
independently of this branch.
tests/unit/cli/commands/test_run_server_helpers.py
call_with_body -> _map_body_to_params calls inspect.signature(func), and
signature() of a Mock is version-dependent: on 3.10 it raises
`TypeError: 'Mock' object is not subscriptable` (Mock auto-creates a
__signature__ child which inspect then tries to use), while on 3.11+ it
reports (*args, **kwargs). Passing spec= does not help -- verified; the
failure is in signature() itself.
call_with_body catches Exception and converts it into a 500 JSONResponse,
so the visible failure was the uninformative
`assert <JSONResponse object> == {'ok': True}`, with the real TypeError
readable only inside the response body.
The mock is now wrapped in a real `async def (*args, **kwargs)` -- the
same signature 3.11+ inferred from the Mock, so both branches of
_map_body_to_params behave exactly as before -- and delegating to the mock
keeps `assert_called_once_with()`.
tests/unit/core/resources/test_resource_manager_extended.py
test_loads_legacy_dict_format cloudpickle.dump()s a MagicMock, which 3.10
rejects with "Could not pickle object as excessively deep recursion
required". Replaced with a module-level _LegacyResource: only .config_hash
is read (via _refresh_config_hashes), and the *absence* of
get_resource_key is what keeps the legacy key un-migrated, which is what
this test asserts. Module-level rather than defined in the test body,
since cloudpickle serialises a function-local class by value.
Verified on 3.10, 3.11 and 3.13: 40 passed in both affected files on each,
where 3.10 previously failed all three tests. ruff format and check clean.
Worth following up separately: the blanket `except Exception ->
JSONResponse(500)` in call_with_body turns genuine errors into silent 500s,
which is what made this take three steps to diagnose.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The coverage run is split in two: parallel tests with --cov-report=xml, then serial tests with --cov-append but only --cov-report=term-missing. The second run appends to .coverage without re-emitting the XML, so coverage.xml was left holding the parallel run alone. Measured locally: 85.1% (8085/9503) before the append is re-emitted vs 85.6% (8132/9503) after — 47 covered lines from the 19 serial-marked tests were being dropped from the report. Also upload coverage.xml so the weekly coverage report can read the number from the newest successful run on main instead of it being collected by hand. `if-no-files-found: error` so a silently-missing report fails the step rather than publishing an empty artifact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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. Reads both pytest-results-parallel.xml and pytest-results-serial.xml, so the counts cover the whole suite rather than one pass. 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>
Addresses all four review comments on this PR.
Summary step (same three defects, all reachable because the 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 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 reports it in the summary.
* 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.
Upload now uses `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 too, masking the root cause. This
upload hangs off the PR-gating job, so it should not fail a PR on its own.
Makefile: documented the two asymmetries flagged in review.
* the serial line, not the parallel one, is the real coverage gate: the
parallel pass passes --cov-fail-under=0 while the serial pass inherits
65 from pyproject addopts. Aligning the two flags would quietly
disable the gate, so that is now written down.
* make stops at the first failing recipe line, so a failing parallel pass
means the serial re-emit never runs and coverage.xml holds the parallel
subset. Left as-is deliberately rather than guarding with `-`/`|| true`:
that would let a red parallel suite exit 0, which is worse than an
undercount on a run that already failed. The weekly report only reads
runs whose status is success, so nothing consumes that undercount.
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. Purely additive: line coverage is unchanged (verified per repo), so the weekly trend, which reads line coverage, is unaffected. The Cobertura report now carries branches-valid/covered, which the summary renders as its own row. Note --cov-fail-under gates on coverage.py total, which now blends lines and branches, so that number drops even though line coverage does not. Measured before committing: this repo stays comfortably above its gate. 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>
3ad315f to
eb12ff8
Compare
2 participants
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 19serial-marked tests 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.github/workflows/ci.yml: uploadcoverage.xmlascoverage-${{ matrix.python-version }}The artifact name is per-matrix-version because
upload-artifact@v4requires unique names within a run.if-no-files-found: errorso a silently-missing report fails the step rather than publishing an empty artifact.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) in a clean worktree at this commit:ruff format --check(258 files) andruff checkboth clean. The resultingcoverage.xmlparses to 8122/9503 lines.No production code touched — CI config and Makefile flags only.
🤖 Generated with Claude Code