Skip to content

ci(tests): add a plain-unittest leg to catch conftest-masked ordering defects #984

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JarryShaw merged 6 commits into from
Oct 2, 2026
Merged

ci(tests): add a plain-unittest leg to catch conftest-masked ordering defects #984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0d2a20c
ci(tests): add a plain-unittest leg to catch pytest-subtests-masked o…
JarryShaw Oct 2, 2026
1323de1
fix(ci): correct the unittest-ordering job's pytest-subtests claim an…
JarryShaw Oct 2, 2026
c688633
docs(ci): correct three unverifiable prose claims in the unittest-ord…
JarryShaw Oct 2, 2026
3306b7e
docs: fix a dangling cross-reference and the ordering claim it sat in…
JarryShaw Oct 2, 2026
fa6216e
docs(ci): correct round 2's false SIGINT/SIGTERM/SIGKILL timeout clai…
JarryShaw Oct 2, 2026
69fd563
docs(ci): drop the CI-timeout paragraph instead of correcting it agai…
JarryShaw Oct 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 143 additions & 0 deletions .github/workflows/unit-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,6 +745,149 @@ jobs:
echo "Fixture-dependent selection: $selection"
python -m pytest -q -n auto --dist load $selection

# 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 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
# 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 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
# 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 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:
#
# * 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 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. 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
# 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
# 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:
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: 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 }}

# ``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
Expand Down
122 changes: 98 additions & 24 deletions tests/test_base_class_contract.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand All @@ -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':
Expand Down Expand Up @@ -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:
Expand Down
Loading
Loading