From 6e7c998df70f893b110abdd87e577098de027c5e Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 22 Sep 2026 13:30:02 -0400 Subject: [PATCH] fix(tests): restore sys.modules after a test installs a stand-in module (#660) Twelve tests in ``tests/project/`` failed with ``TypeError: type 'ProtocolBase' is not subscriptable`` depending only on what had run before them: the same three files passed in the reverse order. ``tests/_support.py``'s fake-module helpers isolated a test by purging ``sys.modules`` on entry and never restoring on exit, so a stand-in ``ProtocolBase`` that is not ``Generic`` stayed bound after the test that installed it, and every later ``class X(Protocol[...])`` raised at ``pcapkit/protocols/misc/pcap/frame.py:59``. * ``tests/_support.py`` gains ``snapshot_modules``/``restore_modules`` and ``isolate_modules``, which purge on entry and restore on teardown via ``addCleanup``. ``purge_modules`` keeps its purge-only behaviour, which is correct for the ~120 callers that only re-import the real package. * The stand-in installers now require the running test and refuse to bind anything without active isolation, so the same mistake is a ``RuntimeError`` at the call rather than failures in another directory. * ``tests/conftest.py`` restores the ``pcapkit`` region after every test, which covers files that roll their own purge and never import ``tests._support`` -- ``tests/cli/test_main.py`` is one, and is left unchanged as the witness. ``pytest_sessionstart`` imports the package once so that restore is a no-op for tests that do not purge: ``tests/project/`` is 0.35s warm against 9.34s cold and 1.59s unguarded. * Five leaks fixed at the call site: ``tests/corekit/test_protochain.py``, ``tests/interface/test_core.py``, ``tests/protocols/transport/`` ``test_transport_unit.py``, ``tests/utilities/test_decorators.py``, and ``tests/integration/test_module_loading.py`` -- the last a second, differently failing leak not in the issue. * ``EndToEndTestCase.setUpClass`` re-imports after purging. Without it the per-test restore snapshots an empty table, costing that tier a 0.7s re-import on each of its 92 test methods instead of one per class. Deliberately not fixed by moving the file's sort position or leaning on a neighbour's purge: accidental healing by a neighbour is why this stayed hidden. Nine new tests. The three-file order from the issue: exit 1, 12 failed 5 passed before; exit 0, 17 passed after. Cross-tier selection 156 -> 165 passed, both exit 0. --- tests/_support.py | 221 +++++++++++++++++- tests/conftest.py | 110 ++++++++- tests/corekit/test_protochain.py | 9 +- tests/integration/_helpers.py | 32 ++- tests/integration/test_module_loading.py | 8 +- tests/interface/test_core.py | 8 +- tests/project/test_module_isolation.py | 159 +++++++++++++ .../transport/test_transport_unit.py | 11 +- tests/test_support_helpers.py | 185 ++++++++++++++- tests/utilities/test_decorators.py | 13 +- 10 files changed, 734 insertions(+), 22 deletions(-) create mode 100644 tests/project/test_module_isolation.py diff --git a/tests/_support.py b/tests/_support.py index a431bf9564..e78d34adb6 100644 --- a/tests/_support.py +++ b/tests/_support.py @@ -202,7 +202,35 @@ def bootstrap_core_modules() -> dict[str, object]: } -def install_fake_protocol_module() -> type: +def install_fake_protocol_module(test: 'unittest.TestCase') -> type: + """Bind a non-generic ``ProtocolBase`` stand-in over the real one. + + The stand-in shares the real class's :attr:`~type.__name__` but is *not* + :class:`~typing.Generic`, so every ``class X(Protocol[...])`` in the package + raises :exc:`TypeError` while it is installed. That is the point -- the + tests using it are unit-testing :mod:`pcapkit.corekit.protochain` against a + cheap stand-in rather than importing the library -- but it also means the + stand-in surviving the test that installed it breaks every later test that + imports :mod:`pcapkit`. See :func:`isolate_modules` for the mechanism and + issue #660 for what it looked like when it was missing. + + Args: + test: The running test. Must already be under :func:`isolate_modules`, + which is what puts :data:`sys.modules` back afterwards. Required + rather than optional so that a future caller cannot install the + stand-in without arranging for its removal -- the mistake is a + :exc:`TypeError` at the call rather than twelve unrelated failures + in another directory. + + Returns: + The stand-in class, to subclass in the test. + + Raises: + RuntimeError: If ``test`` is not under :func:`isolate_modules`. + + """ + require_module_isolation(test, 'install_fake_protocol_module') + ensure_package('pcapkit.protocols', ROOT / 'pcapkit' / 'protocols') protocol_module = types.ModuleType('pcapkit.protocols.protocol') @@ -229,7 +257,25 @@ def expand_comp(cls, value) -> tuple[object, ...]: return ProtocolBase -def install_fake_payload_protocols(raw_cls: type, null_cls: type) -> None: +def install_fake_payload_protocols(test: 'unittest.TestCase', raw_cls: type, + null_cls: type) -> None: + """Bind ``Raw`` and ``NoPayload`` stand-ins over the real payload protocols. + + Args: + test: The running test, for the same reason as in + :func:`install_fake_protocol_module` -- these two names are on the + import path of most of the package, so leaving stand-ins bound to + them poisons every later test that imports :mod:`pcapkit`. + raw_cls: Stand-in to bind as ``pcapkit.protocols.misc.raw.Raw``. + null_cls: Stand-in to bind as + ``pcapkit.protocols.misc.null.NoPayload``. + + Raises: + RuntimeError: If ``test`` is not under :func:`isolate_modules`. + + """ + require_module_isolation(test, 'install_fake_payload_protocols') + ensure_package('pcapkit.protocols.misc', ROOT / 'pcapkit' / 'protocols' / 'misc') raw_module = types.ModuleType('pcapkit.protocols.misc.raw') @@ -268,9 +314,178 @@ class objects, and their creation churns the C-level ``_abc_impl`` caches reset(obj) +#: Default :data:`sys.modules` prefixes the isolation helpers below cover, and +#: the ones :func:`tests.conftest.restore_module_table` puts back after every +#: test. Only the package under test: a test that imports a third-party module +#: for the first time is not polluting anything, and dropping, say, ``scapy`` +#: from :data:`sys.modules` between tests would cost a re-import for no gain. +ISOLATED_PREFIXES = ('pcapkit',) + +#: Attribute :func:`isolate_modules` sets on the test it is given, and that +#: :func:`require_module_isolation` looks for. Private by name because nothing +#: outside this module should read it. +_ISOLATION_FLAG = '_pcapkit_module_isolation' + + +def _under_prefix(name: str, prefixes: 'tuple[str, ...]') -> bool: + """Whether ``name`` is one of ``prefixes`` or a submodule of one.""" + return any(name == prefix or name.startswith(prefix + '.') for prefix in prefixes) + + +def snapshot_modules(prefixes: Iterable[str] = ISOLATED_PREFIXES) -> 'dict[str, types.ModuleType]': + """The :data:`sys.modules` entries currently under ``prefixes``. + + Args: + prefixes: Module-name prefixes to capture, matched as in + :func:`purge_modules`. + + Returns: + A new mapping of name to module object. The *objects* are shared with + :data:`sys.modules`, which is what makes :func:`restore_modules` a + restore rather than a re-import: putting the same object back leaves + every class it holds identical, so an ``isinstance`` check against a + class captured before the test still answers the same afterwards. + + """ + prefixes = tuple(prefixes) + return {name: module for name, module in list(sys.modules.items()) + if _under_prefix(name, prefixes)} + + +def restore_modules(snapshot: 'dict[str, types.ModuleType]', + prefixes: Iterable[str] = ISOLATED_PREFIXES) -> None: + """Put the ``prefixes`` region of :data:`sys.modules` back to ``snapshot``. + + The inverse of :func:`snapshot_modules` over the *bindings*, and exact in + both directions: a name the test added is removed, a name it dropped is put + back, and a name it rebound is bound to what it held before. Nothing outside + ``prefixes`` is touched. + + Bindings are the whole of it, though, and the limit is worth stating. This + restores which module object a name refers to; it does not restore the + *contents* of a module object. A test that reaches into an already-imported + module and mutates it in place -- adding an entry to a registry dict, say -- + rebinds nothing, so there is nothing here to undo and the mutation outlives + the test. Isolating against that needs a purge, so that the next import + rebuilds the module from source, which is what the callers of + :func:`purge_modules` are doing. Issue #660 was a rebinding, which is why + this is the right shape for it. + + Args: + snapshot: The mapping :func:`snapshot_modules` returned. + prefixes: The prefixes it was taken over. Passing a wider set than was + snapshotted would delete modules that were never captured, so the + two calls have to agree -- which is why :func:`isolate_modules` + makes both of them rather than leaving it to the caller. + + """ + prefixes = tuple(prefixes) + for name in list(sys.modules): + if _under_prefix(name, prefixes) and name not in snapshot: + sys.modules.pop(name, None) + for name, module in snapshot.items(): + sys.modules[name] = module + # For the same reason :func:`purge_modules` does it: whatever the test + # imported while the region was purged built a second set of ``Mapping`` + # subclasses and churned the shared ABC caches, and those stale answers + # outlive the modules that caused them. + _reset_abc_caches() + + +def isolate_modules(test: 'unittest.TestCase', + prefixes: Iterable[str] = ISOLATED_PREFIXES) -> None: + """Purge ``prefixes`` for the duration of ``test``, and restore them after. + + This is :func:`purge_modules` with the other half attached, and it is what + every test that stands something in for a real module wants instead. The + difference is the whole of issue #660: purging on the way *in* protects the + test that does it and nothing else, so a test that replaces + ``pcapkit.protocols.protocol`` with a stand-in and then finishes leaves that + stand-in bound for whatever runs next. Twelve tests in + :mod:`tests.project.test_public_api` and + :mod:`tests.project.test_documentation_claims` failed with ``TypeError: type + 'ProtocolBase' is not subscriptable`` on exactly that, and only when the + file that installed the stand-in happened to run before them. + + Restoring on the way *out* makes the order irrelevant, which is the only + fix worth having here: the failure was hidden for as long as it was because + some *other* test's purge usually healed the state before anything noticed, + so any fix that leaves the healing accidental -- moving a file so it sorts + elsewhere, relying on a sibling to purge -- leaves the defect in place and + merely re-hides it. + + Registered with :meth:`~unittest.TestCase.addCleanup` rather than done in a + ``tearDown``, so it also runs when ``setUp`` itself raises part-way through + installing the stand-ins, and so a subclass cannot forget to call ``super``. + + Args: + test: The running test, or anything else exposing + :meth:`~unittest.TestCase.addCleanup`. + prefixes: Module-name prefixes to isolate. + + """ + snapshot = snapshot_modules(prefixes) + setattr(test, _ISOLATION_FLAG, True) + # Registered *before* the purge, so the restore still happens if the purge + # itself raises half-way through the table. + test.addCleanup(_release_module_isolation, test, snapshot, tuple(prefixes)) + purge_modules(prefixes) + + +def _release_module_isolation(test: 'unittest.TestCase', + snapshot: 'dict[str, types.ModuleType]', + prefixes: 'tuple[str, ...]') -> None: + """Restore ``snapshot`` and mark ``test`` as no longer isolated.""" + try: + restore_modules(snapshot, prefixes) + finally: + setattr(test, _ISOLATION_FLAG, False) + + +def require_module_isolation(test: 'unittest.TestCase', helper: str) -> None: + """Refuse to install a stand-in for a test that has not arranged its removal. + + Args: + test: The test the caller was handed. + helper: Name of the calling helper, for the error message. + + Raises: + RuntimeError: If ``test`` never called :func:`isolate_modules`, or has + already released its isolation. + + """ + if not getattr(test, _ISOLATION_FLAG, False): + raise RuntimeError( + f'{helper}() needs {type(test).__name__} to be under ' + f'tests._support.isolate_modules(self) first -- otherwise the stand-in it ' + f'binds outlives this test and breaks whatever imports pcapkit next. ' + f'Call isolate_modules(self) in setUp instead of purge_modules([...]); ' + f'see issue #660.' + ) + + def purge_modules(prefixes: Iterable[str]) -> None: + """Drop every :data:`sys.modules` entry under ``prefixes``. + + Purging only, with nothing put back: a test calling this protects *itself* + from what ran before it and makes no promise to what runs after. That is + enough for the majority of callers, which purge so that the next ``import + pcapkit`` re-runs the package from source and then import nothing unusual -- + a re-imported real module is not pollution. + + It is *not* enough for a test that binds a stand-in over a real module name. + Use :func:`isolate_modules` there, which snapshots first and restores on + teardown. + + Args: + prefixes: Module-name prefixes. A name matches when it equals a prefix + or begins with the prefix followed by a dot, so ``'pcapkit'`` takes + ``pcapkit`` and ``pcapkit.corekit.protochain`` but not + ``pcapkit_extra``. + + """ for name in list(sys.modules): - if any(name == prefix or name.startswith(prefix + '.') for prefix in prefixes): + if _under_prefix(name, tuple(prefixes)): sys.modules.pop(name, None) _reset_abc_caches() diff --git a/tests/conftest.py b/tests/conftest.py index d5e89f8c2a..417e0b531b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,7 +1,11 @@ # -*- coding: utf-8 -*- """Suite-wide :program:`pytest` configuration. -Holds one thing: the collection-time half of the tier guard described in +Holds two things. The first is :func:`restore_module_table`, the guard that puts +the :mod:`pcapkit` region of :data:`sys.modules` back after every test; see its +own docstring for why it is here rather than left to each test file. + +The second is the collection-time half of the tier guard described in :mod:`tests._tiers`. Every unit-tier module about to be run is read and checked for a ``sample_path('...')`` call naming a capture git does not track, and the run is stopped before any test executes if one is found. @@ -23,12 +27,14 @@ """ from __future__ import annotations +import importlib import pathlib import warnings from typing import TYPE_CHECKING import pytest +from tests._support import ISOLATED_PREFIXES, restore_modules, snapshot_modules from tests._tiers import (TierGuardWarning, audit_module, guard_unavailable_reason, is_unit_tier) @@ -36,6 +42,108 @@ from typing import Iterable, Iterator, Optional +def pytest_sessionstart(session: 'pytest.Session') -> 'None': + """Import :mod:`pcapkit` once, before the first test takes a snapshot. + + Purely a performance measure, and it belongs to + :func:`restore_module_table`: that fixture restores whatever the region held + when the test began, so what it restores *to* decides what the next test has + to re-import. Left cold, the region is empty at the first snapshot and + restored to empty after every test, which makes each of the tests that does + not purge for itself -- :mod:`tests.project` is the bulk of them -- pay a + fresh ``import pcapkit`` it used to get from the warm table. Measured over + the 96 tests of ``tests/project/``: 1.59s with no guard at all, **9.34s** + guarded from a cold table, **0.35s** guarded from a warm one. The last is the + fastest of the three because the import it would otherwise do inside the + first test has moved here instead. + + This does **not** help a class that purges in + :meth:`~unittest.TestCase.setUpClass`, and it is worth being clear about why, + because the obvious reading is wrong. Such a class purges *after* this hook + and *before* the first snapshot, so its snapshot is whatever it left -- + warming the table earlier changes nothing about it. Where the class loads + something straight after its own purge the snapshot is populated anyway and + nothing is lost; where it purges and defers the import to its test methods, + every one of them re-imports. Exactly one class in the suite did the latter, + :class:`tests.integration._helpers.EndToEndTestCase`, at a measured cost of + some 45s across its 92 test methods, and it now re-imports in its own + ``setUpClass`` rather than deferring. A future class that purges per class + should do the same. + + Warming it also means the restore is a no-op for the common case. A test + that imports the library and nothing else leaves the region exactly as it + found it, so there is nothing to put back. + + Failure here is not an error. A checkout without the runtime dependencies + installed cannot import the package at all -- which is a supported way to run + the parts of the suite that are guarded by ``skipUnless`` -- so this degrades + to the cold baseline rather than taking the run down with it. + + Args: + session: The pytest session, unused; the hook's signature requires it. + + """ + try: + importlib.import_module('pcapkit') + except Exception: # pragma: no cover # pylint: disable=broad-except + # Deliberately broad: anything at all going wrong here should cost + # nothing but the optimisation. ``BaseException`` is not caught, so an + # interrupt during startup still ends the run. + pass + + +@pytest.fixture(autouse=True) +def restore_module_table() -> 'Iterator[None]': + """Put the :mod:`pcapkit` region of :data:`sys.modules` back after every test. + + The suite tests a library by re-importing it from source over and over, and + a good deal of it works by binding a stand-in over a real module name -- + :func:`tests._support.install_fake_protocol_module` and its neighbours, the + hand-rolled equivalents in :mod:`tests.interface.test_core` and + :mod:`tests.cli.test_main`. :data:`sys.modules` is process-global, so every + one of those is a write that outlives the test unless something undoes it. + + Issue #660 is what that costs. A stand-in ``ProtocolBase`` that is not + :class:`~typing.Generic` stayed bound after the test that installed it, and + the next test to import :mod:`pcapkit` died on ``TypeError: type + 'ProtocolBase' is not subscriptable`` at + ``pcapkit/protocols/misc/pcap/frame.py:59`` -- twelve tests in + :mod:`tests.project`, and only when the polluting file happened to be + collected first. Three separate files turned out to leak this way, one of + which does not import :mod:`tests._support` at all. + + Hence a guard here rather than a ``tearDown`` in each of them. The three + known leaks are also fixed at their call sites, with + :func:`tests._support.isolate_modules`, because that is the honest fix and it + holds under :mod:`unittest` as well; but a per-file fix only covers the files + that have it, and the next one written without it would reintroduce the same + order-dependent failure. This covers every test that exists and every test + that will be written, which is the difference between the failure being + unlikely and being impossible. + + Deliberately *not* fixed by moving ``test_protochain.py`` so it sorts + elsewhere, or by leaning on a neighbouring test's purge to heal the state. + Accidental healing by a neighbour is precisely why this survived for as long + as it did -- the full CI selection passes because unrelated directories sort + between the polluter and its victims -- so a fix of that shape would hide the + defect again rather than remove it. + + Function-scoped, so :meth:`~unittest.TestCase.setUpClass` runs *inside* the + snapshot: pytest instantiates class-scoped fixtures before function-scoped + ones, so a class that purges once for all its tests has that purge captured + here and restored to, rather than undone between its own tests. + + Yields: + Nothing; the restore happens on the way out. + + """ + snapshot = snapshot_modules(ISOLATED_PREFIXES) + try: + yield + finally: + restore_modules(snapshot, ISOLATED_PREFIXES) + + def _collected_modules(items: 'Iterable[pytest.Item]') -> 'Iterator[pathlib.Path]': """Distinct module paths behind ``items``, in collection order. diff --git a/tests/corekit/test_protochain.py b/tests/corekit/test_protochain.py index db4d60b1ca..abe86b155a 100644 --- a/tests/corekit/test_protochain.py +++ b/tests/corekit/test_protochain.py @@ -2,13 +2,16 @@ import unittest -from tests._support import bootstrap_core_modules, install_fake_protocol_module, purge_modules +from tests._support import bootstrap_core_modules, install_fake_protocol_module, isolate_modules class ProtoChainTests(unittest.TestCase): def setUp(self) -> None: - purge_modules(['pcapkit']) - self.ProtocolBase = install_fake_protocol_module() + # ``isolate_modules`` rather than ``purge_modules``: the stand-in + # ``ProtocolBase`` installed below is not ``Generic``, so leaving it in + # ``sys.modules`` breaks every later test that imports pcapkit. See #660. + isolate_modules(self) + self.ProtocolBase = install_fake_protocol_module(self) modules = bootstrap_core_modules() self.protochain = modules['protochain'] self.exceptions = modules['exceptions'] diff --git a/tests/integration/_helpers.py b/tests/integration/_helpers.py index 4046daa644..16aab01910 100644 --- a/tests/integration/_helpers.py +++ b/tests/integration/_helpers.py @@ -65,14 +65,44 @@ def setUpClass(cls) -> None: """Drop the imported library so the class starts from a clean state. The surrounding tiers purge in :meth:`setUp`, i.e. once per test. A - fresh :mod:`pcapkit` import measures at roughly 0.45s on this machine, + fresh :mod:`pcapkit` import measures at roughly 0.7s on this machine, which across this tier would cost more than the extractions themselves, so the purge happens once per class instead. That is equivalent here: every test below imports :mod:`pcapkit` inside the test method, so none of them depends on what an earlier test left in :data:`sys.modules`. + The re-import on the last line is what keeps it once per class, and it + has to happen here rather than being left to the first test. + :func:`tests.conftest.restore_module_table` snapshots the module table + immediately after this method and restores to that snapshot after every + test, so purging without re-importing makes the snapshot an *empty* + table -- and then all 92 of this tier's test methods pay the 0.7s each, + rather than one per class across its 28 classes. Measured with a + throwaway subclass of this class whose second and third test methods + report whether ``pcapkit`` is still in :data:`sys.modules`: ``True`` on + ``mainline`` and ``True`` with this line, ``False`` without it. The other + ``setUpClass`` methods in the suite are unaffected because each already + loads something immediately after its own purge, which populates the + table before the snapshot is taken. + + The re-import is guarded because it is an optimisation and nothing more, + so it must not be able to turn a test failure into a class error. Most + subclasses carry ``@skipUnless(HAS_RUNTIME, ...)`` and never reach this + method without the runtime dependencies installed, but + ``PlistRoundTripTests`` and ``PcapngUnescapedKeyTests`` do not, so on a + checkout without them an unguarded ``import`` here would raise + :exc:`ModuleNotFoundError` out of ``setUpClass`` and error the whole + class, where before it was the individual tests that failed. Swallowed, + the table simply stays cold and those tests fail exactly as they did. + :exc:`BaseException` is deliberately not caught, for the same reason as + in :func:`tests._support._close_quietly`. + """ purge_modules(['pcapkit']) + try: + importlib.import_module('pcapkit') + except Exception: # pragma: no cover # pylint: disable=broad-except + pass def setUp(self) -> None: """Hand the test a private scratch directory.""" diff --git a/tests/integration/test_module_loading.py b/tests/integration/test_module_loading.py index 65dc4ddb91..3707d7942a 100644 --- a/tests/integration/test_module_loading.py +++ b/tests/integration/test_module_loading.py @@ -2,12 +2,16 @@ import unittest -from tests._support import bootstrap_core_modules, purge_modules +from tests._support import bootstrap_core_modules, isolate_modules class ModuleLoadingIntegrationTests(unittest.TestCase): def test_core_modules_bootstrap_together(self) -> None: - purge_modules(['pcapkit']) + # ``isolate_modules`` rather than ``purge_modules``: ``bootstrap_core_modules`` + # leaves a stub ``pcapkit`` -- a bare module object carrying only + # ``__path__`` -- bound in ``sys.modules``, so a later ``import pcapkit`` + # silently gets a package with none of its public names on it. See #660. + isolate_modules(self) modules = bootstrap_core_modules() self.assertIn('compat', modules) diff --git a/tests/interface/test_core.py b/tests/interface/test_core.py index 4c5ba09d0a..645ead2b64 100644 --- a/tests/interface/test_core.py +++ b/tests/interface/test_core.py @@ -7,7 +7,7 @@ import warnings from unittest import mock -from tests._support import load_module, purge_modules, sample_path +from tests._support import isolate_modules, load_module, purge_modules, sample_path RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) @@ -71,7 +71,11 @@ def __init__(self, packet, layers=0) -> None: class InterfaceCoreTests(unittest.TestCase): def setUp(self) -> None: - purge_modules(['pcapkit']) + # ``isolate_modules`` rather than ``purge_modules``: ``_load_module`` below + # binds a non-generic ``ProtocolBase`` stand-in and five other stand-in + # modules under ``pcapkit.*`` names, none of which may outlive the test + # that installed them. See #660. + isolate_modules(self) def _load_module(self): import sys diff --git a/tests/project/test_module_isolation.py b/tests/project/test_module_isolation.py new file mode 100644 index 0000000000..9e5c696e3d --- /dev/null +++ b/tests/project/test_module_isolation.py @@ -0,0 +1,159 @@ +# -*- coding: utf-8 -*- +"""The suite's result does not depend on the order its files were collected in. + +Issue #660: three files, run in one order, gave ``12 failed, 5 passed``; the same +three in the opposite order gave ``17 passed``. The cause was a test that bound a +non-generic ``ProtocolBase`` stand-in into :data:`sys.modules` and finished +without removing it, so the next test to import :mod:`pcapkit` died at +``pcapkit/protocols/misc/pcap/frame.py:59`` with ``TypeError: type 'ProtocolBase' +is not subscriptable``. + +The tests here run :program:`pytest` in a subprocess over the exact selections +that failed. That is the only way to state this invariant: it is a property of +one run of several files against each other, not of anything observable from +inside a single test. Cross-file ordering is also precisely what a normal test +cannot reach -- by the time it executes, the polluting file either has or has not +already run, and the guard under test is the thing that makes the difference +invisible. + +Each case is a *pair*: the order that failed, and one file that is known to +pollute paired with one that is known to import the package. Three distinct leaks +were found, and they are covered separately because they fail differently and are +fixed by different halves of the change: + +* :meth:`OrderIndependenceTests.test_the_reported_three_file_order_passes` is the + reproduction from the issue verbatim -- ``tests/corekit/test_protochain.py`` + leaking a fake ``ProtocolBase``. +* :meth:`OrderIndependenceTests.test_the_integration_tier_polluter_order_passes` + is a second leak, in ``tests/integration/test_module_loading.py``, found while + fixing the first and not mentioned in the issue. It fails differently -- a stub + ``pcapkit`` carrying only ``__path__``, so ``__all__`` entries resolve to + nothing rather than raising. +* :meth:`OrderIndependenceTests.test_a_polluter_outside_the_helpers_is_covered` + is the one that pins the *structural* half of the fix. + ``tests/cli/test_main.py`` rolls its own purge loop and never imports + :mod:`tests._support`, so nothing in that module could have fixed it; only + :func:`tests.conftest.restore_module_table` does. It is deliberately left as it + was, as the standing witness that the guard covers a file which has not opted + into anything. + +Why not simply move ``test_protochain.py`` so it sorts after its victims: because +that is what was already happening by accident. It sorts last inside +``tests/corekit/``, so no sibling healed it, and the full CI selection passed only +because unrelated directories sort between it and ``tests/project/``. A fix that +rearranges the collection order leaves the defect in place and re-hides it, and +the next file added anywhere between the two re-exposes it. + +This module is unit-tier by location but is not a unit test of anything: it +reads no sample capture, and every subprocess it starts is a narrow, named file +selection -- never the whole suite, which needs tens of gigabytes of memory. + +""" +from __future__ import annotations + +import os +import subprocess +import sys +import unittest + +from tests._tiers import ROOT + +#: Selections that failed before #660 was fixed, each as (label, files). The +#: polluting file comes first in every one, because that is the order that broke. +POLLUTING_ORDERS = [ + ( + 'the three files from the issue', + [ + 'tests/corekit/test_protochain.py', + 'tests/project/test_public_api.py', + 'tests/project/test_documentation_claims.py', + ], + ), + ( + 'the integration-tier bootstrap leak', + [ + 'tests/integration/test_module_loading.py', + 'tests/project/test_public_api.py', + ], + ), + ( + 'a polluter that does not use tests._support', + [ + 'tests/cli/test_main.py::CLIMainTests::test_get_parser_parses_expected_arguments', + 'tests/project/test_public_api.py', + ], + ), +] + + +def run_pytest(selection: 'list[str]') -> 'subprocess.CompletedProcess[str]': + """Run :program:`pytest` over ``selection`` in a subprocess. + + Args: + selection: Paths or node ids, relative to the repository root. + + Returns: + The finished process, with output captured. + + """ + environ = dict(os.environ) + # The child must import the tree this test is running from, not whatever an + # editable install happens to point at -- the whole question is about which + # ``pcapkit`` gets imported. + environ['PYTHONPATH'] = os.pathsep.join( + [str(ROOT), environ['PYTHONPATH']] if environ.get('PYTHONPATH') else [str(ROOT)] + ) + # Inherited from a parent run, this would have the child write to the same + # cache directory as the run that started it. + environ.pop('PYTEST_ADDOPTS', None) + environ.pop('PYTEST_CURRENT_TEST', None) + + return subprocess.run( + [sys.executable, '-m', 'pytest', '-p', 'no:cacheprovider', '-q', *selection], + cwd=str(ROOT), env=environ, capture_output=True, text=True, + timeout=600, check=False, + ) + + +class OrderIndependenceTests(unittest.TestCase): + """Each selection that used to fail on collection order now passes.""" + + def assert_selection_passes(self, label: str, selection: 'list[str]') -> None: + """Run ``selection`` and fail with its output if pytest did not exit 0.""" + finished = run_pytest(selection) + + self.assertEqual( + finished.returncode, 0, + f'pytest exited {finished.returncode} on {label} -- the result still ' + f'depends on collection order (#660).\n\n' + f'selection: {" ".join(selection)}\n\n' + f'stdout:\n{finished.stdout[-4000:]}\n\nstderr:\n{finished.stderr[-2000:]}') + self.assertNotIn('is not subscriptable', finished.stdout, + 'a stand-in ProtocolBase reached a later test (#660)') + + def test_the_reported_three_file_order_passes(self) -> None: + label, selection = POLLUTING_ORDERS[0] + self.assert_selection_passes(label, selection) + + def test_the_integration_tier_polluter_order_passes(self) -> None: + label, selection = POLLUTING_ORDERS[1] + self.assert_selection_passes(label, selection) + + def test_a_polluter_outside_the_helpers_is_covered(self) -> None: + label, selection = POLLUTING_ORDERS[2] + self.assert_selection_passes(label, selection) + + def test_the_reverse_order_passes_too(self) -> None: + """The control from the issue: the same files, victims first. + + This one passed before the fix as well, and is kept because it is what + makes the pair meaningful -- a change that broke it would have traded one + order dependence for another rather than removing it. + + """ + label, selection = POLLUTING_ORDERS[0] + self.assert_selection_passes(f'{label}, reversed', list(reversed(selection))) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/protocols/transport/test_transport_unit.py b/tests/protocols/transport/test_transport_unit.py index b832b9c006..e528efa74a 100644 --- a/tests/protocols/transport/test_transport_unit.py +++ b/tests/protocols/transport/test_transport_unit.py @@ -6,7 +6,7 @@ import unittest from unittest import mock -from tests._support import install_fake_payload_protocols, purge_modules +from tests._support import install_fake_payload_protocols, isolate_modules RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) @@ -15,7 +15,10 @@ @unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') class TransportUnitTests(unittest.TestCase): def setUp(self) -> None: - purge_modules(['pcapkit']) + # ``isolate_modules`` rather than ``purge_modules``: two tests below bind + # stand-ins over ``pcapkit.protocols.misc.raw`` and ``...misc.null``, which + # are on the import path of most of the package. See #660. + isolate_modules(self) def test_register_validates_protocols_and_abstract_base(self) -> None: from pcapkit.corekit.module import ModuleDescriptor @@ -172,7 +175,7 @@ def read(self, length: int | None = None, **kwargs: object) -> object: def make(self, **kwargs: object) -> object: raise NotImplementedError - install_fake_payload_protocols(FakeRaw, FakeNoPayload) + install_fake_payload_protocols(self, FakeRaw, FakeNoPayload) with mock.patch('pcapkit.protocols.transport.transport.logger.error') as logger_error: report = DummyTransport.analyze((80, 60000), b'data') @@ -216,7 +219,7 @@ def read(self, length: int | None = None, **kwargs: object) -> object: def make(self, **kwargs: object) -> object: raise NotImplementedError - install_fake_payload_protocols(FakeRaw, FakeNoPayload) + install_fake_payload_protocols(self, FakeRaw, FakeNoPayload) with mock.patch('pcapkit.protocols.transport.transport.logger.error') as logger_error: report = DummyTransport.analyze((443, 65000), b'body') diff --git a/tests/test_support_helpers.py b/tests/test_support_helpers.py index d159e5baae..4f5f441d03 100644 --- a/tests/test_support_helpers.py +++ b/tests/test_support_helpers.py @@ -24,17 +24,29 @@ * nothing it is handed in teardown makes it raise (:class:`ToleranceTests`). +The module-isolation helpers are pinned here too, for the same reason and with +more cause: :class:`SnapshotRestoreTests` and +:class:`StandInsDoNotOutliveTheirTestTests` cover +:func:`~tests._support.isolate_modules` and the snapshot/restore pair beneath it, +which exist because issue #660 -- twelve failures in :mod:`tests.project`, +visible only in some collection orders -- was a stand-in module left bound in +:data:`sys.modules` by a test that had finished. + This module is unit-tier: it drives the helper with stand-ins rather than real extractors, so it reads no sample capture and needs no engine installed. """ from __future__ import annotations +import importlib import signal +import sys import time +import types import unittest -from tests._support import close_extractor, time_limit +from tests._support import (close_extractor, install_fake_protocol_module, isolate_modules, + restore_modules, snapshot_modules, time_limit) class Closeable: @@ -252,5 +264,176 @@ def test_nothing_is_re_armed_when_nothing_was_pending(self) -> None: self.assertEqual(signal.alarm(0), 0) +class SnapshotRestoreTests(unittest.TestCase): + """:func:`~tests._support.restore_modules` is an exact inverse. + + Three ways a test can leave the table different, and the restore has to undo + all three: a name it added, a name it dropped, and a name it rebound to + something else. The third is the one issue #660 turned on -- a stand-in bound + over ``pcapkit.protocols.protocol`` is a rebind, and a restore that only + removed additions would leave it in place. + + Every name used here is under ``pcapkit.`` so the helpers actually match it, + and every one is removed again on teardown whatever the assertions do. + + """ + + ADDED = 'pcapkit.__support_probe_added' + REBOUND = 'pcapkit.__support_probe_rebound' + DROPPED = 'pcapkit.__support_probe_dropped' + + def setUp(self) -> None: + self.addCleanup(self._forget_probes) + self.original = types.ModuleType(self.REBOUND) + self.dropped = types.ModuleType(self.DROPPED) + sys.modules[self.REBOUND] = self.original + sys.modules[self.DROPPED] = self.dropped + + def _forget_probes(self) -> None: + for name in (self.ADDED, self.REBOUND, self.DROPPED): + sys.modules.pop(name, None) + + def test_restore_undoes_an_addition_a_drop_and_a_rebind(self) -> None: + snapshot = snapshot_modules(['pcapkit']) + + sys.modules[self.ADDED] = types.ModuleType(self.ADDED) + sys.modules[self.REBOUND] = types.ModuleType(self.REBOUND) + del sys.modules[self.DROPPED] + + restore_modules(snapshot, ['pcapkit']) + + self.assertNotIn(self.ADDED, sys.modules) + self.assertIs(sys.modules[self.REBOUND], self.original) + self.assertIs(sys.modules[self.DROPPED], self.dropped) + + def test_the_snapshot_is_the_whole_pcapkit_region_and_nothing_else(self) -> None: + snapshot = snapshot_modules(['pcapkit']) + + self.assertIn(self.REBOUND, snapshot) + self.assertTrue(all(name == 'pcapkit' or name.startswith('pcapkit.') + for name in snapshot), + 'snapshot_modules captured a name outside the prefixes it was given') + # A prefix match is on dotted components, not on the string: a + # differently-named top-level package that merely starts with the same + # letters is a different package and must not be swept up. + sibling = 'pcapkit_not_ours' + sys.modules[sibling] = types.ModuleType(sibling) + self.addCleanup(sys.modules.pop, sibling, None) + + self.assertNotIn(sibling, snapshot_modules(['pcapkit'])) + + restore_modules(snapshot_modules(['pcapkit']), ['pcapkit']) + self.assertIn(sibling, sys.modules) + + +class StandInsDoNotOutliveTheirTestTests(unittest.TestCase): + """A test that installs a stand-in leaves :data:`sys.modules` as it found it. + + The regression test for issue #660, and written the way it is on purpose. + The real polluting test case is *run* here, through + :class:`unittest.TestResult`, exactly as a runner would run it -- so what is + measured is the test case's own isolation and nothing else. Going through + :mod:`pytest` instead would prove nothing about it, because + :func:`tests.conftest.restore_module_table` would clean up after it either + way and the assertion would pass whether or not the test case had been + fixed. + + Before the fix this failed on any ordering, on the fake ``ProtocolBase`` and + the stub ``pcapkit`` that ``ProtoChainTests.setUp`` left behind. + + """ + + def setUp(self) -> None: + # This test case re-imports pcapkit into a table it then compares, so it + # gets the same isolation it is asserting about. + isolate_modules(self) + + def _run_and_diff(self, case: 'unittest.TestCase') -> 'unittest.TestResult': + """Run ``case`` and assert it changed no ``pcapkit.*`` binding.""" + before = snapshot_modules(['pcapkit']) + + result = unittest.TestResult() + case.run(result) + + after = snapshot_modules(['pcapkit']) + + self.assertEqual( + sorted(set(after) - set(before)), [], + 'the test left new pcapkit entries in sys.modules; whatever imports ' + 'pcapkit next inherits them (#660)') + self.assertEqual( + sorted(set(before) - set(after)), [], + 'the test dropped pcapkit entries from sys.modules and did not put them back') + self.assertEqual( + sorted(name for name in before if before[name] is not after.get(name)), [], + 'the test rebound a pcapkit name to a different module object and left it ' + 'rebound; a non-generic ProtocolBase stand-in left this way makes every ' + 'later `class X(Protocol[...])` raise TypeError (#660)') + return result + + def test_protochain_leaves_no_fake_protocol_module_behind(self) -> None: + from tests.corekit.test_protochain import ProtoChainTests + + case = ProtoChainTests(sorted(_method_names(ProtoChainTests))[0]) + + result = self._run_and_diff(case) + + # Asserted after the isolation check, so a genuine failure in that test + # case is reported as its own failure rather than as a leak here. + self.assertEqual((len(result.failures), len(result.errors)), (0, 0), + f'the borrowed test case did not pass: ' + f'{result.failures or result.errors}') + + def test_the_real_protocol_base_still_subscripts_afterwards(self) -> None: + """The failure mode itself, rather than the table it came from. + + ``pcapkit/protocols/misc/pcap/frame.py:59`` is the line that raised + ``TypeError: type 'ProtocolBase' is not subscriptable``, so importing it + after the polluting test case has run is the most direct statement of + what #660 was. + + """ + from tests.corekit.test_protochain import ProtoChainTests + + case = ProtoChainTests(sorted(_method_names(ProtoChainTests))[0]) + case.run(unittest.TestResult()) + + frame = importlib.import_module('pcapkit.protocols.misc.pcap.frame') + protocol = importlib.import_module('pcapkit.protocols.protocol') + + self.assertTrue(hasattr(frame, 'Frame')) + self.assertIsNotNone(getattr(protocol.ProtocolBase, '__class_getitem__', None), + 'pcapkit.protocols.protocol.ProtocolBase is not the real ' + 'generic class -- a stand-in is still bound over it (#660)') + + def test_installing_a_stand_in_without_isolation_is_refused(self) -> None: + """The other half: the mistake cannot be made quietly again. + + A future test file that calls the installer without arranging its removal + gets a :exc:`RuntimeError` naming the fix, in its own ``setUp``, instead + of a green run that breaks a different directory. + + """ + class Unisolated(unittest.TestCase): + def runTest(self) -> None: + pass + + with self.assertRaises(RuntimeError) as caught: + install_fake_protocol_module(Unisolated()) + + self.assertIn('isolate_modules', str(caught.exception)) + + +def _method_names(case_class: type) -> 'list[str]': + """Test-method names on ``case_class``, however many it happens to have. + + Read off the class rather than hard-coded, so that renaming or adding a test + in the borrowed module does not break this one. + + """ + return [name for name in dir(case_class) + if name.startswith('test') and callable(getattr(case_class, name))] + + if __name__ == '__main__': unittest.main() diff --git a/tests/utilities/test_decorators.py b/tests/utilities/test_decorators.py index c3bfe707d5..a49ff86178 100644 --- a/tests/utilities/test_decorators.py +++ b/tests/utilities/test_decorators.py @@ -3,12 +3,16 @@ import io import unittest -from tests._support import bootstrap_core_modules, install_fake_payload_protocols, purge_modules +from tests._support import (bootstrap_core_modules, install_fake_payload_protocols, + isolate_modules) class DecoratorTests(unittest.TestCase): def setUp(self) -> None: - purge_modules(['pcapkit']) + # ``isolate_modules`` rather than ``purge_modules``: ``bootstrap_core_modules`` + # binds partially-initialised real modules under ``pcapkit.*`` names, and + # ``_payload_stand_ins`` binds outright stand-ins. See #660. + isolate_modules(self) modules = bootstrap_core_modules() self.decorators = modules['decorators'] self.exceptions = modules['exceptions'] @@ -364,8 +368,7 @@ def unpack(cls, data, length=None, packet=None): # ``test_beholder_uses_the_protocol_payload_accessor_not_the_schema``. ########################################################################## - @staticmethod - def _payload_stand_ins(): + def _payload_stand_ins(self): """``Raw`` and ``NoPayload`` stand-ins recording what they were handed.""" class NoPayload: def __init__(self, file_, length, error=None, alias=None) -> None: @@ -377,7 +380,7 @@ def __init__(self, file_, length, error=None, alias=None) -> None: class Raw(NoPayload): pass - install_fake_payload_protocols(Raw, NoPayload) + install_fake_payload_protocols(self, Raw, NoPayload) return Raw, NoPayload @staticmethod