Skip to content

ci(coverage): report test coverage from a workflow (#1063) - #1064

Merged
JarryShaw merged 1 commit into
mainfrom
ci/1063-coverage
Oct 6, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
ci/1063-coverage

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 6, 2026 •

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • chore — anything else

Description of your pull request and other information

Closes #1063. Coverage runs inside the existing test leg for Python 3.14 (coverage: true), so no leg is added. That leg had the most headroom on main: 15.6–16.8 min across 3 runs.

  • The pytest selection is unchanged and runs under coverage run --rcfile=.github/coverage.toml. That rcfile repeats pyproject.toml's settings and adds parallel and patch = ["subprocess"], plus core = "ctrace". Under the default sys.monitoring core, the suite's pcapkit re-imports grew test_http_unit.py's peak RSS from 0.40 to 4.17 GiB (ctrace: 0.41), and that killed the runner on the first two heads.
  • Measured locally on tests/corekit/test_multidict.py -n 2: without the patch, only the controller is measured (1 data file, multidict.py 20%). With it, 3 files are combined and multidict.py reaches 100%.
  • Results go to the job summary (total, then a per-package table; the first line flags failed tests), the coverage-html artifact (30 days) and a coverage-summary numbers artifact.
  • The PR comment comes from the new coverage-comment.yml. It runs on workflow_run and runs no tests, so it is consistent with the no-separate-coverage-run ruling. It is not in unit-tests.yml because that file is reusable and its callers grant only contents: read.
  • It never checks out PR code. It takes the PR number from the API (the open PR whose head is the run's head sha, digits only) and renders its own Markdown from validated numbers. It edits one comment on each push.
  • That also covers fork PRs, via the head-owner:branch lookup, but this is unexercised: the repo has no fork PRs. It only fires once the file is on main.
  • Expected overhead with ctrace is about 1.4–1.5x wall time, measured locally on three modules and tests/protocols/application -n 2. That puts the leg at roughly 22–25 min against its 45-min cap.
  • Codecov or Coveralls (PR deltas, history) can be layered on later.

@JarryShaw JarryShaw added ci Pull requests that change CI or workflow configuration (ci: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 611264ba4: NEEDS CHANGES (ran on Sonnet; author Opus)

  • Blocker (probable): coverage-comment asks for pull-requests: write inside unit-tests.yml, which is also a reusable workflow. Its workflow_call callers grant only contents: read: create-release.yml:372-374, deploy-pages.yml:45-47, cron-vendor.yml:47-49 and cron-conda.yml:41-43. GitHub's docs say a called workflow can only downgrade the token's permissions, never raise them. The docs don't spell out how that fails; my reading is a startup failure of every caller, the release gate included. This PR's own pull_request run can't exercise that path. Fix: move the comment job out of the reusable file into a small workflow_run-triggered workflow, which also gives fork PRs a comment.
  • docs/source/contributing/releasing.rst:49 and :59 still cite unit-tests.yml:989-1058 and :962-983. gate and changelog now start at 1108 and 1081.
  • Minor: when test fails, the comment still posts the coverage figure with no sign the tests failed.

Confirmed:

  • Without the subprocess patch, test_multidict.py -n 2 writes 1 data file and covers 20% of multidict.py; with it, 3 files and 100%.
  • Coverage 7.10.0 is the first release with patch = subprocess.
  • The parity test goes red on each mutation of the rcfile.
  • required-checks is unchanged.
  • No ${{ }} inside run:.
  • 15 subprocess-spawning test modules pass identically under the rcfile.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 6, 2026
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on c0d4219ae: GOOD TO GO (ran on Sonnet; author Opus)

  • The blocker is fixed. unit-tests.yml carries only the top-level contents: read, so the four workflow_call callers keep working. The PR comment moved to coverage-comment.yml, which runs on workflow_run, grants only actions: read and pull-requests: write, and checks out no PR code.
  • No injection path was found:
    • Event fields reach the shell only through env:. HEAD_SHA must be hex and the PR number all digits, and the PR number comes from the API, matched on head.sha.
    • The renderer rejected every tampered artifact: markup, newlines, pipes and links in names; strings, bools, floats, negative and oversized values or NaN in counts; and a fake outcome.
  • releasing.rst:49/:59 and the workflows.rst references match the head, and the workflow count (9) and graph edges are right.
  • All 18 tests/project modules pass.
  • Unverified until merge: GitHub fires workflow_run only from main's copy, so the first real comment appears after this lands.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict withdrawn — NEEDS CHANGES on c0d4219ae: the coverage leg kills its runner.

The coverage-measured Python 3.14 leg failed on both heads:

  • 611264ba4: job in run 37404088634, killed at 9m20s.
  • c0d4219ae: job 112084516964, killed at 7m30s, at [ 26%].

Both died in Run unit tests with "The runner has received a shutdown signal". On main the same leg finishes in 15–17 min, so this is caused by the PR, not the runner. It matches #1059's earlier pattern, where per-test pcapkit re-imports pinned in memory exhausted the runner. The likely suspect is memory under coverage with patch = subprocess across the xdist workers. That is an inference, and it is being measured now.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Oct 6, 2026
- unit-tests.yml: the `test` job's Python 3.14 leg (matrix key
  `coverage: true`) runs its unchanged pytest selection under
  `coverage run --rcfile=.github/coverage.toml`, writes the total and a
  per-package table to the job summary, and uploads the HTML report and
  the bare numbers as artifacts. The first line flags failed tests.
- New coverage-comment.yml (workflow_run on Unit Tests, runs no tests)
  turns those numbers into one PR comment, edited on later pushes. It
  stays out of unit-tests.yml because that file is reusable and its
  callers grant only `contents: read`. The PR number comes from the API,
  and the artifact is validated, never executed.
- .github/coverage.toml repeats pyproject.toml's coverage settings and adds
  `parallel` and `patch = ["subprocess"]`, so xdist workers are measured,
  and `core = "ctrace"`: sysmon's per-generation state under the suite's
  pcapkit re-imports OOM-killed the runner;
  tests/project/test_coverage_rcfile.py guards the repetition.
- workflows.rst / releasing.rst: document the above, re-point line refs.

tests/project and tests/test_tier_guard.py pass locally (427 passed).
@JarryShaw JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 412ea8e84: GOOD TO GO (ran on Sonnet; author Opus). It is confirmed by this PR's own coverage leg on CI.

  • The coverage-measured Python 3.14 leg passed. Job 112093769209 ran in 27m31s against the 45 min cap: 2593 passed, 107 skipped, 48362 subtests. That is the leg that killed its runner at 7–9 min on both earlier heads.
  • The total is 89%: 44,383 statements, 3,901 missed, 9,076 branches. Both artifacts uploaded: coverage-summary and coverage-html (5.1 MB).
  • Root cause of the earlier kills: coverage's default sys.monitoring core on 3.14 keeps state per re-imported pcapkit. On test_http_unit it peaked at 3.5–4.2 GiB under sysmon, against 0.41 GiB under core = "ctrace", with arcs and branch counts identical. test_the_core_is_ctrace pins that setting.
  • Still unverifiable until merge: coverage-comment.yml fires on workflow_run only from main's copy, so the first PR comment appears after this lands.
  • Conflicts with ci: cut queue pressure in unit-tests (#1052) #1055: citation hunks in workflows.rst and releasing.rst. Whichever merges second rebases.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 6, 2026
@JarryShaw
JarryShaw merged commit c92a138 into main Oct 6, 2026
76 of 80 checks passed
@JarryShaw
JarryShaw deleted the ci/1063-coverage branch October 6, 2026 04:46
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Pull requests that change CI or workflow configuration (ci: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

ci: report test coverage from a workflow

1 participant