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

.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"
 

.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

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"

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
 <https://github.com/JarryShaw/PyPCAPKit/actions/runs/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

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()