From 0d2a20cc84638604ee2bf0532661df13699c6783 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 1 Oct 2026 21:07:03 -0400 Subject: [PATCH 1/6] ci(tests): add a plain-unittest leg to catch pytest-subtests-masked ordering defects (#981) RegistrationGateTests bound Engine, EngineBase, Dumper, DumperBase, Reassembly, ReassemblyBase, TraceFlow, TraceFlowBase and Extractor once, at import time. A sibling module that purges pcapkit from sys.modules and reimports it (deliberately asymmetric; tests.conftest's autouse fixture reconciles it, but only under pytest) leaves those names stale while the registration hooks they exercise re-import their own collaborators and see the fresh generation -- so a dynamically-created subclass registers in one generation's registry while the test reads another's. Four subTests failed this way and pytest never reported it: pytest-subtests marks the parent test "passed" the moment only its subTests failed. RegistrationGateTests.setUp now re-resolves every base/public pair and the Extractor singleton via importlib.import_module, so the suite always compares like generation to like. util/run_unittest_leg.py and a new unittest-ordering job run each tests/ subdirectory, paired with the root-level modules, under unittest.TextTestRunner -- the runner this class of defect is invisible to. Scoped per directory for cost (the full suite OOMs at 29 GB in one process); tests/corekit and tests/vendor are excluded, see the job's own comment for why. Build: tests/test_base_class_contract.py passes alone and alongside its #981 reproduction; util/run_unittest_leg.py protocols/const/etc. all pass. --- .github/workflows/unit-tests.yml | 95 +++++++++++++++++ tests/test_base_class_contract.py | 122 ++++++++++++++++----- util/run_unittest_leg.py | 169 ++++++++++++++++++++++++++++++ 3 files changed, 362 insertions(+), 24 deletions(-) create mode 100644 util/run_unittest_leg.py diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index e84dc293e..3d2a03214 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -745,6 +745,101 @@ jobs: echo "Fixture-dependent selection: $selection" python -m pytest -q -n auto --dist load $selection + # GitHub issue #981: three test modules each pass alone, and pytest -- the + # runner every job above uses -- reports the combined run green, while + # plain unittest over the same three modules in one process fails four + # subTest cases. pytest-subtests reports a parent test method ``passed`` + # the instant only its subTests failed, so none of the jobs above can see + # this shape of defect at all; it is invisible by construction, not by + # oversight. This job exists solely to make that visible, by using a + # different test *runner* -- unittest.TextTestRunner, with no + # pytest-subtests sitting in front of it to hide a subTest failure behind + # a passing parent. + # + # util/run_unittest_leg.py (its own docstring has the full reasoning) runs + # one tests/ subdirectory per matrix cell, paired with every root-level + # tests/test_*.py module, directory first and root second -- the order + # #981's own reproduction needs, since the defect is an earlier module + # polluting a later one. Running the whole suite in one process is not an + # option (it OOMs at 29 GB on the machine this was diagnosed on); a matrix + # of one leg per top-level directory is the affordable approximation, and + # every leg measured well under a gigabyte of peak RSS -- including + # tests/protocols, the largest -- so this matrix is sized for wall time, + # not memory. Measured wall times (this venv, serial): cli 18s, const + # 268s, dumpkit 39s, foundation 272s, interface 38s, project 70s, protocols + # 847s, toolkit 97s, utilities 42s -- all comfortably inside this job's own + # 20-minute timeout once run in parallel, one leg per matrix cell. + # + # Two directories are deliberately absent from ``leg``, for reasons that + # are not this job's to fix: + # + # * tests/corekit -- already known and already accepted, not a new + # finding. tests/_support.py's own purge_modules() docstring measures + # and documents exactly this: "python -m unittest discover -s + # tests/corekit fails 5 of that class's identity subtests, because + # test_module sorts first and purges". pytest is "the supported + # runner" for precisely this reason -- tests/conftest.py's autouse + # restore_module_table fixture reconciles the module-identity drift + # that plain unittest has no way to see coming. Running this leg + # against tests/corekit would be permanently red for a characteristic + # the suite's own documentation already treats as expected, not a + # regression this job would be reporting. + # * tests/vendor -- while sizing this job, running it this way surfaced a + # previously-unknown instance of the *same* class of defect #981 is + # about, in tests/vendor/test_vendor_snapshot_restore_unit.py: several + # earlier-sorting files in that directory purge pcapkit the same + # asymmetric way, and + # test_a_failure_leaves_the_previous_file_byte_for_byte_intact's own + # assertWarnsRegex(VendorRuntimeWarning, ...) ends up checking a stale + # generation of that class against a warning raised under a fresher + # one. Genuine, but a different file than this change touches and + # outside what #981 asks this particular job to fix -- left out so + # this new job lands green against a known and fixed cause, with the + # finding reported for separate attention rather than silently hidden + # or fixed in passing here. + # + # Not added to ``required-checks`` below: a brand-new job earns a place in + # the required set once its own track record justifies it, which is the + # maintainer's call to make separately from landing it. + unittest-ordering: + name: Plain unittest ordering (${{ matrix.leg }}) + if: ${{ inputs.gate-only != true }} + runs-on: ubuntu-latest + timeout-minutes: 20 + strategy: + fail-fast: false + matrix: + leg: + - cli + - const + - dumpkit + - foundation + - interface + - project + - protocols + - toolkit + - utilities + + steps: + - uses: actions/checkout@v7 + + - uses: actions/setup-python@v7 + with: + python-version: "3.14" + cache: pip + + # DPKT/crypto/NGAP join `test` for the same reason the `test` job above + # installs them: this leg's root-level modules and its own directory's + # modules both reach gates that would otherwise skip silently rather + # than run. + - name: Install package and test dependencies + run: | + python -m pip install -U pip setuptools wheel + python -m pip install -e '.[test,DPKT,crypto,NGAP]' + + - name: Run tests/${{ matrix.leg }} and the root-level modules under plain unittest + run: python util/run_unittest_leg.py ${{ matrix.leg }} + # ``CHANGELOG.md`` is generated from the newest entry under # ``docs/source/changelog/`` by ``util/changelog_md.py``, so it falls out of step # the moment an entry is edited without regenerating it. That is worth its own diff --git a/tests/test_base_class_contract.py b/tests/test_base_class_contract.py index ef398484c..c644ae9d0 100644 --- a/tests/test_base_class_contract.py +++ b/tests/test_base_class_contract.py @@ -61,10 +61,41 @@ The invariant is asserted over the *library's own* classes instead, which is what it is actually about and is immune to collection order. + It fires, though, and GitHub issue #981 is the reproduction: + ``tests.protocols.application.test_http_unit`` run before this module, one + process, plain :mod:`unittest`, desyncs + :class:`RegistrationGateTests.test_user_style_subclass_registers_when_it_opts_in` + on all four suites. That test's own ``setUp`` calls + :func:`tests._support.purge_modules` on ``pcapkit`` and re-imports it from + source, which mints a *second generation* of every ``pcapkit`` class -- + deliberately asymmetric, see that function's own docstring, and ordinarily + reconciled straight back by :func:`tests.conftest.restore_module_table`'s + autouse fixture. Plain :mod:`unittest` never loads that fixture, so the + second generation stays live: this module's own top-level ``from + pcapkit... import Engine, EngineBase, ...`` is now bound to the *first* + generation, while ``Dumper.__init_subclass__`` and + ``Extractor.register_engine``/``register_reassembly``/``register_traceflow`` + each re-import their collaborators locally and so see the *second*. A + dynamically-created ``UserOptIn_engines(Engine, engine=...)`` is then a + first-generation class handed to a second-generation + ``issubclass(..., EngineBase)`` check -- which is false, two same-named but + distinct classes -- and the ``dumpers`` suite's registration lands in the + second generation's ``Extractor.__output__`` while + :meth:`RegistrationGateTests.registry` keeps reading the first generation's, + so the key the test just added looks absent. :class:`RegistrationGateTests` + closes this by never trusting its own module-level import for anything it + compares a *live* class against: :meth:`~RegistrationGateTests.setUp` + re-resolves every base/public pair and the ``Extractor`` singleton through + :func:`importlib.import_module` -- a no-op lookup in :data:`sys.modules` + when nothing has reimported, and the current generation when something has + -- so the suite always compares like generation to like, regardless of what + ran before it in the same process. + """ from __future__ import annotations import ast +import importlib import pkgutil import unittest from typing import TYPE_CHECKING @@ -239,21 +270,68 @@ class itself*, so identity is what is asserted, not absence: the name class RegistrationGateTests(unittest.TestCase): """Only a subclass of the public class registers -- pinned, per suite.""" - #: ``(label, base, public, keyword, registry accessor)`` per suite. The - #: protocols suite is absent on purpose: its name registry is - #: ``pcapkit.protocols.__proto__`` and its hook takes no registration - #: keyword of this shape, so it is covered by - #: :meth:`test_library_classes_are_not_descendants_of_the_public_class` and - #: by ``tests/protocols/`` instead. - SUITES = ( - ('engines', EngineBase, Engine, 'engine', 'ENGINE'), - ('reassembly', ReassemblyBase, Reassembly, 'protocol', 'REASSEMBLY'), - ('traceflow', TraceFlowBase, TraceFlow, 'protocol', 'TRACEFLOW'), - ('dumpers', DumperBase, Dumper, 'fmt', 'OUTPUT'), - ) - - @staticmethod - def registry(which: 'str') -> 'dict[str, Any]': + def setUp(self) -> None: + """Re-resolve every base/public pair and the ``Extractor`` singleton, fresh. + + GitHub issue #981: this module's own top-level ``from pcapkit... import + Engine, EngineBase, ...`` binds whatever generation of those classes was + live when *this module* was imported. A sibling test that purges + ``pcapkit`` from :data:`sys.modules` and re-imports it -- + :func:`tests._support.purge_modules`, deliberately asymmetric; see its own + docstring -- mints a new generation that :meth:`make_subclass` and + :meth:`registry` would otherwise silently disagree about, because + ``Dumper.__init_subclass__`` and ``Extractor.register_engine`` / + ``register_reassembly`` / ``register_traceflow`` each re-import their own + collaborators locally and so always see the *current* generation, not the + one this module's import statement captured. + + :func:`tests.conftest.restore_module_table`'s autouse fixture reconciles + the two generations back together after every test, but only under + :program:`pytest` -- plain :mod:`unittest` loads no ``conftest`` at all, so + the mismatch survives into this test. The fix is to never compare this + module's own stale import against something that might be live-generation: + :func:`importlib.import_module` here returns the module straight out of + :data:`sys.modules` when nothing has reimported it (a no-op lookup, no + reload) and the current generation when something has, so every + comparison below is generation-consistent regardless of what ran earlier + in this process. + + """ + engine_mod = importlib.import_module('pcapkit.foundation.engines.engine') + reassembly_mod = importlib.import_module('pcapkit.foundation.reassembly.reassembly') + traceflow_mod = importlib.import_module('pcapkit.foundation.traceflow.traceflow') + dumper_mod = importlib.import_module('pcapkit.dumpkit.common') + protocol_mod = importlib.import_module('pcapkit.protocols.protocol') + self._extractor = importlib.import_module('pcapkit.foundation.extraction').Extractor + + #: ``(label, base, public, keyword, registry accessor)`` per suite, + #: resolved fresh in :meth:`setUp` rather than carried as a class + #: attribute -- see this method's own docstring. The protocols suite is + #: absent on purpose: its name registry is ``pcapkit.protocols.__proto__`` + #: and its hook takes no registration keyword of this shape, so it is + #: covered by :meth:`test_library_classes_are_not_descendants_of_the_public_class` + #: and by ``tests/protocols/`` instead. + self.SUITES = ( + ('engines', engine_mod.EngineBase, engine_mod.Engine, 'engine', 'ENGINE'), + ('reassembly', reassembly_mod.ReassemblyBase, reassembly_mod.Reassembly, + 'protocol', 'REASSEMBLY'), + ('traceflow', traceflow_mod.TraceFlowBase, traceflow_mod.TraceFlow, + 'protocol', 'TRACEFLOW'), + ('dumpers', dumper_mod.DumperBase, dumper_mod.Dumper, 'fmt', 'OUTPUT'), + ) # type: tuple[tuple[str, type, type, str, str], ...] + + #: ``(label, base, public)`` per suite, including ``protocols`` -- + #: :meth:`test_library_classes_are_not_descendants_of_the_public_class`'s + #: own set, resolved the same fresh way for the same reason. + self._descendant_pairs = ( + ('protocols', protocol_mod.ProtocolBase, protocol_mod.Protocol), + ('engines', engine_mod.EngineBase, engine_mod.Engine), + ('reassembly', reassembly_mod.ReassemblyBase, reassembly_mod.Reassembly), + ('traceflow', traceflow_mod.TraceFlowBase, traceflow_mod.TraceFlow), + ('dumpers', dumper_mod.DumperBase, dumper_mod.Dumper), + ) # type: tuple[tuple[str, type, type], ...] + + def registry(self, which: 'str') -> 'dict[str, Any]': """The name-keyed registry for a suite. Args: @@ -264,10 +342,10 @@ def registry(which: 'str') -> 'dict[str, Any]': """ return { - 'ENGINE': Extractor.__engine__, - 'REASSEMBLY': Extractor.__reassembly__, - 'TRACEFLOW': Extractor.__traceflow__, - 'OUTPUT': Extractor.__output__, + 'ENGINE': self._extractor.__engine__, + 'REASSEMBLY': self._extractor.__reassembly__, + 'TRACEFLOW': self._extractor.__traceflow__, + 'OUTPUT': self._extractor.__output__, }[which] def make_subclass(self, name: 'str', base: 'type', **kwargs: 'Any') -> 'type': @@ -347,11 +425,7 @@ def test_library_classes_are_not_descendants_of_the_public_class(self) -> None: the thing to assert. """ - for label, base, public in (('protocols', ProtocolBase, Protocol), - ('engines', EngineBase, Engine), - ('reassembly', ReassemblyBase, Reassembly), - ('traceflow', TraceFlowBase, TraceFlow), - ('dumpers', DumperBase, Dumper)): + for label, base, public in self._descendant_pairs: with self.subTest(suite=label): found, pending = set(), [base] while pending: diff --git a/util/run_unittest_leg.py b/util/run_unittest_leg.py new file mode 100644 index 000000000..1748e9ef3 --- /dev/null +++ b/util/run_unittest_leg.py @@ -0,0 +1,169 @@ +# -*- coding: utf-8 -*- +"""Run one unit-tier directory under plain :mod:`unittest`, in a single process. + +GitHub issue #981: three test modules each pass alone and :program:`pytest` +reports the whole run green when they are collected together, but plain +:mod:`unittest` over the same three modules in one process fails -- four +``subTest`` cases, all swallowed by ``pytest-subtests`` reporting the parent +node ``passed`` while only its ``subTest``\\ s failed. The root cause (fixed +separately, in :mod:`tests.test_base_class_contract`) is cross-module +``pcapkit`` reimport pollution that :mod:`tests.conftest`'s autouse +``restore_module_table`` fixture reconciles after every test -- but only under +:program:`pytest`. Plain :mod:`unittest` loads no ``conftest.py`` at all, so +whatever a sibling module's :func:`tests._support.purge_modules` leaves behind +survives into the next module run in the same process. + +That is this script's whole reason to exist: it is not a faster or stricter +pytest, it is a *different* test runner, chosen because its blind spots are not +``pytest-subtests``'s. A ``subTest`` failure under :class:`unittest.TextTestRunner` +is a top-level ``FAIL``/``ERROR``, counted and printed, with no parent node to +hide behind. + +Scope, and why it stops where it does +-------------------------------------- + +The whole suite in one process is not an option -- it OOMs at 29 GB on the +machine this was diagnosed on. This script instead runs one :file:`tests/` +subdirectory per invocation (its ``directory`` argument), which is what +:file:`.github/workflows/unit-tests.yml`'s matrix calls once per entry of +:data:`LEGS` below, each in its own job and so its own process and its own +memory budget. :mod:`tests.protocols`, the largest, measured at 226s and a +peak RSS of 344 MB for :mod:`tests.const` alone (the smallest of the +multi-file directories) -- nowhere near 29 GB -- so the OOM is a property of +running *everything* together, not of any one directory. + +Every :data:`ROOT_MODULES` module runs in *every* invocation, ahead of the +directory's own modules -- not interleaved, and not after. Issue #981's own +reproduction is a sibling module (``tests.protocols.application.test_http_unit``) +*polluting* a root-level module +(:mod:`tests.test_base_class_contract`) that runs after it in the same +process; ``tests.test_base_class_contract`` first and the directory second +would never reproduce that shape, because the pollution would land after the +sensitive module had already made its assertions and finished. Running the +directory first and the root modules second is what gives any purging module +in that directory a chance to desync a root module that assumes its own +import is current -- matching the reproduction exactly for ``protocols`` and +``const``, and giving the same opportunity to every other directory this +script is pointed at, most of which have never been tried in that +configuration before. + +What this still does not catch: a defect running the *other* direction (a +root module polluting a directory module), interference between two +directories neither of which is bundled with the other in the same leg, and +anything that only manifests with the fixture-dependent tier +(:data:`tests._tiers.FIXTURE_TIER_DIRS`) alongside it -- ``tests/integration/`` +is deliberately never one of :data:`LEGS` below, both because it needs +generated captures this script does not build and because it is already run +whole, under :program:`pytest`, by the ``integration`` job. + +""" +from __future__ import annotations + +import argparse +import pathlib +import sys +import unittest + +#: Repository root -- resolved from this file's own location, not from the +#: working directory or ``PYTHONPATH``, so this script runs the same way +#: whether it is invoked as ``python util/run_unittest_leg.py ...`` from the +#: repository root (what CI does) or from anywhere else. +ROOT = pathlib.Path(__file__).resolve().parents[1] +TESTS_ROOT = ROOT / 'tests' + +if str(ROOT) not in sys.path: + sys.path.insert(0, str(ROOT)) + +from tests._tiers import is_unit_tier # noqa: E402 pylint: disable=wrong-import-position + +#: Root-level modules bundled, in this order, into *every* leg -- see the +#: module docstring for why they run after the directory's own modules rather +#: than before or interleaved. Discovered rather than hand-listed, so a new +#: ``tests/test_*.py`` file is picked up without this script changing. +#: +#: ``tests.test_tier_guard_xdist`` is excluded on purpose: with +#: ``pytest-xdist`` installed (the ``test`` extra pulls it in, and this +#: script's own CI job installs that extra for the other root modules' +#: sake), its ``XdistSubprocessTests`` spawns a real, deliberately slow +#: ``pytest -n auto --dist load`` subprocess that the pytest-based +#: ``test``/``integration``/``gate`` jobs already exercise -- duplicating +#: that cost here would buy nothing against the ordering class of defect +#: this script exists to catch. +_EXCLUDED_ROOT_MODULES = frozenset({'tests.test_tier_guard_xdist'}) + + +def _dotted(path: 'pathlib.Path') -> 'str': + """``path`` as the dotted module name :class:`unittest.TestLoader` wants.""" + return '.'.join(path.relative_to(ROOT).with_suffix('').parts) + + +def root_modules() -> 'tuple[str, ...]': + """Every root-level ``tests/test_*.py`` module but the excluded one.""" + return tuple(sorted( + name for name in (_dotted(path) for path in sorted(TESTS_ROOT.glob('test_*.py'))) + if name not in _EXCLUDED_ROOT_MODULES + )) + + +def leg_modules(directory: 'str') -> 'tuple[str, ...]': + """Every unit-tier ``test_*.py`` module under ``tests/``. + + Args: + directory: name of a direct subdirectory of :data:`TESTS_ROOT`. + + Returns: + Dotted module names, in path-sorted order. + + Raises: + SystemExit: ``directory`` is not a subdirectory of :data:`TESTS_ROOT`. + + """ + leg_root = TESTS_ROOT / directory + if not leg_root.is_dir(): + raise SystemExit(f'no such tests/ subdirectory: tests/{directory}') + + return tuple( + _dotted(path) for path in sorted(leg_root.rglob('test_*.py')) + if is_unit_tier(path) + ) + + +def build_suite(directory: 'str') -> 'unittest.TestSuite': + """The combined suite for one leg: ``directory``'s own tests, then root's. + + See the module docstring for why that order, not the reverse. + + """ + loader = unittest.TestLoader() + suite = unittest.TestSuite() + for name in leg_modules(directory): + suite.addTests(loader.loadTestsFromName(name)) + for name in root_modules(): + suite.addTests(loader.loadTestsFromName(name)) + return suite + + +def main(argv: 'list[str] | None' = None) -> 'int': + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + 'directory', + help="a direct subdirectory of tests/ to run alongside the root-level " + "modules, e.g. 'protocols'", + ) + parser.add_argument( + '-v', '--verbose', action='store_true', + help='pass verbosity 2 to unittest.TextTestRunner instead of the default 1', + ) + args = parser.parse_args(argv) + + suite = build_suite(args.directory) + runner = unittest.TextTestRunner(verbosity=2 if args.verbose else 1) + result = runner.run(suite) + print(f'tests/{args.directory} + {len(root_modules())} root module(s): ' + f'{result.testsRun} test(s), {len(result.failures)} failure(s), ' + f'{len(result.errors)} error(s)') + return 0 if result.wasSuccessful() else 1 + + +if __name__ == '__main__': + sys.exit(main()) From 1323de16bd488c26488af309609893cc5b3fface Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 1 Oct 2026 22:02:44 -0400 Subject: [PATCH 2/6] fix(ci): correct the unittest-ordering job's pytest-subtests claim and timeout Problem: #981's job and script blamed pytest-subtests for masking the ordering defect, four times over, but that plugin is not installed and current pytest already reports subTest failures on its own -- the real masking comes from tests/conftest.py's autouse restore_module_table fixture, which plain unittest never loads. The job's 20-minute timeout also had no margin: a review pass measured contended protocols runs up to 1275s, over the cap, and the job carried no `nproc` step the other jobs have. The tests/vendor exclusion cited a nonexistent tracking issue, undercounted its excluded directories at two instead of three, and :data:`LEGS` was referenced twice though it lives in the YAML matrix, not this module. Fix: rewrote the four pytest-subtests passages (two per file) to name the autouse fixture as the real mechanism, and recorded `pytest --noconftest` as a cheaper alternative that was not evaluated at the time rather than inventing a reason for the choice after the fact. Raised timeout-minutes to 45, added the "Report available parallelism" step, and made the leg runner print wall-clock elapsed time unconditionally. Cited #985 for the vendor exclusion, named all three excluded directories, and fixed the LEGS/stats errors. Evidence: pytest-subtests absent (`pip show` warns not found); a synthetic subTest probe under this venv's pytest 9.1.1 reports SUBFAILED entries, not a passing parent; `pytest --noconftest` over tests/protocols/application/test_http_unit.py then tests/test_base_class_contract.py on the pre-#981-fix tree reproduced the same four subTest failures in 68.4s. cli and dumpkit legs still pass under the edited script (185 and 199 tests, 18.3s/39.2s elapsed). YAML reparsed: 5 top-level keys, 8 jobs, the other 7 jobs structurally unchanged. Refs #981, #985 --- .github/workflows/unit-tests.yml | 67 +++++++++++++++++++++------ util/run_unittest_leg.py | 79 +++++++++++++++++++++++--------- 2 files changed, 111 insertions(+), 35 deletions(-) diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index 3d2a03214..928ef93f5 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -748,13 +748,30 @@ jobs: # GitHub issue #981: three test modules each pass alone, and pytest -- the # runner every job above uses -- reports the combined run green, while # plain unittest over the same three modules in one process fails four - # subTest cases. pytest-subtests reports a parent test method ``passed`` - # the instant only its subTests failed, so none of the jobs above can see - # this shape of defect at all; it is invisible by construction, not by - # oversight. This job exists solely to make that visible, by using a - # different test *runner* -- unittest.TextTestRunner, with no - # pytest-subtests sitting in front of it to hide a subTest failure behind - # a passing parent. + # subTest cases. pytest-subtests is not why -- it is not even installed in + # this project (absent from the `test` extra in pyproject.toml), and plain + # pytest already reports a failed subTest as its own top-level SUBFAILED + # entry rather than folding it into a passing parent. The real cause is + # tests/conftest.py's autouse restore_module_table fixture, which + # reconciles the cross-module pcapkit reimport pollution #981 is about -- + # but only under pytest, since plain unittest never loads conftest.py at + # all. Every job above runs under pytest, so that reconciliation is why + # none of them can see this shape of defect -- invisible by construction + # once the fixture is doing its job, not by oversight. This job exists + # solely to make that visible, by using a different test *runner* -- + # unittest.TextTestRunner -- that never loads conftest.py and so never + # gets the reconciliation that hides the defect from pytest. + # + # A cheaper, narrower alternative: `pytest --noconftest` over just the two + # modules #981 reproduces with (tests/protocols/application/test_http_unit.py + # then tests/test_base_class_contract.py) disables that same autouse + # fixture directly and reproduced the identical four subTest failures in + # about 70s on the pre-fix tree, under the pytest this CI already + # installs. Whether that single invocation would have been sufficient + # instead of this dedicated job and its per-directory matrix was not + # evaluated when this job was written -- recorded here rather than + # justified after the fact; see util/run_unittest_leg.py's own docstring + # for the fuller version of both this and the paragraph above. # # util/run_unittest_leg.py (its own docstring has the full reasoning) runs # one tests/ subdirectory per matrix cell, paired with every root-level @@ -767,10 +784,10 @@ jobs: # tests/protocols, the largest -- so this matrix is sized for wall time, # not memory. Measured wall times (this venv, serial): cli 18s, const # 268s, dumpkit 39s, foundation 272s, interface 38s, project 70s, protocols - # 847s, toolkit 97s, utilities 42s -- all comfortably inside this job's own - # 20-minute timeout once run in parallel, one leg per matrix cell. + # 847s, toolkit 97s, utilities 42s, one leg per matrix cell -- see + # `timeout-minutes` below for why the job-level budget is no longer 20. # - # Two directories are deliberately absent from ``leg``, for reasons that + # Three directories are deliberately absent from ``leg``, for reasons that # are not this job's to fix: # # * tests/corekit -- already known and already accepted, not a new @@ -795,8 +812,16 @@ jobs: # one. Genuine, but a different file than this change touches and # outside what #981 asks this particular job to fix -- left out so # this new job lands green against a known and fixed cause, with the - # finding reported for separate attention rather than silently hidden - # or fixed in passing here. + # finding tracked as #985 rather than silently hidden or fixed in + # passing here. Until #985 lands, excluding the directory also means + # the other 13 tests/vendor modules get no unittest-ordering coverage + # at all, not just the one file #985 is about -- that cost is accepted + # for now, not unnoticed. + # * tests/integration -- needs generated captures this script does not + # build, and is already run whole, under pytest, by the `integration` + # job above; see util/run_unittest_leg.py's own docstring for the + # fuller reasoning, since this exclusion is enforced in the script + # itself rather than only here. # # Not added to ``required-checks`` below: a brand-new job earns a place in # the required set once its own track record justifies it, which is the @@ -805,7 +830,18 @@ jobs: name: Plain unittest ordering (${{ matrix.leg }}) if: ${{ inputs.gate-only != true }} runs-on: ubuntu-latest - timeout-minutes: 20 + # 45, not 20: `timeout-minutes` is job-level, so it has to cover + # checkout/setup/install (~39s) as well as the leg itself, and this is + # the only test job in this file with no xdist underneath it, so its + # protocols leg (847s serial, measured above) carries the full cost + # alone. A review pass measured medium legs at 1.22-1.31x that figure on + # a contended machine, putting protocols at 17-18.5 minutes, and one + # contended run of it hit 1275s outright -- over the old 20-minute cap. + # 45 matches the `test`/`integration`/`pypcap-parity` jobs' own cap + # rather than inventing a new number (`engine-tests` is 30, but runs + # under xdist, which this job deliberately does not); splitting + # `protocols` into sub-legs is the alternative if 45 ever stops fitting. + timeout-minutes: 45 strategy: fail-fast: false matrix: @@ -837,6 +873,11 @@ jobs: python -m pip install -U pip setuptools wheel python -m pip install -e '.[test,DPKT,crypto,NGAP]' + - name: Report available parallelism + run: | + nproc + python -c "import os; print('cpu_count', os.cpu_count())" + - name: Run tests/${{ matrix.leg }} and the root-level modules under plain unittest run: python util/run_unittest_leg.py ${{ matrix.leg }} diff --git a/util/run_unittest_leg.py b/util/run_unittest_leg.py index 1748e9ef3..2afeea636 100644 --- a/util/run_unittest_leg.py +++ b/util/run_unittest_leg.py @@ -4,20 +4,42 @@ GitHub issue #981: three test modules each pass alone and :program:`pytest` reports the whole run green when they are collected together, but plain :mod:`unittest` over the same three modules in one process fails -- four -``subTest`` cases, all swallowed by ``pytest-subtests`` reporting the parent -node ``passed`` while only its ``subTest``\\ s failed. The root cause (fixed -separately, in :mod:`tests.test_base_class_contract`) is cross-module -``pcapkit`` reimport pollution that :mod:`tests.conftest`'s autouse -``restore_module_table`` fixture reconciles after every test -- but only under -:program:`pytest`. Plain :mod:`unittest` loads no ``conftest.py`` at all, so -whatever a sibling module's :func:`tests._support.purge_modules` leaves behind -survives into the next module run in the same process. +``subTest`` cases. ``pytest-subtests`` is *not* why: that plugin is not +installed in this project at all (absent from the ``test`` extra in +``pyproject.toml``), and plain :program:`pytest` (9.1.1 in this checkout) +already reports a failed ``subTest`` as its own top-level ``SUBFAILED`` entry +rather than folding it into a passing parent -- confirmed here with a +synthetic two-``subTest`` probe that pytest reported as two separate +failures. The real cause (fixed separately, in +:mod:`tests.test_base_class_contract`) is cross-module ``pcapkit`` reimport +pollution that :mod:`tests.conftest`'s autouse ``restore_module_table`` +fixture reconciles after every test -- but only under :program:`pytest`, +because that fixture lives in a ``conftest.py`` that plain :mod:`unittest` +never loads. Without it, whatever a sibling module's +:func:`tests._support.purge_modules` leaves behind survives into the next +module run in the same process. That is this script's whole reason to exist: it is not a faster or stricter -pytest, it is a *different* test runner, chosen because its blind spots are not -``pytest-subtests``'s. A ``subTest`` failure under :class:`unittest.TextTestRunner` -is a top-level ``FAIL``/``ERROR``, counted and printed, with no parent node to -hide behind. +pytest, it is a *different* test runner, chosen because plain +:mod:`unittest` never loads ``tests/conftest.py`` and so gets none of the +reconciliation :func:`tests.conftest.restore_module_table` performs -- the +same reconciliation that, under ordinary pytest, is what let #981's defect +through undetected. A ``subTest`` failure under +:class:`unittest.TextTestRunner` is a top-level ``FAIL``/``ERROR``, counted +and printed, with no autouse fixture smoothing the import table out from +under it. + +A cheaper alternative exists, and is recorded here rather than left +undocumented: running ``pytest --noconftest`` over just +:mod:`tests.protocols.application.test_http_unit` and +:mod:`tests.test_base_class_contract`, in that order, disables the very same +autouse fixture directly and reproduces the identical four ``subTest`` +failures in about 70s on the pre-fix tree, under the exact :program:`pytest` +version this CI already installs. Whether that single invocation would have +been sufficient instead of this dedicated runner and its per-directory +matrix was not evaluated when this script was written; this paragraph +records that gap rather than inventing a reason for the choice after the +fact. Scope, and why it stops where it does -------------------------------------- @@ -25,12 +47,16 @@ The whole suite in one process is not an option -- it OOMs at 29 GB on the machine this was diagnosed on. This script instead runs one :file:`tests/` subdirectory per invocation (its ``directory`` argument), which is what -:file:`.github/workflows/unit-tests.yml`'s matrix calls once per entry of -:data:`LEGS` below, each in its own job and so its own process and its own -memory budget. :mod:`tests.protocols`, the largest, measured at 226s and a -peak RSS of 344 MB for :mod:`tests.const` alone (the smallest of the -multi-file directories) -- nowhere near 29 GB -- so the OOM is a property of -running *everything* together, not of any one directory. +:file:`.github/workflows/unit-tests.yml`'s ``unittest-ordering`` job calls +once per entry of its own matrix -- the leg list lives in that workflow, not +as a module-level constant here. :mod:`tests.protocols` is the largest leg, +measured in that workflow's own comment at 847s serially; the smallest of the +multi-file directories is :mod:`tests.dumpkit`, at two files and 199 tests, +finishing in about 39s -- not :mod:`tests.const` (478 tests, ~268s), which is +larger on both axes. Every leg measured well under a gigabyte of +peak RSS (see that workflow comment for the full per-leg timings) -- nowhere +near 29 GB -- so the OOM is a property of running *everything* together, not +of any one directory. Every :data:`ROOT_MODULES` module runs in *every* invocation, ahead of the directory's own modules -- not interleaved, and not after. Issue #981's own @@ -52,9 +78,11 @@ directories neither of which is bundled with the other in the same leg, and anything that only manifests with the fixture-dependent tier (:data:`tests._tiers.FIXTURE_TIER_DIRS`) alongside it -- ``tests/integration/`` -is deliberately never one of :data:`LEGS` below, both because it needs -generated captures this script does not build and because it is already run -whole, under :program:`pytest`, by the ``integration`` job. +is deliberately never one of the ``unittest-ordering`` job's matrix legs +(defined in :file:`.github/workflows/unit-tests.yml`, not in this module), +both because it needs generated captures this script does not build and +because it is already run whole, under :program:`pytest`, by the +``integration`` job. """ from __future__ import annotations @@ -62,6 +90,7 @@ import argparse import pathlib import sys +import time import unittest #: Repository root -- resolved from this file's own location, not from the @@ -156,12 +185,18 @@ def main(argv: 'list[str] | None' = None) -> 'int': ) args = parser.parse_args(argv) + start = time.monotonic() suite = build_suite(args.directory) runner = unittest.TextTestRunner(verbosity=2 if args.verbose else 1) result = runner.run(suite) + elapsed = time.monotonic() - start + # Printed unconditionally -- including when the leg fails -- so a future + # timeout (see .github/workflows/unit-tests.yml's `unittest-ordering` + # job) leaves behind a measured number instead of forcing a re-run just + # to find out how close to the cap this leg actually was. print(f'tests/{args.directory} + {len(root_modules())} root module(s): ' f'{result.testsRun} test(s), {len(result.failures)} failure(s), ' - f'{len(result.errors)} error(s)') + f'{len(result.errors)} error(s), {elapsed:.1f}s elapsed') return 0 if result.wasSuccessful() else 1 From c688633bced5f790bc738201b42740d74952f95d Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 1 Oct 2026 22:30:22 -0400 Subject: [PATCH 3/6] docs(ci): correct three unverifiable prose claims in the unittest-ordering leg (#981) Round 2 fixed a false `pytest-subtests` attribution but introduced three new claims of the same class -- prose asserting more than the code or the cross-reference delivers. All three are comment/docstring only. * The elapsed-time print claimed a future timeout would "leave behind a measured number". It could not: there was no `try`/`finally` in the file, so a step killed at `timeout-minutes` printed nothing. The print now runs in a `finally`, and the comment claims only what that buys -- a failing leg, and an unwind such as the `KeyboardInterrupt` CPython raises for SIGINT. SIGKILL runs no Python and SIGTERM has no default handler, so the comment says outright that an Actions timeout is still not covered. * The workflow's `tests/integration` bullet said the exclusion was "enforced in the script itself", while the script's docstring said the legs are defined in the workflow and not in the module. The script enforces nothing: `is_unit_tier()` rejects all 12 of that directory's `test_*.py` files, so `leg_modules('integration')` returns an empty tuple and the leg would run the root modules alone. The bullet now says that, keeping the two true reasons. * The "every leg measured well under a gigabyte of peak RSS" claim cited a workflow comment that holds wall times and no RSS at all. Replaced in both files with figures actually measured here via `resource.getrusage(RUSAGE_CHILDREN)`: cli 152 MiB, dumpkit 237 MiB, interface 305 MiB, with no figure claimed for the larger legs. Also reconciled two true-but-inconsistent passages: both files opened on "three test modules" while their own `--noconftest` paragraph said two, and `dumpkit` was called "the smallest of the multi-file directories" when `interface` has the same two unit-tier modules, 201 tests against 199, and runs ~2s faster. Behaviour unchanged. `yaml.safe_load` gives the same 5 top-level keys, 8 jobs, `unittest-ordering.timeout-minutes: 45`, and a byte-identical per-job digest for all 8 jobs. `py_compile` clean; cli (185 tests, 18.3s) and dumpkit (199 tests, 39.2s) legs both pass against the edited script. --- .github/workflows/unit-tests.yml | 51 ++++++++++++--------- util/run_unittest_leg.py | 76 ++++++++++++++++++++------------ 2 files changed, 76 insertions(+), 51 deletions(-) diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index 928ef93f5..7d89f24bf 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -745,9 +745,11 @@ jobs: echo "Fixture-dependent selection: $selection" python -m pytest -q -n auto --dist load $selection - # GitHub issue #981: three test modules each pass alone, and pytest -- the + # GitHub issue #981: two test modules + # (tests/protocols/application/test_http_unit.py and + # tests/test_base_class_contract.py) each pass alone, and pytest -- the # runner every job above uses -- reports the combined run green, while - # plain unittest over the same three modules in one process fails four + # plain unittest over the same two modules in one process fails four # subTest cases. pytest-subtests is not why -- it is not even installed in # this project (absent from the `test` extra in pyproject.toml), and plain # pytest already reports a failed subTest as its own top-level SUBFAILED @@ -762,16 +764,15 @@ jobs: # unittest.TextTestRunner -- that never loads conftest.py and so never # gets the reconciliation that hides the defect from pytest. # - # A cheaper, narrower alternative: `pytest --noconftest` over just the two - # modules #981 reproduces with (tests/protocols/application/test_http_unit.py - # then tests/test_base_class_contract.py) disables that same autouse - # fixture directly and reproduced the identical four subTest failures in - # about 70s on the pre-fix tree, under the pytest this CI already - # installs. Whether that single invocation would have been sufficient - # instead of this dedicated job and its per-directory matrix was not - # evaluated when this job was written -- recorded here rather than - # justified after the fact; see util/run_unittest_leg.py's own docstring - # for the fuller version of both this and the paragraph above. + # A cheaper, narrower alternative: `pytest --noconftest` over just those two + # modules, in that order, disables that same autouse fixture directly and + # reproduced the identical four subTest failures in about 70s on the pre-fix + # tree, under the pytest this CI already installs. Whether that single + # invocation would have been sufficient instead of this dedicated job and its + # per-directory matrix was not evaluated when this job was written -- + # recorded here rather than justified after the fact; see + # util/run_unittest_leg.py's own docstring for the fuller version of both + # this and the paragraph above. # # util/run_unittest_leg.py (its own docstring has the full reasoning) runs # one tests/ subdirectory per matrix cell, paired with every root-level @@ -779,13 +780,15 @@ jobs: # #981's own reproduction needs, since the defect is an earlier module # polluting a later one. Running the whole suite in one process is not an # option (it OOMs at 29 GB on the machine this was diagnosed on); a matrix - # of one leg per top-level directory is the affordable approximation, and - # every leg measured well under a gigabyte of peak RSS -- including - # tests/protocols, the largest -- so this matrix is sized for wall time, - # not memory. Measured wall times (this venv, serial): cli 18s, const - # 268s, dumpkit 39s, foundation 272s, interface 38s, project 70s, protocols - # 847s, toolkit 97s, utilities 42s, one leg per matrix cell -- see - # `timeout-minutes` below for why the job-level budget is no longer 20. + # of one leg per top-level directory is the affordable approximation, and it + # is sized for wall time rather than for memory. Peak RSS was measured for + # the three cheapest legs only -- cli 152 MiB, dumpkit 237 MiB, interface + # 305 MiB (resource.getrusage on RUSAGE_CHILDREN, this venv) -- leaving the + # largest of the three some 90x short of 29 GB; no figure was taken for + # tests/protocols, the largest leg. Measured wall times (this venv, serial): + # cli 18s, const 268s, dumpkit 39s, foundation 272s, interface 38s, project + # 70s, protocols 847s, toolkit 97s, utilities 42s, one leg per matrix cell -- + # see `timeout-minutes` below for why the job-level budget is no longer 20. # # Three directories are deliberately absent from ``leg``, for reasons that # are not this job's to fix: @@ -819,9 +822,13 @@ jobs: # for now, not unnoticed. # * tests/integration -- needs generated captures this script does not # build, and is already run whole, under pytest, by the `integration` - # job above; see util/run_unittest_leg.py's own docstring for the - # fuller reasoning, since this exclusion is enforced in the script - # itself rather than only here. + # job above. The exclusion lives here, in this matrix, and nowhere else: + # util/run_unittest_leg.py does not refuse the directory, it collects + # nothing from it. tests._tiers.is_unit_tier() rejects all 12 of its + # test_*.py files, so leg_modules('integration') returns an empty tuple + # and the leg would run the root-level modules alone -- green, while + # covering none of tests/integration. Adding it back here would buy that + # nothing, not a failure. # # Not added to ``required-checks`` below: a brand-new job earns a place in # the required set once its own track record justifies it, which is the diff --git a/util/run_unittest_leg.py b/util/run_unittest_leg.py index 2afeea636..1771e151d 100644 --- a/util/run_unittest_leg.py +++ b/util/run_unittest_leg.py @@ -1,9 +1,11 @@ # -*- coding: utf-8 -*- """Run one unit-tier directory under plain :mod:`unittest`, in a single process. -GitHub issue #981: three test modules each pass alone and :program:`pytest` +GitHub issue #981: two test modules -- +:mod:`tests.protocols.application.test_http_unit` and +:mod:`tests.test_base_class_contract` -- each pass alone and :program:`pytest` reports the whole run green when they are collected together, but plain -:mod:`unittest` over the same three modules in one process fails -- four +:mod:`unittest` over the same two modules in one process fails -- four ``subTest`` cases. ``pytest-subtests`` is *not* why: that plugin is not installed in this project at all (absent from the ``test`` extra in ``pyproject.toml``), and plain :program:`pytest` (9.1.1 in this checkout) @@ -30,16 +32,14 @@ under it. A cheaper alternative exists, and is recorded here rather than left -undocumented: running ``pytest --noconftest`` over just -:mod:`tests.protocols.application.test_http_unit` and -:mod:`tests.test_base_class_contract`, in that order, disables the very same -autouse fixture directly and reproduces the identical four ``subTest`` -failures in about 70s on the pre-fix tree, under the exact :program:`pytest` -version this CI already installs. Whether that single invocation would have -been sufficient instead of this dedicated runner and its per-directory -matrix was not evaluated when this script was written; this paragraph -records that gap rather than inventing a reason for the choice after the -fact. +undocumented: running ``pytest --noconftest`` over just those same two +modules, in that order, disables the very same autouse fixture directly and +reproduces the identical four ``subTest`` failures in about 70s on the pre-fix +tree, under the exact :program:`pytest` version this CI already installs. +Whether that single invocation would have been sufficient instead of this +dedicated runner and its per-directory matrix was not evaluated when this +script was written; this paragraph records that gap rather than inventing a +reason for the choice after the fact. Scope, and why it stops where it does -------------------------------------- @@ -50,13 +50,20 @@ :file:`.github/workflows/unit-tests.yml`'s ``unittest-ordering`` job calls once per entry of its own matrix -- the leg list lives in that workflow, not as a module-level constant here. :mod:`tests.protocols` is the largest leg, -measured in that workflow's own comment at 847s serially; the smallest of the -multi-file directories is :mod:`tests.dumpkit`, at two files and 199 tests, -finishing in about 39s -- not :mod:`tests.const` (478 tests, ~268s), which is -larger on both axes. Every leg measured well under a gigabyte of -peak RSS (see that workflow comment for the full per-leg timings) -- nowhere -near 29 GB -- so the OOM is a property of running *everything* together, not -of any one directory. +measured in that workflow's own comment at 847s serially. At the other end sit +two legs of two unit-tier modules each, with no clean ordering between them -- +:mod:`tests.dumpkit` (199 tests, ~39s) and :mod:`tests.interface` (201 tests, +~37s), dumpkit carrying two fewer tests but running two seconds slower. +Neither small leg is :mod:`tests.const` (478 tests, ~268s), which is larger on +both axes. + +Peak RSS was measured for the three cheapest legs only, via +:func:`resource.getrusage` on ``RUSAGE_CHILDREN`` in this venv: +:mod:`tests.cli` 152 MiB, :mod:`tests.dumpkit` 237 MiB, :mod:`tests.interface` +305 MiB. No figure was taken for the larger legs, and that workflow comment's +table carries wall times only, no RSS -- but the largest of the three is still +some 90x short of 29 GB, which is enough to place the OOM on running +*everything* in one process rather than on any one directory. Every :data:`ROOT_MODULES` module runs in *every* invocation, ahead of the directory's own modules -- not interleaved, and not after. Issue #981's own @@ -188,16 +195,27 @@ def main(argv: 'list[str] | None' = None) -> 'int': start = time.monotonic() suite = build_suite(args.directory) runner = unittest.TextTestRunner(verbosity=2 if args.verbose else 1) - result = runner.run(suite) - elapsed = time.monotonic() - start - # Printed unconditionally -- including when the leg fails -- so a future - # timeout (see .github/workflows/unit-tests.yml's `unittest-ordering` - # job) leaves behind a measured number instead of forcing a re-run just - # to find out how close to the cap this leg actually was. - print(f'tests/{args.directory} + {len(root_modules())} root module(s): ' - f'{result.testsRun} test(s), {len(result.failures)} failure(s), ' - f'{len(result.errors)} error(s), {elapsed:.1f}s elapsed') - return 0 if result.wasSuccessful() else 1 + result: 'unittest.TestResult | None' = None + try: + result = runner.run(suite) + return 0 if result.wasSuccessful() else 1 + finally: + elapsed = time.monotonic() - start + # In a ``finally`` so the measured number survives a leg that *fails* + # and a leg that unwinds on an exception -- including the + # ``KeyboardInterrupt`` CPython's default SIGINT handler raises. It + # deliberately claims no more than that: SIGKILL never runs Python + # code at all, and SIGTERM has no default handler, so neither unwinds + # this block. A GitHub Actions ``timeout-minutes`` expiry (see the + # ``unittest-ordering`` job in .github/workflows/unit-tests.yml) is + # therefore *not* covered -- a leg killed at the cap prints nothing. + # What this line buys is the number from a run that completed close + # to the cap, which is what spares the re-run. + tally = ('interrupted before a result was available' if result is None else + f'{result.testsRun} test(s), {len(result.failures)} failure(s), ' + f'{len(result.errors)} error(s)') + print(f'tests/{args.directory} + {len(root_modules())} root module(s): ' + f'{tally}, {elapsed:.1f}s elapsed') if __name__ == '__main__': From 3306b7e6383f58c23e543fafaf5db4face01e559 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 1 Oct 2026 22:35:41 -0400 Subject: [PATCH 4/6] docs: fix a dangling cross-reference and the ordering claim it sat in (#981) `util/run_unittest_leg.py`'s docstring referenced `:data:`ROOT_MODULES``, a name the module does not define -- the real names are the `root_modules()` function and the `_EXCLUDED_ROOT_MODULES` frozenset. Same defect class as the `:data:`LEGS`` references round 1 caught: a dangling Sphinx target renders as plain text and warns about nothing, since the docs build sets neither `-n` nor `-W` and does not take `util/` in at all. Re-pointed at `:func:`root_modules``, which is what the sentence means -- the set of modules that run every invocation. That is also more precise than the old target implied, since `root_modules()` excludes `tests.test_tier_guard_xdist` and a constant listing every module would not have. Fixing the reference exposed a worse claim in the same sentence: it had the ordering backwards. It read "ahead of the directory's own modules -- not interleaved, and not after", while `build_suite()` queues `leg_modules()` first and `root_modules()` second. Measured on both the `dumpkit` and `cli` legs: the root-level modules occupy the last five positions, after the directory's own. Three other places in the tree already said so correctly -- `build_suite()`'s own docstring, the `_EXCLUDED_ROOT_MODULES` comment, and the workflow's "directory first and root second" -- as did the next sentence but one of this very paragraph, so the docstring was contradicting itself. Now reads "after the directory's own modules -- not before, and not interleaved", which is the order #981 reproduces in and the only order that reproduces it. Audited every remaining cross-reference in the file by resolving each target: 33 role occurrences, 25 of them resolvable roles, all 25 now resolve. Comment and docstring only; the workflow file is untouched this round. `py_compile` clean, and the `cli` leg still passes (185 tests, 18.3s). --- util/run_unittest_leg.py | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/util/run_unittest_leg.py b/util/run_unittest_leg.py index 1771e151d..251239013 100644 --- a/util/run_unittest_leg.py +++ b/util/run_unittest_leg.py @@ -65,20 +65,19 @@ some 90x short of 29 GB, which is enough to place the OOM on running *everything* in one process rather than on any one directory. -Every :data:`ROOT_MODULES` module runs in *every* invocation, ahead of the -directory's own modules -- not interleaved, and not after. Issue #981's own +Every module :func:`root_modules` returns runs in *every* invocation, after the +directory's own modules -- not before, and not interleaved. Issue #981's own reproduction is a sibling module (``tests.protocols.application.test_http_unit``) -*polluting* a root-level module -(:mod:`tests.test_base_class_contract`) that runs after it in the same -process; ``tests.test_base_class_contract`` first and the directory second -would never reproduce that shape, because the pollution would land after the -sensitive module had already made its assertions and finished. Running the -directory first and the root modules second is what gives any purging module -in that directory a chance to desync a root module that assumes its own -import is current -- matching the reproduction exactly for ``protocols`` and -``const``, and giving the same opportunity to every other directory this -script is pointed at, most of which have never been tried in that -configuration before. +*polluting* a root-level module (:mod:`tests.test_base_class_contract`) that +runs after it in the same process; ``tests.test_base_class_contract`` first and +the directory second would never reproduce that shape, because the pollution +would land after the sensitive module had already made its assertions and +finished. Running the directory first and the root modules second is what gives +any purging module in that directory a chance to desync a root module that +assumes its own import is current -- matching the reproduction exactly for +``protocols`` and ``const``, and giving the same opportunity to every other +directory this script is pointed at, most of which have never been tried in +that configuration before. What this still does not catch: a defect running the *other* direction (a root module polluting a directory module), interference between two From fa6216ee70ae9d30f1718f3f02a069d172217eed Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 1 Oct 2026 23:10:52 -0400 Subject: [PATCH 5/6] docs(ci): correct round 2's false SIGINT/SIGTERM/SIGKILL timeout claim (#981) Problem: round 2's `finally`-block comment in util/run_unittest_leg.py drew an accurate pair of Python-signal facts (SIGKILL runs no Python code; SIGTERM has no default handler) into a false conclusion -- that a GitHub Actions `timeout-minutes` expiry is therefore not covered and a leg killed at the cap prints nothing. Neither signal is what the runner sends first. Fix: `actions/runner`'s `src/Runner.Sdk/ProcessInvoker.cs` sends SIGINT with a 7.5s grace period before SIGTERM (2.5s) and only then SIGKILL (`CancelAndKillProcessTree`, `_sigintTimeout`/`_sigtermTimeout`); `run:` steps select that ladder by passing `killProcessOnCancel: false` (`Handlers/ScriptHandler.cs`). Traced the job-level path too, not just the generic step-cancellation one the previous round verified: the backend's cancellation reaches the worker as a `CancelRequest` (`Runner.Worker/Worker.cs`), cancelling the token `JobRunner`/`StepsRunner` thread into the step's own `ExecutionContext.CancellationToken` -- the same token `ScriptHandler.cs` hands `ProcessInvoker.ExecuteAsync`. So a `timeout-minutes` expiry normally does still print, via the `KeyboardInterrupt` CPython raises for SIGINT, which `unittest.case._Outcome.testPartExecutor` re-raises rather than swallowing. Narrowed the real gap to a test blocked inside a C extension, which can defer signal delivery past SIGTERM/SIGKILL. No mirror of this claim exists in .github/workflows/unit-tests.yml -- round 2's diff there touched three other passages, not this one -- so that file is unchanged this round. Evidence: unittest.case._Outcome.testPartExecutor in this venv (Python 3.14.7) contains `except KeyboardInterrupt: raise`, verbatim. A real SIGINT sent to a child running the `cli` leg 3s in printed "tests/cli + 5 root module(s): interrupted before a result was available, 2.9s elapsed" and exited via signal 2, 0.148s after the signal. `py_compile` clean; `cli` leg still passes (185 tests, 18.4s elapsed) against the edited script. Refs #981 --- util/run_unittest_leg.py | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/util/run_unittest_leg.py b/util/run_unittest_leg.py index 251239013..efd8c2822 100644 --- a/util/run_unittest_leg.py +++ b/util/run_unittest_leg.py @@ -202,14 +202,29 @@ def main(argv: 'list[str] | None' = None) -> 'int': elapsed = time.monotonic() - start # In a ``finally`` so the measured number survives a leg that *fails* # and a leg that unwinds on an exception -- including the - # ``KeyboardInterrupt`` CPython's default SIGINT handler raises. It - # deliberately claims no more than that: SIGKILL never runs Python - # code at all, and SIGTERM has no default handler, so neither unwinds - # this block. A GitHub Actions ``timeout-minutes`` expiry (see the - # ``unittest-ordering`` job in .github/workflows/unit-tests.yml) is - # therefore *not* covered -- a leg killed at the cap prints nothing. - # What this line buys is the number from a run that completed close - # to the cap, which is what spares the re-run. + # ``KeyboardInterrupt`` CPython's default SIGINT handler raises, + # which :class:`unittest.case._Outcome`'s ``testPartExecutor`` + # re-raises rather than swallowing. A GitHub Actions + # ``timeout-minutes`` expiry (see the ``unittest-ordering`` job in + # .github/workflows/unit-tests.yml) normally *does* still reach this + # line: the runner sends SIGINT first, with a 7.5s grace period, + # before SIGTERM (2.5s) and only then SIGKILL -- + # ``actions/runner``'s ``src/Runner.Sdk/ProcessInvoker.cs`` + # (``CancelAndKillProcessTree``, ``_sigintTimeout``/ + # ``_sigtermTimeout``). A job-level timeout uses that same ladder, + # not a separate one: the backend's cancellation reaches the worker + # as a ``CancelRequest`` (``Runner.Worker/Worker.cs``), which cancels + # the token ``JobRunner``/``StepsRunner`` thread into this step's + # own ``ExecutionContext.CancellationToken`` -- the token + # ``Handlers/ScriptHandler.cs`` hands ``ProcessInvoker.ExecuteAsync`` + # with ``killProcessOnCancel: false``, which is what selects the + # SIGINT/SIGTERM ladder for a ``run:`` step like this one, rather + # than an immediate kill. The real gap is narrower: a test blocked + # inside a C extension defers signal delivery, so the handler may + # not run before SIGTERM/SIGKILL follow it -- SIGKILL in particular + # still runs no Python code at all. What this line buys is the + # number from a run that finished, or was interrupted by SIGINT, + # close to the cap -- not one genuinely stuck in C. tally = ('interrupted before a result was available' if result is None else f'{result.testsRun} test(s), {len(result.failures)} failure(s), ' f'{len(result.errors)} error(s)') From 69fd563a87c7839dfb524da2a676295f5a475c94 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 1 Oct 2026 23:33:08 -0400 Subject: [PATCH 6/6] docs(ci): drop the CI-timeout paragraph instead of correcting it again (#981) * The comment on `main`'s `finally` block has been wrong four times running, each fix introducing the next error: `pytest-subtests` masking the defect (not installed), the print surviving a kill (no `try`/`finally` existed yet), a timed-out leg printing nothing, and now a timeout expiry normally reaching the line. * The fourth is wrong too. Every round reasoned about which signal is sent and how Python handles it; none asked which process is signalled. `ProcessInvoker.SendSignal` is `kill()` on one positive pid -- no `killpg`/`setsid` on the Unix path -- and that pid is the step's shell, since the step is a bare `run:` and Python is bash's child. Non-interactive bash does not forward SIGINT to a foreground child (measured: child alive 2.5s after `kill -INT` of the bash pid), so both grace windows expire with the leg running and SIGKILL lands on bash. * So delete the paragraph rather than rewrite it a fifth time: a CI timeout says nothing about whether this line prints, and the claim was never load-bearing. What remains needs no CI justification -- the `finally` is there so a failing leg, and one unwinding on an exception, still report elapsed time. The residual "number from a run that finished" clause goes with it; a finished run prints by the normal path regardless. Comment-only: tokenising before and after, excluding comments and docstrings, gives an identical 552-token sequence. `py_compile` clean, workflow file byte-identical, `cli` leg still prints its elapsed line. --- util/run_unittest_leg.py | 30 +++++------------------------- 1 file changed, 5 insertions(+), 25 deletions(-) diff --git a/util/run_unittest_leg.py b/util/run_unittest_leg.py index efd8c2822..e31ff90bc 100644 --- a/util/run_unittest_leg.py +++ b/util/run_unittest_leg.py @@ -200,31 +200,11 @@ def main(argv: 'list[str] | None' = None) -> 'int': return 0 if result.wasSuccessful() else 1 finally: elapsed = time.monotonic() - start - # In a ``finally`` so the measured number survives a leg that *fails* - # and a leg that unwinds on an exception -- including the - # ``KeyboardInterrupt`` CPython's default SIGINT handler raises, - # which :class:`unittest.case._Outcome`'s ``testPartExecutor`` - # re-raises rather than swallowing. A GitHub Actions - # ``timeout-minutes`` expiry (see the ``unittest-ordering`` job in - # .github/workflows/unit-tests.yml) normally *does* still reach this - # line: the runner sends SIGINT first, with a 7.5s grace period, - # before SIGTERM (2.5s) and only then SIGKILL -- - # ``actions/runner``'s ``src/Runner.Sdk/ProcessInvoker.cs`` - # (``CancelAndKillProcessTree``, ``_sigintTimeout``/ - # ``_sigtermTimeout``). A job-level timeout uses that same ladder, - # not a separate one: the backend's cancellation reaches the worker - # as a ``CancelRequest`` (``Runner.Worker/Worker.cs``), which cancels - # the token ``JobRunner``/``StepsRunner`` thread into this step's - # own ``ExecutionContext.CancellationToken`` -- the token - # ``Handlers/ScriptHandler.cs`` hands ``ProcessInvoker.ExecuteAsync`` - # with ``killProcessOnCancel: false``, which is what selects the - # SIGINT/SIGTERM ladder for a ``run:`` step like this one, rather - # than an immediate kill. The real gap is narrower: a test blocked - # inside a C extension defers signal delivery, so the handler may - # not run before SIGTERM/SIGKILL follow it -- SIGKILL in particular - # still runs no Python code at all. What this line buys is the - # number from a run that finished, or was interrupted by SIGINT, - # close to the cap -- not one genuinely stuck in C. + # In a ``finally`` so a leg that *fails*, and a leg that unwinds on an + # exception -- including the ``KeyboardInterrupt`` CPython's default + # SIGINT handler raises, which :class:`unittest.case._Outcome`'s + # ``testPartExecutor`` re-raises rather than swallowing -- still + # reports its elapsed time. tally = ('interrupted before a result was available' if result is None else f'{result.testsRun} test(s), {len(result.failures)} failure(s), ' f'{len(result.errors)} error(s)')