From 7eef5df56d74c08da8124327aeb78a339c623d0f Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 8 Oct 2026 10:32:15 -0400 Subject: [PATCH] ci(unit-tests): save each xdist worker's coverage at session finish and keep the process log (#1423) - tests/conftest.py: a pytest_sessionfinish hook saves an xdist worker's auto-started coverage before shutdown, so a worker that dies after reporting its results no longer loses its whole data file. - .github/coverage.toml: sigterm = true, so a SIGTERM saves the data too. - unit-tests.yml: the guard names each missing pid's command line and whether its atexit save ran; coverage-processes.log is now uploaded as the coverage-processes-log artifact on every coverage leg. - docs: unit-tests.yml citations renumbered by line content; the coverage section mentions the save and the new artifact. tests/project: 404 passed, 1 skipped; the 5 new tests fail without the fix. --- .github/coverage.toml | 9 ++++ .github/workflows/unit-tests.yml | 27 +++++++++- docs/source/contributing/releasing.rst | 4 +- docs/source/contributing/workflows.rst | 27 ++++++---- tests/conftest.py | 45 +++++++++++++++++ tests/project/test_coverage_data_guard.py | 61 +++++++++++++++++++++++ tests/project/test_coverage_rcfile.py | 4 ++ 7 files changed, 163 insertions(+), 14 deletions(-) diff --git a/.github/coverage.toml b/.github/coverage.toml index 01710f9a3e..7ab942f2bc 100644 --- a/.github/coverage.toml +++ b/.github/coverage.toml @@ -13,6 +13,14 @@ # 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). +# sigterm -- installs a SIGTERM handler that saves the data. `patch = +# ["_exit"]` covers os._exit and atexit covers a clean exit, but +# nothing covered a signal, so a worker SIGTERMed during +# shutdown would leave no file -- one candidate for #1423, whose +# actual cause is not established. Does not help against +# SIGKILL -- execnet's post-timeout kill is `Popen.kill()` -- +# which is why `tests/conftest.py` also saves at +# `pytest_sessionfinish` rather than relying on this alone. # 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 @@ -30,6 +38,7 @@ patch = [ "subprocess", "_exit", ] +sigterm = true core = "ctrace" [tool.coverage.report] diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index 406690c1c1..8846cc9ad6 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -172,16 +172,26 @@ jobs: # direct children in the log above) left one. python - <<'PY' import glob, os, re, sys - parent, cmd = {}, {} + parent, cmd, atexit_ = {}, {}, {} 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] + elif m := re.match(r' *(\d+)\.\w+: (atexit|sigterm): pid: \d+, (.*)', line): + atexit_[m[1]] = f'{m[2]}: {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}') + # #1423: which process it was, and whether coverage's atexit handler ran + # in it at all -- a pid with no `atexit:` line died on a signal before + # saving, one with such a line reached the save and lost it afterwards. + # Those are different bugs, and the log that distinguishes them is kept + # as an artifact by the step below rather than only read here. + for p in missing: + print(f' pid {p}: cmd {cmd.get(p, "")}') + print(f' pid {p}: {atexit_.get(p, "no atexit/sigterm entry -- died before saving")}') 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: @@ -227,6 +237,21 @@ jobs: print('Per-file detail: the `coverage-html` artifact of this run.') PY + # #1423: the guard above fires on runs whose tests all pass, and the only + # record of what became of the lost process is this log -- it carries each + # measured process's command line, its parent, and the `atexit:` line + # coverage writes as it saves. Reading it off the runner is impossible once + # the job ends, so keep it whenever the leg ran, not only on failure: a run + # that passed is the baseline the failing one has to be read against. + - name: Upload coverage process log + if: ${{ !cancelled() && matrix.coverage }} + uses: actions/upload-artifact@v7 + with: + name: coverage-processes-log + path: coverage-processes.log + retention-days: 30 + if-no-files-found: warn + - name: Upload coverage HTML report if: ${{ !cancelled() && matrix.coverage }} uses: actions/upload-artifact@v7 diff --git a/docs/source/contributing/releasing.rst b/docs/source/contributing/releasing.rst index 3b0c77f9c1..8812340449 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:1189-1258``). +subset an ordinary push runs (its ``gate`` job, ``unit-tests.yml:1214-1283``). 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:1049-1070``) fails outright if +process: the ``changelog`` job (``unit-tests.yml:1074-1095``) 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 fd4536074e..5b546f18cf 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:1037-1043``): ``create-release.yml`` feeds +comment (``unit-tests.yml:1062-1068``): ``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 @@ -352,9 +352,14 @@ and ``patch = ["subprocess"]``, without which only the xdist controller is measured, not its workers, plus ``core = "ctrace"``: with the suite purging and re-importing pcapkit, one module's peak RSS was 4.17 GiB under the default ``sys.monitoring`` core against 0.41 GiB under ``ctrace``, and that growth -killed the runner. The leg then writes the total and a per-package table to the -job summary and uploads the HTML report as the ``coverage-html`` -artifact (30 days), with the bare numbers as the ``coverage-summary`` artifact. +killed the runner. ``sigterm = true`` and a ``pytest_sessionfinish`` hook in +:file:`tests/conftest.py` that saves each xdist worker's data early keep a +worker that dies during shutdown from losing its file +(`#1423 `__). The leg then +writes the total and a per-package table to the job summary and uploads the HTML +report as the ``coverage-html`` artifact (30 days), with the bare numbers as the +``coverage-summary`` artifact and the per-process debug log as +``coverage-processes-log``. The PR comment comes from **Coverage Comment** (:file:`coverage-comment.yml`), not from ``unit-tests.yml``. That file is also a reusable workflow, a called @@ -384,7 +389,7 @@ grepping every workflow file for its name: - Where * - ``Required checks passed`` - job ``required-checks`` - - ``unit-tests.yml:1346`` + - ``unit-tests.yml:1371`` * - ``Compat Python 3.10`` - job ``compatibility``, matrix leg ``3.10`` - ``python-compatibility.yml:31,41`` @@ -402,20 +407,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:1346``). ``Compat Python`` is defined by two in +(``unit-tests.yml:1371``). ``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:1261`` mentions the string without defining it. +``unit-tests.yml:1286`` 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:1260-1270``) records. It +``unit-tests.yml``'s own comment (``unit-tests.yml:1285-1295``) 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:1345-1399``). Rulesets match check names literally, with no +(``unit-tests.yml:1370-1424``). 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 +429,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:1348``), +``if: ${{ always() && inputs.gate-only != true }}`` (``unit-tests.yml:1373``), 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:256``) both say why 3.15 is +``test``/``integration`` jobs (``unit-tests.yml:59-62``, ``unit-tests.yml:281``) 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/conftest.py b/tests/conftest.py index 205ac5bfac..a8248280ba 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -168,6 +168,51 @@ def pytest_sessionstart(session: 'pytest.Session') -> 'None': _pin_module_snapshot() +def pytest_sessionfinish(session: 'pytest.Session') -> 'None': + """Write an xdist worker's coverage data out before the process can die (#1423). + + The coverage leg of ``unit-tests.yml`` measures each worker through + coverage's ``patch = ["subprocess"]`` hook, and that measurement is saved by + an :mod:`atexit` handler -- so it is written *after* the worker has already + reported its results and told the controller it is finished. Anything that + kills the process in that window takes the whole worker's data with it, and + takes it silently: pytest has its results, so every test still passes, while + ``coverage combine`` merges one file fewer and reports the rest as the whole. + That is #1387, and the guard in ``Report coverage`` turns it into a failed + step rather than a quietly low number -- four times on 2026-10-08. + + Saving here closes the window instead of policing it. :func:`cov.save` is + safe to call mid-run (coverage's own long-running-process recipe), and the + later ``atexit`` save appends to the same file, so the data file exists from + this point on and a worker that dies during shutdown costs at most the arcs + executed after this hook. + + A no-op outside that one leg: it returns immediately unless the process is + an xdist worker (``workerinput``) whose coverage was auto-started by the + subprocess patch. + + Args: + session: The pytest session, for its config's ``workerinput``. + + """ + if not hasattr(session.config, 'workerinput'): + return + try: + import coverage # pylint: disable=import-outside-toplevel + except ImportError: # pragma: no cover + return + cov = getattr(coverage.process_startup, 'coverage', None) + if cov is None: + return + try: + cov.save() + except Exception: # pragma: no cover # pylint: disable=broad-except + # Deliberately broad, as in `pytest_sessionstart`: a failure to flush + # early must not fail a run whose tests have already passed. The atexit + # save still gets its turn, and the guard still catches a lost file. + pass + + @pytest.fixture(autouse=True) def restore_module_table() -> 'Iterator[None]': """Put the :mod:`pcapkit` region of :data:`sys.modules` back to a known-good state. diff --git a/tests/project/test_coverage_data_guard.py b/tests/project/test_coverage_data_guard.py index 44a652a338..2def3f2d24 100644 --- a/tests/project/test_coverage_data_guard.py +++ b/tests/project/test_coverage_data_guard.py @@ -26,7 +26,13 @@ import sys import tempfile import textwrap +import types +import typing import unittest +import unittest.mock + +if typing.TYPE_CHECKING: + import pytest ROOT = pathlib.Path(__file__).resolve().parents[2] WORKFLOW = ROOT / '.github' / 'workflows' / 'unit-tests.yml' @@ -155,6 +161,61 @@ 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') + def test_a_missing_worker_is_named_with_its_fate(self) -> None: + """#1423: the failure says which process it was and whether its atexit save ran.""" + log = (_entry(100, 1, CONTROLLER_CMD) + _entry(101, 100, "['-c']") + + _entry(102, 100, "['-c']") + _entry(103, 100, "['-c']") + + ' 102.ab12: atexit: pid: 102, instance: \n') + self.write(log, [100, 101]) + result = self.run_guard() + self.assertIn("pid 102: cmd ['-c']", result.stdout) + self.assertIn('pid 102: atexit: instance: ', result.stdout) + self.assertIn('pid 103: no atexit/sigterm entry -- died before saving', result.stdout) + self.assert_fails("no data file from pid ['102', '103']") + + def test_the_process_log_is_kept_as_an_artifact(self) -> None: + """The log is the only record of a lost worker's fate once the runner is gone (#1423).""" + text = WORKFLOW.read_text(encoding='utf-8') + step = text.split('- name: Upload coverage process log', 1)[1] + step = step.split('\n - name:', 1)[0] + self.assertIn('if: ${{ !cancelled() && matrix.coverage }}', step) + self.assertIn(f'path: {LOG}', step) + + +class WorkerSessionFinishSaveTests(unittest.TestCase): + """#1423: an xdist worker writes its data at session finish, not only at exit.""" + + def finish(self, *, worker: bool) -> int: + """Run the conftest hook under a stand-in auto-started coverage; return its saves.""" + import coverage # pylint: disable=import-outside-toplevel + + from tests import conftest # pylint: disable=import-outside-toplevel + + cov = unittest.mock.Mock() + sentinel = object() + before = getattr(coverage.process_startup, 'coverage', sentinel) + coverage.process_startup.coverage = cov # type: ignore[attr-defined] + try: + config = types.SimpleNamespace(**({'workerinput': {}} if worker else {})) + conftest.pytest_sessionfinish(typing.cast('pytest.Session', + types.SimpleNamespace(config=config))) + finally: + if before is sentinel: + del coverage.process_startup.coverage # type: ignore[attr-defined] + else: + coverage.process_startup.coverage = before # type: ignore[attr-defined] + return int(cov.save.call_count) + + @unittest.skipUnless(_has_coverage(), 'needs coverage 7.10+ (`[run] patch`)') + def test_a_worker_saves(self) -> None: + """The file exists before shutdown, so a death there cannot lose it whole.""" + self.assertEqual(self.finish(worker=True), 1) + + @unittest.skipUnless(_has_coverage(), 'needs coverage 7.10+ (`[run] patch`)') + def test_the_controller_does_not(self) -> None: + """The controller saves at its own exit; only workers die after reporting.""" + self.assertEqual(self.finish(worker=False), 0) + if __name__ == '__main__': unittest.main() diff --git a/tests/project/test_coverage_rcfile.py b/tests/project/test_coverage_rcfile.py index f87cf0b6d7..fd125aae2d 100644 --- a/tests/project/test_coverage_rcfile.py +++ b/tests/project/test_coverage_rcfile.py @@ -49,6 +49,10 @@ def test_the_xdist_workers_are_measured(self) -> None: self.assertIs(run.get('parallel'), True) self.assertIn('subprocess', run.get('patch', ())) + def test_a_sigterm_saves_the_data(self) -> None: + """A worker SIGTERMed during shutdown otherwise leaves no data file (#1423).""" + self.assertIs(self.rcfile['run'].get('sigterm'), True) + def test_the_core_is_ctrace(self) -> None: """sysmon grew one module's peak RSS from 0.40 to 4.17 GiB and killed the runner (#1064).""" self.assertEqual(self.rcfile['run'].get('core'), 'ctrace')