From 8447c8156ec8955986d0f24c86aea61baceaff72 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 7 Oct 2026 19:03:03 -0400 Subject: [PATCH] ci(unit-tests): coverage combine can silently miss a worker's data file, 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. --- .github/coverage.toml | 6 +- .github/workflows/unit-tests.yml | 26 ++++ docs/source/contributing/releasing.rst | 4 +- docs/source/contributing/workflows.rst | 16 +-- tests/project/test_coverage_data_guard.py | 160 ++++++++++++++++++++++ 5 files changed, 201 insertions(+), 11 deletions(-) create mode 100644 tests/project/test_coverage_data_guard.py diff --git a/.github/coverage.toml b/.github/coverage.toml index d581e9c19e..01710f9a3e 100644 --- a/.github/coverage.toml +++ b/.github/coverage.toml @@ -9,7 +9,10 @@ # patch -- "subprocess" (coverage 7.10+) starts measurement in every # child Python process. pytest-xdist's workers are such children # (execnet popen), so without it only the controller is measured -# and the -n auto run reports next to nothing. +# and the -n auto run reports next to nothing. "_exit" saves the +# data before os._exit(), which skips atexit: execnet calls it on +# a worker whose execution outlives shutdown, and pytest-timeout's +# thread method on a hung test (#1387). # core -- "ctrace", not the sys.monitoring default on 3.12+. Tests # purge and re-import pcapkit, and sysmon keeps state for every # generation's code objects: measured on test_http_unit.py @@ -25,6 +28,7 @@ branch = true parallel = true patch = [ "subprocess", + "_exit", ] core = "ctrace" diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index 7b1782e453..406690c1c1 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -145,6 +145,8 @@ jobs: --ignore-glob='*_runtime.py' \ --ignore-glob='*_regression.py' if [ "$COVERAGE" = "true" ]; then + # Logs every measured process and its parent, for the check below. + COVERAGE_DEBUG=pid,process COVERAGE_DEBUG_FILE="$PWD/coverage-processes.log" \ python -m coverage run --rcfile=.github/coverage.toml -m pytest "$@" else python -m pytest "$@" @@ -164,6 +166,30 @@ jobs: TESTS_OUTCOME: ${{ steps.tests.outcome }} run: | set -euo pipefail + # #1387: a worker killed before its atexit save leaves no .coverage.* + # file, and combine then reports the rest as the whole. So fail, and + # post no number, unless the controller and every xdist worker (its + # direct children in the log above) left one. + python - <<'PY' + import glob, os, re, sys + parent, cmd = {}, {} + log = 'coverage-processes.log' + for line in open(log, errors='replace').readlines() if os.path.exists(log) else []: + if m := re.match(r' *(\d+)\.\w+: New process(: cmd| parent pid): (.*)', line): + (parent if m[2] == ' parent pid' else cmd)[m[1]] = m[3] + root = [p for p, c in cmd.items() if "'run', '--rcfile=.github/coverage.toml'" in c] + workers = sorted(p for p, q in parent.items() if root and q == root[0]) + saved = set(re.findall(r'\.pid(\d+)\.', ' '.join(glob.glob('.coverage.*')))) + missing = [p for p in root + workers if p not in saved] + print(f'controller {root}, {len(workers)} xdist workers, missing data: {missing}') + if len(root) != 1 or not workers or missing: + why = f'no data file from pid {missing}' if missing else 'the process log is missing or unreadable' + with open(os.environ['GITHUB_STEP_SUMMARY'], 'a') as summary: + print(f'**Coverage not reported:** {why} (#1387); a total would under-count.', + file=summary) + print(f'::error::Coverage data is incomplete: {why}.') + sys.exit(1) + PY python -m coverage combine --rcfile=.github/coverage.toml python -m coverage report --rcfile=.github/coverage.toml python -m coverage html --rcfile=.github/coverage.toml -d coverage-html diff --git a/docs/source/contributing/releasing.rst b/docs/source/contributing/releasing.rst index db783b658b..3b0c77f9c1 100644 --- a/docs/source/contributing/releasing.rst +++ b/docs/source/contributing/releasing.rst @@ -46,7 +46,7 @@ Editing ``__version__`` by hand without also moving :file:`CITATION.cff`'s (``:494-514``) asserts the two agree, and ``create-release.yml``'s ``unit-tests`` job (``:362-376``) calls ``unit-tests.yml`` with ``gate-only: true``, which runs the **full** suite rather than the tiered -subset an ordinary push runs (its ``gate`` job, ``unit-tests.yml:1163-1232``). +subset an ordinary push runs (its ``gate`` job, ``unit-tests.yml:1189-1258``). That gate runs whenever a release will -- a moved ``__version__`` is exactly what makes ``version_check``'s evidence read "not yet published" -- and every publishing job requires it to have succeeded (``:392,493,573,674``), so a @@ -56,7 +56,7 @@ Either run the script, or move both fields by hand in the same commit. This is the **only** edit a person makes to get a release started -- the tag, the Release, and every upload are the workflow's job from here. Two other checks run unconditionally on the same path, though neither is a step in *this* -process: the ``changelog`` job (``unit-tests.yml:1023-1044``) fails outright if +process: the ``changelog`` job (``unit-tests.yml:1049-1070``) fails outright if ``CHANGELOG.md`` has drifted from its source entry under :file:`docs/source/changelog/`, and the release-body step warns, without failing, if that entry's heading still reads "unreleased" diff --git a/docs/source/contributing/workflows.rst b/docs/source/contributing/workflows.rst index 4a9edc219b..fd4536074e 100644 --- a/docs/source/contributing/workflows.rst +++ b/docs/source/contributing/workflows.rst @@ -245,7 +245,7 @@ and ``project-tests``, and ``required-checks`` (see `Required Status Checks`_ below), gated out by their own ``if:`` rather than by having already run. ``changelog`` carries no ``if:`` at all and runs on every path regardless -- deliberately, per its own -comment (``unit-tests.yml:1011-1017``): ``create-release.yml`` feeds +comment (``unit-tests.yml:1037-1043``): ``create-release.yml`` feeds ``CHANGELOG.md`` to the GitHub Release body, so the release path is exactly where a drifted file must not go unchecked. Confirmed on run `36210743295 `__ (a @@ -384,7 +384,7 @@ grepping every workflow file for its name: - Where * - ``Required checks passed`` - job ``required-checks`` - - ``unit-tests.yml:1320`` + - ``unit-tests.yml:1346`` * - ``Compat Python 3.10`` - job ``compatibility``, matrix leg ``3.10`` - ``python-compatibility.yml:31,41`` @@ -402,20 +402,20 @@ grepping every workflow file for its name: - ``python-compatibility.yml:31,45`` ``Required checks passed`` is defined by exactly one job -(``unit-tests.yml:1320``). ``Compat Python`` is defined by two in +(``unit-tests.yml:1346``). ``Compat Python`` is defined by two in ``python-compatibility.yml``: the required ``compatibility`` job (``:31``, ``Compat Python ${{ matrix.python-version }}``, expanding to the five required legs at ``:41-45``) and the non-required ``compatibility-nightly`` job (``:69``, ``Compat Python 3.15 (scheduled)``, see below). A comment at -``unit-tests.yml:1235`` mentions the string without defining it. +``unit-tests.yml:1261`` mentions the string without defining it. ``Required checks passed`` is an aggregate, not a single check run. The ruleset requires it and the five ``Compat Python 3.10``-``3.14`` legs, nothing else, as -``unit-tests.yml``'s own comment (``unit-tests.yml:1234-1244``) records. It +``unit-tests.yml``'s own comment (``unit-tests.yml:1260-1270``) records. It ``needs:`` the gating jobs of ``unit-tests.yml`` -- ``test``, ``integration``, ``engine-tests`` and ``pypcap-parity``, plus ``changes`` and ``project-tests`` for the docs-only path -- and checks each result explicitly -(``unit-tests.yml:1319-1373``). Rulesets match check names literally, with no +(``unit-tests.yml:1345-1399``). Rulesets match check names literally, with no wildcard, so requiring the matrix cells one by one would mean editing the ruleset whenever a matrix changes; the aggregate keeps the required list fixed. It accepts the four legs as ``skipped`` only on a docs-only pull request, where @@ -424,13 +424,13 @@ instead. The ``Compat`` legs stay required separately because they come from ``python-compatibility.yml``, and a job cannot ``needs:`` a job in another workflow file. This job runs on Unit Tests' own ``push``/``pull_request`` triggers; the ``gate-only: true`` reusable calls documented above skip it via -``if: ${{ always() && inputs.gate-only != true }}`` (``unit-tests.yml:1322``), +``if: ${{ always() && inputs.gate-only != true }}`` (``unit-tests.yml:1348``), so it is never produced -- and never expected -- on those paths. ``Compat Python 3.10``-``3.14`` are five ordinary matrix legs, not an aggregate: ``python-compatibility.yml``'s own comment (``:37-40``) and ``unit-tests.yml``'s matching one for its own, differently-matrixed -``test``/``integration`` jobs (``unit-tests.yml:59-62``, ``unit-tests.yml:230``) both say why 3.15 is +``test``/``integration`` jobs (``unit-tests.yml:59-62``, ``unit-tests.yml:256``) both say why 3.15 is excluded from every *required* matrix in this repository -- the ruleset's required-checks list stops at 3.14, so a 3.15 leg cannot gate a merge and is kept advisory (``continue-on-error: true`` in ``python-compatibility.yml``'s diff --git a/tests/project/test_coverage_data_guard.py b/tests/project/test_coverage_data_guard.py new file mode 100644 index 0000000000..44a652a338 --- /dev/null +++ b/tests/project/test_coverage_data_guard.py @@ -0,0 +1,160 @@ +# -*- coding: utf-8 -*- +"""Tests for the coverage leg's check that no process's data file went missing. + +#1387: PR #1345's coverage leg combined 29 data files where #1341's combined 30, +with every test passing in both, and posted 85.85% against about 88.9%. A +process that dies before coverage's atexit save leaves no ``.coverage.*`` file, +and ``coverage combine`` reports what is left as the whole. So the ``Run unit +tests`` step of :file:`.github/workflows/unit-tests.yml` logs every measured +process (``COVERAGE_DEBUG=pid,process``), and ``Report coverage`` runs a check +before ``coverage combine`` that fails the step, posting no total, unless the +controller and each of its direct children -- the xdist workers -- saved a file. + +The check is extracted from the workflow and run here, both on hand-written +logs and on one written by a real ``coverage run``, so the format it parses is +the one coverage emits rather than one assumed. + +""" + +from __future__ import annotations + +import os +import pathlib +import re +import shutil +import subprocess # nosec: B404 +import sys +import tempfile +import textwrap +import unittest + +ROOT = pathlib.Path(__file__).resolve().parents[2] +WORKFLOW = ROOT / '.github' / 'workflows' / 'unit-tests.yml' +RCFILE = ROOT / '.github' / 'coverage.toml' +LOG = 'coverage-processes.log' +CONTROLLER_CMD = ("['/x/coverage/__main__.py', 'run', '--rcfile=.github/coverage.toml', " + "'-m', 'pytest', '-q', '-n', 'auto']") +#: A ``['-c']`` child's pid in a debug log; coverage pads pids under five digits with spaces. +CHILD = re.compile(r"^ *(\d+)\.\w+: New process: cmd: \['-c'\]", re.M) + + +def _report_step() -> str: + """The ``run:`` text of the ``Report coverage`` step.""" + text = WORKFLOW.read_text(encoding='utf-8') + return text.split('- name: Report coverage', 1)[1].split('\n - name:', 1)[0] + + +def _guard() -> str: + """The Python source of the check, the first heredoc in ``Report coverage``.""" + body = _report_step().split("python - <<'PY'\n", 1)[1].split('\n PY\n', 1)[0] + return textwrap.dedent(body) + + +def _has_coverage() -> bool: + try: + import coverage # pylint: disable=import-outside-toplevel + except ImportError: + return False + return tuple(int(x) for x in coverage.__version__.split('.')[:2]) >= (7, 10) + + +def _entry(pid: int, parent: int, cmd: str) -> str: + # coverage right-aligns the pid prefix to a fixed width, so short pids lead with spaces + return (f'{pid:5d}.ab12: New process: pid={pid}, executable: /usr/bin/python\n' + f'{pid:5d}.ab12: New process: cmd: {cmd}\n' + f'{pid:5d}.ab12: New process parent pid: {parent}\n') + + +class CoverageDataGuardTests(unittest.TestCase): + """The check fails exactly when the controller or an xdist worker saved nothing.""" + + def setUp(self) -> None: + self.tempdir = pathlib.Path(tempfile.mkdtemp(prefix='coverage-guard-')) + self.addCleanup(shutil.rmtree, self.tempdir, ignore_errors=True) + self.summary = self.tempdir / 'summary.md' + self.summary.touch() + + def run_guard(self) -> 'subprocess.CompletedProcess[str]': + env = dict(os.environ, GITHUB_STEP_SUMMARY=str(self.summary)) + return subprocess.run([sys.executable, '-c', _guard()], cwd=self.tempdir, env=env, # nosec: B603 + capture_output=True, text=True, check=False, timeout=60) + + def write(self, log: str, pids: 'list[int]') -> None: + (self.tempdir / LOG).write_text(log, encoding='utf-8') + for pid in pids: + (self.tempdir / f'.coverage.runner.pid{pid}.Xabcdefgh.Habcdefghijkh').touch() + + def assert_fails(self, why: str) -> None: + result = self.run_guard() + self.assertEqual(result.returncode, 1, result.stdout + result.stderr) + self.assertIn(f'::error::Coverage data is incomplete: {why}', result.stdout) + self.assertIn('**Coverage not reported:**', self.summary.read_text(encoding='utf-8')) + + def test_the_check_runs_before_combine(self) -> None: + """``combine`` deletes the data files it merges, so the check must come first.""" + step = _report_step() + self.assertLess(step.index('#1387'), step.index('coverage combine')) + tests = WORKFLOW.read_text(encoding='utf-8').split('- name: Run unit tests', 1)[1] + self.assertRegex(tests.split('- name:', 1)[0], + r'COVERAGE_DEBUG=pid,process COVERAGE_DEBUG_FILE="\$PWD/' + re.escape(LOG)) + + def test_a_complete_run_passes(self) -> None: + """A child of a worker that saved nothing is not the controller's or a worker's loss.""" + log = (_entry(100, 1, CONTROLLER_CMD) + _entry(101, 100, "['-c']") + + _entry(102, 100, "['-c']") + _entry(200, 101, "['-c']")) + self.write(log, [100, 101, 102]) + result = self.run_guard() + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertIn("2 xdist workers, missing data: []", result.stdout) + self.assertEqual(self.summary.read_text(encoding='utf-8'), '') + + def test_a_missing_worker_fails(self) -> None: + """#1345's case: one worker's file absent, every test passed.""" + log = _entry(100, 1, CONTROLLER_CMD) + _entry(101, 100, "['-c']") + _entry(102, 100, "['-c']") + self.write(log, [100, 101]) + self.assert_fails("no data file from pid ['102']") + + def test_a_missing_controller_fails(self) -> None: + self.write(_entry(100, 1, CONTROLLER_CMD) + _entry(101, 100, "['-c']"), [101]) + self.assert_fails("no data file from pid ['100']") + + def test_a_log_naming_no_worker_fails(self) -> None: + """A blind check -- the log format changed, or debug output went elsewhere -- fails.""" + self.write(_entry(100, 1, CONTROLLER_CMD), [100]) + self.assert_fails('the process log is missing or unreadable') + + def test_a_missing_log_fails_without_a_traceback(self) -> None: + """No log at all -- the test step died before writing one -- is the same clean failure.""" + result = self.run_guard() + self.assertNotIn('Traceback', result.stderr) + self.assert_fails('the process log is missing or unreadable') + + @unittest.skipUnless(_has_coverage(), 'needs coverage 7.10+ (`[run] patch`)') + def test_the_log_coverage_writes_is_the_one_parsed(self) -> None: + """A real ``coverage run`` whose child stands in for an xdist worker.""" + (self.tempdir / '.github').mkdir() + shutil.copy(RCFILE, self.tempdir / '.github' / 'coverage.toml') + (self.tempdir / 'spawn.py').write_text( + 'import subprocess, sys\nsubprocess.run([sys.executable, "-c", "pass"], check=True)\n', + encoding='utf-8') + env = {k: v for k, v in os.environ.items() if not k.startswith('COVERAGE')} + env.update(COVERAGE_DEBUG='pid,process', COVERAGE_DEBUG_FILE=str(self.tempdir / LOG)) + subprocess.run([sys.executable, '-m', 'coverage', 'run', '--rcfile=.github/coverage.toml', # nosec: B603 + 'spawn.py'], cwd=self.tempdir, env=env, check=True, timeout=120, + capture_output=True) + files = sorted(self.tempdir.glob('.coverage.*')) + self.assertEqual(len(files), 2, files) + self.assertEqual(self.run_guard().returncode, 0) + + # dropping the child's file is what a lost worker looks like + child = CHILD.search((self.tempdir / LOG).read_text(encoding='utf-8'))[1] + next(f for f in files if f'.pid{child}.' in f.name).unlink() + self.assert_fails(f"no data file from pid ['{child}']") + + def test_the_child_lookup_allows_a_padded_pid(self) -> None: + """The real-log test above finds its child under a pid shorter than the padding.""" + self.assertEqual(CHILD.search(_entry(9832, 9800, "['-c']"))[1], '9832') + + +if __name__ == '__main__': + unittest.main()