From 62b96d993411669a34269c1b6ba289e7709ebb28 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 9 Oct 2026 22:42:01 -0400 Subject: [PATCH] test: keep the per-class pcapkit import in 12 classes that re-imported per test (#1538) - 12 classes in 7 modules call reimport_once_per_class() in setUp and purged pcapkit in tearDown. tearDown runs before _keep_lazy_imports records the class's import, so every test re-imported (#1070 converted setUp only). Drop the tearDown purge. - SchemaMetaAbcCacheTests and DefaultDescriptorResolutionTests use restore=True, which keeps their documented confinement (the ABC-cache reset, the reloaded Raw) at class scope; their comments are rewritten to match. - test_dispatch_keys_did_not_move_with_the_modules takes a fresh import via isolate_modules(): a dispatch hit writes the resolved class back into the registry, so on the shared import it failed whenever a parsing test ran first. - New tests/project/test_per_class_import_guard.py AST-scans tests/ for the pairing. On main it fails and lists the 12 classes. The 7 modules in one serial pytest process: 71.8 s -> 15.4 s (mean of 3), with 68 passed and 63 subtests both times and pcapkit coverage unchanged. --- tests/project/test_per_class_import_guard.py | 206 ++++++++++++++++++ .../application/test_layer_placement_unit.py | 11 +- ..._ospf_cryptographic_authentication_unit.py | 5 +- tests/protocols/application/test_ospf_unit.py | 5 +- tests/protocols/application/test_rarp_unit.py | 5 +- .../test_schema_metaclass_abc_cache_unit.py | 27 +-- .../test_dispatch_default_resolution_unit.py | 16 +- tests/utilities/test_logging.py | 8 +- 8 files changed, 237 insertions(+), 46 deletions(-) create mode 100644 tests/project/test_per_class_import_guard.py diff --git a/tests/project/test_per_class_import_guard.py b/tests/project/test_per_class_import_guard.py new file mode 100644 index 0000000000..eadf777301 --- /dev/null +++ b/tests/project/test_per_class_import_guard.py @@ -0,0 +1,206 @@ +# -*- coding: utf-8 -*- +"""No test class undoes its own per-class :mod:`pcapkit` import. + +GitHub issue #1538. :func:`tests._support.reimport_once_per_class` (#1065) gives +a class one import of the package, shared by its tests: the first test purges, +and :func:`tests._support._keep_lazy_imports` -- a cleanup, so it runs *after* +``tearDown`` -- records what the test imported for the next one to reuse. A +``tearDown`` that also purges :mod:`pcapkit` empties :data:`sys.modules` before +that cleanup reads it, so the class keeps nothing and every test pays the full +re-import again (about 0.67s and 329 modules, by that helper's docstring) while +the ``setUp`` reads as if it did not. Twelve classes in seven files did this +on ``main`` after #1070 converted their ``setUp`` and left the ``tearDown``. + +A class that needs a fresh import for every test should say so by purging in +``setUp`` *instead of* calling :func:`~tests._support.reimport_once_per_class`, +as ``tests.const.test_const_enum_builtin_parity.ConstEnumRegisterFallbackTests`` +does. That form is not matched here, and neither is a narrow purge of one +submodule, such as ``tests/toolkit/test_scapy_unit.py`` purging +``pcapkit.toolkit.scapy`` from a test body: only a purge of the whole package +discards the class's import. + +This module is unit-tier: it parses source and never imports the package. + +""" +from __future__ import annotations + +import ast +import textwrap +import unittest +from typing import TYPE_CHECKING + +from tests._tiers import ROOT + +if TYPE_CHECKING: + from typing import Iterator, Optional + +#: Directory scanned for test classes. +TESTS = ROOT / 'tests' + +#: ``(path relative to the repository root, class name)`` -> why the class +#: genuinely needs both. Empty: the deliberate per-test form purges in ``setUp`` +#: without the helper, which this guard does not flag. Enforced both ways, so an +#: entry that stops pairing the two has to be removed. +ALLOWED = {} # type: dict[tuple[str, str], str] + +#: Fewest classes the scan must see calling the helper, so that a scan that +#: silently matches nothing cannot pass. It counted 384 when this was written. +MIN_CLASSES = 300 + + +def _callee(node: 'ast.AST') -> 'Optional[str]': + """The bare name a call is made through: ``f`` for ``f()`` and ``x.f()``.""" + if isinstance(node, ast.Name): + return node.id + if isinstance(node, ast.Attribute): + return node.attr + return None + + +def _is_whole_package(node: 'ast.AST') -> bool: + """Whether ``node`` names :mod:`pcapkit` itself among the prefixes to purge.""" + if isinstance(node, ast.Name): + return node.id == 'ISOLATED_PREFIXES' + if isinstance(node, (ast.List, ast.Tuple, ast.Set)): + return any(isinstance(item, ast.Constant) and item.value == 'pcapkit' for item in node.elts) + return False + + +def _whole_purges(method: 'ast.FunctionDef') -> 'Iterator[int]': + """Line numbers in ``method`` that purge, or register a purge of, all of :mod:`pcapkit`.""" + for node in ast.walk(method): + if not isinstance(node, ast.Call): + continue + name = _callee(node.func) + if name == 'purge_modules' and node.args and _is_whole_package(node.args[0]): + yield node.lineno + elif (name == 'addCleanup' and len(node.args) >= 2 and _callee(node.args[0]) == 'purge_modules' + and _is_whole_package(node.args[1])): + yield node.lineno + + +def _helper_calls(method: 'ast.FunctionDef') -> 'list[int]': + """Line numbers in ``method`` that call ``reimport_once_per_class``.""" + return [node.lineno for node in ast.walk(method) + if isinstance(node, ast.Call) and _callee(node.func) == 'reimport_once_per_class'] + + +def _resolve(classes: 'dict[str, ast.ClassDef]', cls: 'ast.ClassDef', + method: str) -> 'Optional[ast.FunctionDef]': + """``cls.method``, looked up through the bases defined in the same module.""" + seen = set() # type: set[str] + stack = [cls] + while stack: + current = stack.pop(0) + if current.name in seen: + continue + seen.add(current.name) + for item in current.body: + if isinstance(item, ast.FunctionDef) and item.name == method: + return item + stack.extend(classes[base.id] for base in current.bases + if isinstance(base, ast.Name) and base.id in classes) + return None + + +def scan(source: str) -> 'tuple[int, list[tuple[str, int]]]': + """Classes in ``source`` that call the helper, and those that also undo it. + + Args: + source: Module source text. + + Returns: + The number of classes whose ``setUp`` calls ``reimport_once_per_class``, + and ``(class name, line)`` for each of them that also purges the whole + package -- anywhere in ``tearDown``, or in ``setUp`` after the call. + + """ + tree = ast.parse(source) + classes = {node.name: node for node in ast.walk(tree) if isinstance(node, ast.ClassDef)} + using, offending = 0, [] + for cls in classes.values(): + set_up = _resolve(classes, cls, 'setUp') + calls = [] if set_up is None else _helper_calls(set_up) + if set_up is None or not calls: + continue + using += 1 + tear_down = _resolve(classes, cls, 'tearDown') + lines = [line for line in _whole_purges(set_up) if line > min(calls)] + if tear_down is not None: + lines.extend(_whole_purges(tear_down)) + if lines: + offending.append((cls.name, min(lines))) + return using, offending + + +def scan_tree() -> 'tuple[int, dict[tuple[str, str], int]]': + """:func:`scan` over every module under :data:`TESTS`, keyed by relative path.""" + using, found = 0, {} + for path in sorted(TESTS.rglob('*.py')): + count, offending = scan(path.read_text(encoding='utf-8')) + using += count + relative = path.relative_to(ROOT).as_posix() + for name, line in offending: + found[(relative, name)] = line + return using, found + + +class PerClassImportGuardTests(unittest.TestCase): + """The tree-wide check, and the detector's own edges.""" + + maxDiff = None + + def test_no_class_purges_the_import_it_keeps(self) -> None: + using, found = scan_tree() + self.assertGreaterEqual(using, MIN_CLASSES, 'the scan found too few classes to be trusted') + unexpected = [f'{path}:{line} {name}' for (path, name), line + in sorted(found.items(), key=lambda item: (item[0][0], item[1])) + if (path, name) not in ALLOWED] + self.assertEqual(unexpected, [], 'drop the whole-package purge, or purge in setUp ' + 'instead of calling reimport_once_per_class (GitHub issue #1538)') + + def test_every_allowed_class_still_pairs_the_two(self) -> None: + _, found = scan_tree() + self.assertEqual(sorted(key for key in ALLOWED if key not in found), []) + + def test_detector_edges(self) -> None: + """Matches the whole-package forms and nothing narrower or deliberate.""" + helper = ' def setUp(self): reimport_once_per_class(self)\n' + cases = { + # label: (classes flagged, source) + 'tearDown purge': (['T'], 'class T:\n' + helper + + " def tearDown(self): purge_modules(['pcapkit'])\n"), + 'tearDown cleanup, tuple': (['T'], 'class T:\n' + helper + + " def tearDown(self): self.addCleanup(purge_modules, ('pcapkit',))\n"), + 'setUp purge after the helper': (['T'], textwrap.dedent(''' + class T: + def setUp(self): + reimport_once_per_class(self) + purge_modules(ISOLATED_PREFIXES) + ''')), + 'inherited setUp': (['T'], 'class Base:\n' + helper + + "class T(Base):\n def tearDown(self): _support.purge_modules(['pcapkit'])\n"), + 'per-test purge, no helper': ([], textwrap.dedent(''' + class T: + def setUp(self): purge_modules(['pcapkit']) + def tearDown(self): purge_modules(['pcapkit']) + ''')), + 'narrow purge in a test body': ([], 'class T:\n' + helper + " def test(self):\n" + " self.addCleanup(purge_modules, ['pcapkit.toolkit.scapy'])\n"), + 'narrow purge in tearDown': ([], 'class T:\n' + helper + + " def tearDown(self): purge_modules(['pcapkit.utilities.compat'])\n"), + 'purge before the helper': ([], textwrap.dedent(''' + class T: + def setUp(self): + purge_modules(['pcapkit']) + reimport_once_per_class(self, restore=True) + ''')), + } + for label, (flagged, source) in cases.items(): + with self.subTest(case=label): + _, offending = scan(source) + self.assertEqual([name for name, _ in offending], flagged) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/protocols/application/test_layer_placement_unit.py b/tests/protocols/application/test_layer_placement_unit.py index 7423a8f052..372dbb6299 100644 --- a/tests/protocols/application/test_layer_placement_unit.py +++ b/tests/protocols/application/test_layer_placement_unit.py @@ -34,7 +34,7 @@ import tempfile import unittest -from tests._support import purge_modules, reimport_once_per_class +from tests._support import isolate_modules, reimport_once_per_class RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) @@ -77,9 +77,6 @@ class ProtocolLayerPlacementTests(unittest.TestCase): def setUp(self) -> None: reimport_once_per_class(self) - def tearDown(self) -> None: - purge_modules(['pcapkit']) - ########################################################################## # Layer values and base classes. ########################################################################## @@ -177,6 +174,12 @@ def test_the_names_left_the_link_subpackage(self) -> None: def test_dispatch_keys_did_not_move_with_the_modules(self) -> None: """The dispatch tier is decoupled from the subpackage, deliberately.""" + # A dispatch hit writes the resolved class back over its descriptor + # (``ProtocolBase._lookup_next_layer``), and the parsing tests below + # share this class's import. So this test takes a fresh one, and reads + # the registrations as declared whatever order the tests run in. Run + # after any of them on the shared import, it failed (GitHub issue #1538). + isolate_modules(self) from pcapkit.const.reg.ethertype import EtherType from pcapkit.const.reg.transtype import TransType from pcapkit.corekit.module import ModuleDescriptor diff --git a/tests/protocols/application/test_ospf_cryptographic_authentication_unit.py b/tests/protocols/application/test_ospf_cryptographic_authentication_unit.py index 61e9baca26..5a39a41ad9 100644 --- a/tests/protocols/application/test_ospf_cryptographic_authentication_unit.py +++ b/tests/protocols/application/test_ospf_cryptographic_authentication_unit.py @@ -11,7 +11,7 @@ import importlib.util import unittest -from tests._support import purge_modules, reimport_once_per_class +from tests._support import reimport_once_per_class RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) @@ -22,9 +22,6 @@ class OSPFCryptographicAuthenticationUnitTests(unittest.TestCase): def setUp(self) -> None: reimport_once_per_class(self) - def tearDown(self) -> None: - purge_modules(['pcapkit']) - def _packages(self) -> 'list': import pcapkit.protocols.data as data import pcapkit.protocols.schema as schema diff --git a/tests/protocols/application/test_ospf_unit.py b/tests/protocols/application/test_ospf_unit.py index 0d02f058bf..d68660b3e8 100644 --- a/tests/protocols/application/test_ospf_unit.py +++ b/tests/protocols/application/test_ospf_unit.py @@ -12,7 +12,7 @@ import unittest from unittest import mock -from tests._support import purge_modules, reimport_once_per_class +from tests._support import reimport_once_per_class RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) @@ -27,9 +27,6 @@ class OSPFUnitTests(unittest.TestCase): def setUp(self) -> None: reimport_once_per_class(self) - def tearDown(self) -> None: - purge_modules(['pcapkit']) - def test_ospf_index_is_its_transtype_and_make_data_preserves_header(self) -> None: from pcapkit.const.ospf.packet import Packet from pcapkit.const.reg.transtype import TransType diff --git a/tests/protocols/application/test_rarp_unit.py b/tests/protocols/application/test_rarp_unit.py index 2e538d447f..d8213492c5 100644 --- a/tests/protocols/application/test_rarp_unit.py +++ b/tests/protocols/application/test_rarp_unit.py @@ -9,7 +9,7 @@ import importlib.util import unittest -from tests._support import purge_modules, reimport_once_per_class +from tests._support import reimport_once_per_class RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) @@ -21,9 +21,6 @@ class RARPUnitTests(unittest.TestCase): def setUp(self) -> None: reimport_once_per_class(self) - def tearDown(self) -> None: - purge_modules(['pcapkit']) - def test_rarp_ids_and_index_are_stable(self) -> None: from pcapkit.const.reg.ethertype import EtherType from pcapkit.protocols.application.rarp import DRARP, RARP diff --git a/tests/protocols/schema/test_schema_metaclass_abc_cache_unit.py b/tests/protocols/schema/test_schema_metaclass_abc_cache_unit.py index 62fa939e91..37638b3035 100644 --- a/tests/protocols/schema/test_schema_metaclass_abc_cache_unit.py +++ b/tests/protocols/schema/test_schema_metaclass_abc_cache_unit.py @@ -5,7 +5,7 @@ from collections import ChainMap from collections.abc import Mapping -from tests._support import purge_modules, reimport_once_per_class +from tests._support import reimport_once_per_class RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) @@ -51,20 +51,17 @@ class was reading and writing the same ``isinstance`` cache that """ def setUp(self) -> None: - # Fresh :mod:`pcapkit` classes *and* a freshly reset - # :mod:`collections.abc` cache, for every test. Both matter: a stale - # positive cache entry left over from an earlier test in the same - # process would make the very first assertion below true for the wrong - # reason. :func:`reimport_once_per_class` resets the cache each time; - # the classes are fresh each time only because ``tearDown`` purges them - # before :func:`reimport_once_per_class` can keep them for the next test. - reimport_once_per_class(self) - - def tearDown(self) -> None: - # A couple of tests below deliberately poison collections.abc's shared - # caches (that is the point of them) -- reset them again on the way - # out, so nothing outside this module ever reads what was left behind. - purge_modules(['pcapkit']) + # A freshly reset :mod:`collections.abc` cache for every test: a stale + # positive entry left over from an earlier test would make the very + # first assertion below true for the wrong reason. + # :func:`reimport_once_per_class` resets it on every call. The classes + # are the class's own import, kept across its tests (GitHub issue + # #1538): on the #439 defect every schema class shares ``Mapping``'s + # cache, so that reset reaches it too, and reverting the fix fails the + # same tests either way. A couple of tests deliberately poison the + # shared caches, so ``restore=True`` resets them again on the way out + # of the class, and nothing outside it reads what was left behind. + reimport_once_per_class(self, restore=True) def test_a_plain_dict_is_not_a_schema_after_mapping_is_asked_first(self) -> None: """The order-sensitive reproduction from GitHub issue #439 itself. diff --git a/tests/protocols/test_dispatch_default_resolution_unit.py b/tests/protocols/test_dispatch_default_resolution_unit.py index c5938037bf..8acfdbeeeb 100644 --- a/tests/protocols/test_dispatch_default_resolution_unit.py +++ b/tests/protocols/test_dispatch_default_resolution_unit.py @@ -49,7 +49,7 @@ import warnings from unittest import mock -from tests._support import purge_modules, reimport_once_per_class +from tests._support import reimport_once_per_class RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) @@ -61,15 +61,15 @@ @unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') class DefaultDescriptorResolutionTests(unittest.TestCase): def setUp(self) -> None: - reimport_once_per_class(self) - - def tearDown(self) -> None: # ``test_no_stale_class_survives_a_module_reload`` leaves a reloaded # module behind, whose ``Raw`` is a different object from the one the - # rest of the imported tree holds. Purging here keeps that confined to - # the test that did it, rather than handing it to whatever imports - # :mod:`pcapkit` next. - purge_modules(['pcapkit']) + # rest of the imported tree holds. ``restore=True`` keeps that confined + # to this class's own import, rather than handing it to whatever + # imports :mod:`pcapkit` next. Inside the class it is harmless: every + # test reads ``Raw`` through :data:`sys.modules`, as dispatch itself + # does, so both sides of each assertion see the same class whichever + # order the tests run in (GitHub issue #1538). + reimport_once_per_class(self, restore=True) def _dummy_protocol(self) -> type: """Build a protocol class with a registry of its own. diff --git a/tests/utilities/test_logging.py b/tests/utilities/test_logging.py index d9f0eb22fd..bfe1dc3a37 100644 --- a/tests/utilities/test_logging.py +++ b/tests/utilities/test_logging.py @@ -6,7 +6,7 @@ import os import unittest -from tests._support import load_module, purge_modules, reimport_once_per_class +from tests._support import load_module, reimport_once_per_class RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) @@ -44,7 +44,6 @@ def tearDown(self) -> None: os.environ.pop(key, None) else: os.environ[key] = value - purge_modules(['pcapkit']) pristine() def test_boolean_environment_flags_are_parsed(self) -> None: @@ -100,7 +99,6 @@ def tearDown(self) -> None: os.environ.pop('PCAPKIT_DEVMODE', None) else: os.environ['PCAPKIT_DEVMODE'] = self._saved - purge_modules(['pcapkit']) pristine() def test_fresh_import_attaches_only_a_null_handler(self) -> None: @@ -188,7 +186,6 @@ def setUp(self) -> None: pristine() def tearDown(self) -> None: - purge_modules(['pcapkit']) pristine() def test_get_logger_maps_module_names_onto_the_hierarchy(self) -> None: @@ -245,7 +242,6 @@ def setUp(self) -> None: pristine() def tearDown(self) -> None: - purge_modules(['pcapkit']) pristine() def test_configure_sets_level_and_attaches_a_stream_handler(self) -> None: @@ -400,7 +396,6 @@ def setUp(self) -> None: pristine() def tearDown(self) -> None: - purge_modules(['pcapkit']) pristine() def test_registry_bookkeeping_is_logged_at_debug_not_info(self) -> None: @@ -462,7 +457,6 @@ def setUp(self) -> None: pristine() def tearDown(self) -> None: - purge_modules(['pcapkit']) pristine() def test_extraction_emits_a_debug_trail(self) -> None: