Skip to content

ci(unit-tests): coverage combine can silently miss a worker's data file, under-reporting a PR's coverage - #1389

Merged
JarryShaw merged 3 commits into
mainfrom
w1387-coverage-guard
Oct 8, 2026
Merged

JarryShaw merged 3 commits into
mainfrom
w1387-coverage-guard

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 7, 2026 •

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort) — not run
  • make test passes, and a test case covers the change — tests/project only: 397 passed, 1 skipped
  • Added a changelog entry — N/A, added centrally

What is the purpose of your pull request?

  • ci — workflows or build tooling

Closes #1387

Guard (fails the step). The test step now logs every measured process with its parent (COVERAGE_DEBUG=pid,process). Before combine, Report coverage fails and posts no total unless the controller and each of its direct children (the xdist workers) left a .coverage.* file. The set is read from the run's own log, so -n auto needs no hard-coded count. A missing child of a worker is not flagged: the 20 duplicate-hash "skipped" files are such children. If the log names no worker, the check also fails, so it cannot pass while blind. Why fail rather than warn: coverage-comment.yml accepts only four tests values, so a warning marker would need an edit there. With no artifact, it already posts a notice and exits 0.

Mechanism: still not traced. I ran 3 local -n 4 runs (2 pinned to 4 CPUs), and every worker saved each time (47 files). The one candidate I measured is mostly ruled out: xdist SIGKILLs a worker 10 s after gw.exit(), but workers saved 2–3 ms after teardown began, and each save took about 0.3 s. Adding patch = ["_exit"] covers the documented os._exit paths: execnet's shutdown fallback (gateway_base.py:1253) and pytest-timeout's thread method. sigterm = true was not added, because nothing in the chain sends SIGTERM, and coverage.py's docs warn it can cause problems (issue 1310).

Tests. New tests/project/test_coverage_data_guard.py (8 tests) runs the extracted guard on synthetic logs and on a real coverage run log. All 8 pass. Mutation checks: the unpadded child regex, the round-1 guard on a missing log, and a warn-only guard each fail their test. The real-log test caught two parse bugs (New process: cmd: and space-padded pids). The guard also passed on all 3 real local runs. The citation shift (+25 lines) is in workflows.rst/releasing.rst.

@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 7, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 2958401f7: NEEDS CHANGES. One test is flaky (Sonnet reviewed; author Opus).

  • The flaky test. The regex in tests/project/test_coverage_data_guard.py:142, ^(\d+)\.\w+: New process: cmd:, does not allow for the leading spaces coverage adds to pids under 5 digits. The fixture at :61 writes {pid:5d}, and the workflow guard's own regex (' *(\d+)') handles them. With child pids 9832 and 9836, re.search returned None and the test died with a TypeError. It will fail on any runner whose pids are below 10000. Fix: ^ *(\d+)\..
  • Guard on a real run. It passes a real xdist run: -n 4 on tests/project gives 395 passed, 1 skipped, the controller plus 4 workers, and nothing missing. Workers that run zero tests still save a file. A killed worker fails the guard, which is correct. A log with no workers exits 1.
  • patch = ["_exit"]. It is supported from coverage 7.10.0, which matches the CI pin coverage>=7.10. An unknown key raises ConfigError, so a typo fails loudly. The execnet and pytest-timeout paths are UNVERIFIED.
  • Logging env. With it set, tests/project gives 395/1 skipped, identical to a run without it. The author's local failures did not reproduce.
  • Permissions and docs. No write permission is added. All the shifted line citations in workflows.rst and releasing.rst match.

Notes, not blocking:

  • The revert proof fails on the missing heredoc, which is weak evidence.
  • A missing coverage-processes.log gives a traceback rather than ::error::.
  • A guard failure on the 3.14 leg now blocks Required checks passed. Before this PR it only under-reported coverage.

@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 7, 2026
…le, under-reporting a PR's coverage

- Run unit tests: the coverage leg logs every measured process and its
  parent (COVERAGE_DEBUG=pid,process to coverage-processes.log).
- Report coverage: before combine, fail the step and post no total unless
  the controller and each xdist worker (its direct children) left a
  .coverage.* file; the expected set is derived from the run's own log.
  A missing or unreadable log fails the same way.
- coverage.toml: add patch "_exit", so a worker ended by os._exit()
  (execnet's shutdown fallback, pytest-timeout's thread method) still saves.
- Shift the unit-tests.yml line citations in workflows.rst/releasing.rst.

tests/project passes: 397 passed, 1 skipped.
@JarryShaw
JarryShaw force-pushed the w1387-coverage-guard branch from 2958401 to 8447c81 Compare October 7, 2026 23:19
@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 7, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Round-2 verdict on 8447c8156: GOOD TO GO (Sonnet reviewed; author Opus). All three round-1 findings are fixed.

  • Padded pids. Every lookup now goes through the CHILD regex (^ *(\d+)\.). Coverage really does pad the pid, as f"{os.getpid():5d}…" at coverage/debug.py:320 shows. CHILD matches 7, 123 and 12345 with or without padding, while the old regex returned None on the padded line.
  • Missing log. A missing coverage-processes.log now gives ::error::, adds the summary line and exits 1, with no traceback.
  • Revert proof. This is now strong. The reviewer wrote their own mutants:
    • a warn-only guard fails 4 tests;
    • the old regex errors the padded-pid test;
    • dropping the exists check fails 1 test;
    • dropping or missing fails 2 tests.
  • Citations. All 16 shifted unit-tests.yml line citations match the new file, and no stale citation remains anywhere in docs/, .github/ or util/.
  • Test runs. The guard module passes 8, and tests/project gives 397 passed, 1 skipped. I re-ran the guard module myself at this head and got 8/0.

Nits, not blocking:

  • A log that exists but cannot be read still raises before the ::error:: line. The step still fails, so only the message is worse.
  • open(...).readlines() never closes the file handle.

@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 7, 2026
@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 89.40% (unit tier, Python 3.14, bc7e19685, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18791 968 2352 869 90.90%
pcapkit/corekit 2018 88 628 24 94.78%
pcapkit/dumpkit 234 2 84 2 98.74%
pcapkit/foundation 2573 99 910 44 94.63%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 16262 203 4296 165 98.17%
pcapkit/toolkit 539 74 168 3 84.58%
pcapkit/utilities 429 4 122 4 98.55%
pcapkit/vendor 4409 2342 1006 157 43.25%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw

Copy link
Copy Markdown
Owner Author

The red Python 3.x legs are main's, not this diff. test_protocol_make_unit.py::TestMakeRawItems::test_unparsable_raw_item_raises_protocol_error also fails on main at 66950b1f2, where I reproduced it locally. It is a semantic conflict between #1344 and #1352, tracked in #1394, and a fix is in progress. This PR needs a re-run once #1394 lands.

@JarryShaw
JarryShaw merged commit e320d75 into main Oct 8, 2026
36 checks passed
@JarryShaw
JarryShaw deleted the w1387-coverage-guard branch October 8, 2026 02:34
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 8, 2026
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 26, 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(unit-tests): coverage combine can silently miss a worker's data file, under-reporting a PR's coverage

1 participant