From f9ca0e627fcbbecf47d7b0f22679043721223c81 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 6 Oct 2026 09:35:51 -0400 Subject: [PATCH] docs(tests): tighten the shared test-support prose from the post-sweep merges (#719) --- tests/_support.py | 56 +++++++++---------- tests/conftest.py | 21 +++---- tests/const/test_const_apptype_split_unit.py | 16 +++--- tests/const/test_const_enum_builtin_parity.py | 2 +- tests/const/test_const_enum_lookup.py | 6 +- tests/const/test_const_enum_no_mint.py | 3 +- tests/const/test_const_registry_protocol.py | 23 ++++---- tests/integration/_helpers.py | 43 +++++++------- tests/utilities/test_compat.py | 7 ++- 9 files changed, 81 insertions(+), 96 deletions(-) diff --git a/tests/_support.py b/tests/_support.py index caeceeaf32..348473647b 100644 --- a/tests/_support.py +++ b/tests/_support.py @@ -102,13 +102,11 @@ def time_limit(seconds: int = 30) -> Iterator[None]: missing altogether. That one-second floor covers every case where the body ran for longer than the enclosing deadline had left. - The deadline is a wall-clock one, so it measures the runner as much as the - body: a 5-second default, against bodies that take single-digit milliseconds, - still expired on a loaded 4-core CI runner under :program:`pytest-xdist` - (#1058). Hence the 30-second default, the same bound the extraction tests - already used -- several thousand times a working body, and still a fast failure - next to a run that never ends -- and :func:`scale_timeout` on top, for a runner - slower than that. + The deadline is wall-clock, so it measures the runner as much as the body: five + seconds over a body of single-digit milliseconds has expired on a loaded 4-core + CI runner under :program:`pytest-xdist` (#1058). Hence the 30-second default -- + several thousand times a working body, and still a fast failure next to a run + that never ends -- and :func:`scale_timeout` for a runner slower than that. Args: seconds: Whole seconds to allow the body, before @@ -139,9 +137,8 @@ def expire(signum: int, frame: object) -> None: previous_handler = signal.signal(signal.SIGALRM, expire) - # Armed inside the ``try``, so that anything raised while arming -- the - # ``OverflowError`` an oversized value once produced, say -- still puts the - # previous handler back on the way out rather than leaking ``expire``. + # Armed inside the ``try``, so that anything raised while arming still puts + # the previous handler back on the way out rather than leaking ``expire``. armed = False pending = 0 started = time.monotonic() @@ -936,26 +933,25 @@ def purge_modules(prefixes: Iterable[str]) -> None: def reimport_once_per_class(test: 'unittest.TestCase', restore: bool = False) -> None: """Give ``test``'s class one fresh :mod:`pcapkit` import, shared by its tests. - GitHub issue #1065. Calling :func:`purge_modules` from ``setUp`` re-imports - the package before every test -- about 0.67s and 329 modules each, which was - some 85% of the suite's serial time. Call this from ``setUp`` instead. The - first test of the class purges, and the class keeps what that test imports. - Every later test of the class has that same import put back in - :data:`sys.modules`. So a class gets a private import, which its own tests - may mutate, and the import is dropped when the class finishes. Nothing is - imported here: a class that only loads modules by path, or that never - imports :mod:`pcapkit` at all, keeps an empty region, as it did before. + GitHub issue #1065. Call this from ``setUp`` in place of + :func:`purge_modules`, under which every test re-imports the package -- about + 0.67s and 329 modules each, some 85% of the suite's serial time. The first + test of the class purges, and the class keeps what its tests import; every + later test has that import put back in :data:`sys.modules`. So a class gets a + private import, which its own tests may mutate, and lets go of it when the + class finishes. Nothing is imported here: a class that only loads modules by + path, or never imports :mod:`pcapkit` at all, keeps an empty region. A purge in ``setUpClass`` would not do this under :program:`pytest`. :func:`tests.conftest.restore_module_table` swaps the region back to its session-wide pin before every test, so such a class would run on the shared - import that every other class mutates as well. That is exactly the - cross-class leakage a per-test purge used to hide. + import that every other class mutates as well -- the cross-class leakage a + private import rules out. - Under plain :mod:`unittest`, the import is also left in place after the class - finishes, as a per-test purge left one. ``util/run_unittest_leg.py`` relies - on that to catch a module still holding an earlier import (GitHub issue - #981). + Under plain :mod:`unittest`, unless ``restore`` is set, the import is also + left in place after the class finishes, as a per-test purge leaves the last + test's. ``util/run_unittest_leg.py`` relies on that to catch a module still + holding an earlier import (GitHub issue #981). Args: test: The test being set up. @@ -984,11 +980,11 @@ def _keep_lazy_imports(held: 'dict[str, types.ModuleType]') -> None: This is how the class's import is built up. Without it, the conftest restore drops a submodule imported during the test, and the next test of the class - imports it again as a second module object. The parent package still holds the first one as an attribute, and - :func:`unittest.mock.patch` resolves through that attribute before Python - 3.12 -- so a patch landed on a module the code under test no longer used. - Only modules loaded from a file are kept; a stand-in a test bound, and its - own cleanup has not removed, is not. + imports it again as a second module object. The parent package still holds + the first one as an attribute, and :func:`unittest.mock.patch` resolves + through that attribute before Python 3.12 -- so a patch would land on a module + the code under test no longer uses. Only modules loaded from a file are kept; + a stand-in a test bound, and its own cleanup has not removed, is not. """ for name, module in snapshot_modules(ISOLATED_PREFIXES).items(): diff --git a/tests/conftest.py b/tests/conftest.py index 20efd84270..205ac5bfac 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -70,10 +70,9 @@ def _pin_module_snapshot() -> None: """Capture the :mod:`pcapkit` region as the restore target, once. - :func:`restore_module_table` used to restore to a snapshot taken at its own - entry -- "whatever the region held when this test began" -- which is exact - for a leak the fixture's own window can see, but blind to one planted - outside it. :meth:`~unittest.TestCase.setUpClass` runs *before* the first + :func:`restore_module_table` restores to this pinned snapshot, not to one + taken at its own entry. An entry snapshot is exact for a leak the fixture's + own window can see, but blind to one planted outside it. :meth:`~unittest.TestCase.setUpClass` runs *before* the first test's fixture entry and a class-level cleanup runs *after* the last one's fixture exit, so a purge in either place is invisible to an entry-snapshot restore: the entry snapshot for that first test is already the post-purge @@ -140,12 +139,11 @@ def pytest_sessionstart(session: 'pytest.Session') -> 'None': this import, and every later test -- including a purging class's own first one -- is restored to it going in as well as coming out, regardless of whether that class reloads anything for itself. - :class:`tests.integration._helpers.EndToEndTestCase` does reload immediately - after its own purge, for the unrelated reason given in its own docstring -- - saving roughly 45s across its 92 test methods -- and nothing here disturbs - that: rebinding :data:`sys.modules` to the pin does not invalidate a module - object a class attribute already holds a direct reference to, so a class - that reimports for itself keeps exactly what it reimported. + So a class that purges and reimports for itself, as + :class:`tests.integration._helpers.EndToEndTestCase` does, still runs its + tests on the pin. Only a module object a class attribute holds a direct + reference to survives the rebinding, since restoring :data:`sys.modules` + does not invalidate it. 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 @@ -260,8 +258,7 @@ def pytest_timeout_cancel_timer(item: 'pytest.Item') -> 'Iterator[None]': installs fresh tbtrim hooks there, so each retained timer pins the :mod:`pcapkit` generation that was live when its test started. Measured on ``tests/const/test_const_registry_protocol.py`` under ``thread``: past 9 GiB - and killed, against 283 MiB with this hook. That growth is what took every - ``test`` leg's runner down at about 13% on CI. ``optionalhook``: without + and killed, against 283 MiB with this hook. ``optionalhook``: without pytest-timeout installed the hookspec does not exist. """ diff --git a/tests/const/test_const_apptype_split_unit.py b/tests/const/test_const_apptype_split_unit.py index 83c920f192..33b757a39c 100644 --- a/tests/const/test_const_apptype_split_unit.py +++ b/tests/const/test_const_apptype_split_unit.py @@ -32,8 +32,8 @@ __all__ = ['AppTypeSplitTests'] -#: Stride of :func:`_dunder_sample`: about 48 evenly spaced members of each -#: registry, plus the ones it always keeps. +#: Divisor of :func:`_dunder_sample`'s stride, ``len(registry) // 48``: about 50 +#: evenly spaced members of TCP and UDP, and every member of a registry under 96. _DUNDER_SAMPLE_SIZE = 48 @@ -46,7 +46,7 @@ def _dunder_sample(registry: 'type') -> 'list': registry, the port and the service name never change which code runs. So the sample keeps every member that carries an alias (the suffix branch), the first and last row, and an evenly spaced stride through the rest, which - covers all four sets in every registry and is the same list on every run. + covers all four sets across the registries and is the same list on every run. """ members = list(registry) @@ -1208,12 +1208,12 @@ def test_the_new_dunders_are_byte_identical_to_the_percent_form(self) -> None: """GitHub issue #798: ``AppType``'s ``__new__``/``__repr__``/``__str__`` moved from ``%`` formatting to f-strings, and ``__new__``'s format sets every real member's underlying :class:`~aenum.StrEnum` value -- a far larger - blast radius than an error path. This was once checked member by - member; it is now a deterministic sample, because one f-string formats - every member and the sweep only ever reached four distinct code paths - (see :func:`_dunder_sample`). + blast radius than an error path. It is checked on a deterministic + sample rather than member by member, because one f-string formats every + member and a full sweep reaches only four distinct code paths (see + :func:`_dunder_sample`). - The population is still counted over all 12,391 real members (TCP 6147, + The population is counted over all 12,391 real members (TCP 6147, UDP 6143, SCTP 91, DCCP 10, matching the population PR #783 measured), but only :func:`_dunder_sample` is formatted, each member compared against what the pre-#798 ``%``-style formula would have produced for diff --git a/tests/const/test_const_enum_builtin_parity.py b/tests/const/test_const_enum_builtin_parity.py index fe8f0f76d1..927f25e6f6 100644 --- a/tests/const/test_const_enum_builtin_parity.py +++ b/tests/const/test_const_enum_builtin_parity.py @@ -676,7 +676,7 @@ class ConstEnumRegisterFallbackTests(unittest.TestCase): """ def setUp(self) -> None: - # Per test, unlike most classes since GitHub issue #1065: these read the + # Per test, not per class (``reimport_once_per_class``): these read the # registries' lookup tables, which any earlier lookup in the same import # can grow -- ``Flags(0)`` anywhere caches ``0`` as a pseudo-member, and # ``Flags.get(UNRESOLVABLE, 0)`` then resolves instead of raising. diff --git a/tests/const/test_const_enum_lookup.py b/tests/const/test_const_enum_lookup.py index 8ba603e349..ef64f7c789 100644 --- a/tests/const/test_const_enum_lookup.py +++ b/tests/const/test_const_enum_lookup.py @@ -41,9 +41,9 @@ this module carries a companion sweep over the :class:`~aenum.IntFlag` classes, covering the contract the :class:`~aenum.IntEnum` sweep declines to. -Every test class re-imports :mod:`pcapkit` for itself from ``setUp``, -matching the convention every other module in this suite uses (see -:func:`tests._support.reimport_once_per_class`). It matters more here than usual: this +Every test class purges :mod:`pcapkit` before its tests import it -- from +``setUp`` through :func:`tests._support.reimport_once_per_class`, or in +``setUpClass`` -- matching the rest of the suite. It matters more here than usual: this module is the first thing in the whole suite to import *every* submodule under :mod:`pcapkit.const`, including ones nothing else touches (e.g. :mod:`pcapkit.const.reg.apptype`). ``tests/cli/test_main.py`` stubs pieces of diff --git a/tests/const/test_const_enum_no_mint.py b/tests/const/test_const_enum_no_mint.py index 29089a6fcf..2c51d0d116 100644 --- a/tests/const/test_const_enum_no_mint.py +++ b/tests/const/test_const_enum_no_mint.py @@ -2245,8 +2245,7 @@ class BespokeGetUnchangedTests(unittest.TestCase): ``_member_map_``/``_value2member_map_`` lookup does not do -- swapping in the base would silently reintroduce #582. That pin lives in :mod:`tests.const.test_const_method_case_sensitive_896_unit` - (``test_command_get_stays_case_insensitive``), whose body this class used - to repeat verbatim. + (``test_command_get_stays_case_insensitive``). :class:`Method` used to resolve case-insensitively the same way (#583), but GitHub issue #896 retired that: RFC 9110 Section 9.1 makes the HTTP diff --git a/tests/const/test_const_registry_protocol.py b/tests/const/test_const_registry_protocol.py index bda6a66394..fa849364c9 100644 --- a/tests/const/test_const_registry_protocol.py +++ b/tests/const/test_const_registry_protocol.py @@ -260,7 +260,7 @@ def test_tcp_flags_now_inherits_the_base(self) -> None: Resolves ``EnumRegistry`` freshly from the just-reimported ``pcapkit.corekit.enum`` rather than the module-scope import above -- - ``setUp`` re-imported ``pcapkit`` for this class, so ``Flags`` now + ``setUp`` gave this class its own ``pcapkit`` import, so ``Flags`` now inherits a *new* ``EnumRegistry`` class object, and identity against the stale outer one would fail for a reason that says nothing about the product (see ``ProtocolIsInheritedTests.setUpClass`` above, which @@ -1403,14 +1403,12 @@ def test_missing_name_with_default_negative_one_is_now_a_real_default(self) -> N """``-1`` and ``-1.0`` are honoured as defaults by :meth:`~pcapkit.corekit.enum.EnumRegistry.get` itself. - This used to probe :class:`~pcapkit.const.ipv6.extension_header.ExtensionHeader`, - and its docstring conceded that it no longer discriminated #857: no - shipped registry's domain reaches ``-1``, so since #864 the name + A throwaway registry holding ``-1`` is what discriminates #857. No + shipped registry's domain reaches ``-1``, so on one of those the name lookup's :exc:`KeyError` propagates whether ``-1`` is read as a default - or as the old marker. A throwaway registry holding ``-1`` tells the two - apart again: it resolves to that member now, while the pre-#857 - ``default == -1`` check read the caller's ``-1`` (and ``-1.0``, which - compares equal) as *no default* and raised (GitHub issue #1062). + or as the old marker. Here it resolves to that member, while the + pre-#857 ``default == -1`` check read the caller's ``-1`` (and ``-1.0``, + which compares equal) as *no default* and raised. """ from aenum import IntEnum @@ -1433,12 +1431,11 @@ def test_negative_one_defaults_on_a_shipped_registry_raise_and_mint_nothing(self Neither is a registered value of any shipped registry, whose domains all start at ``0``, so ``default`` does not resolve: there is no ``cls(default)`` fallback left to attempt, and the name lookup's own - :exc:`KeyError` propagates, as with no default at all. Pinned from the - pre-#864 tree, which raised :exc:`ValueError` from ``cls(-1)``. This - does *not* discriminate #857, which raised the same :exc:`KeyError`; + :exc:`KeyError` propagates, as with no default at all; the pre-#864 + tree raised :exc:`ValueError` from ``cls(-1)`` instead. This does *not* + discriminate #857, which raised the same :exc:`KeyError`; :meth:`test_missing_name_with_default_negative_one_is_now_a_real_default` - does. Merged from separate ``-1`` and ``-1.0`` tests (GitHub issue - #1062). + does. """ from pcapkit.const.ipv6.extension_header import ExtensionHeader diff --git a/tests/integration/_helpers.py b/tests/integration/_helpers.py index 218bbcf6cb..bda0f80525 100644 --- a/tests/integration/_helpers.py +++ b/tests/integration/_helpers.py @@ -64,30 +64,25 @@ class EndToEndTestCase(unittest.TestCase): def setUpClass(cls) -> None: """Drop the imported library so the class starts from a clean state. - The surrounding tiers used to purge in :meth:`setUp`, i.e. once per - test, and since GitHub issue #1065 mostly purge once per class too. A - 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 + Under plain :mod:`unittest` this gives each class one fresh + :mod:`pcapkit` import, which its tests then share. A fresh import + measures at roughly 0.7s, which per test would cost this tier more than + the extractions themselves. 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`. + + Under :program:`pytest` neither the purge nor the re-import reaches the + tests. :func:`tests.conftest.restore_module_table` restores the region + before every test to the warm snapshot + :func:`tests.conftest._pin_module_snapshot` took at session start, so + each test runs on that session-wide import instead. Measured with a + throwaway subclass whose tests report the ``id`` of + ``sys.modules['pcapkit']``: under :program:`pytest` every test saw the + object from before this method ran, under :mod:`unittest` every test saw + the one imported here. + + The re-import is guarded because nothing depends on it, 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 diff --git a/tests/utilities/test_compat.py b/tests/utilities/test_compat.py index d434085fff..7ef9b0f486 100644 --- a/tests/utilities/test_compat.py +++ b/tests/utilities/test_compat.py @@ -135,8 +135,8 @@ #: :mod:`pcapkit.protocols.misc.pcap.frame`, :mod:`pcapkit.protocols.misc.pcapng`, #: :mod:`pcapkit.protocols.link.ethernet` and :mod:`pcapkit.protocols.link.arp`. #: None of them is reachable from the branches faked here, and if one became -#: reachable it would still be harmless: the region is purged and restored around -#: every test, so the next import recomputes the value from source. A third-party +#: reachable it would still be harmless: the region is restored around every +#: test, so the next import recomputes the value from source. A third-party #: module is never purged, which is exactly why its cache is permanent and why the #: assertion is aimed there. OWN_MODULE_PREFIX = 'pcapkit' @@ -172,7 +172,8 @@ class CompatTests(unittest.TestCase): def setUp(self) -> None: reimport_once_per_class(self) - # Registered before anything else, and deliberately first: cleanups run + # Registered deliberately early -- only ``reimport_once_per_class``'s own + # cleanup, which has to run last, comes before it. Cleanups run # last-in-first-out, so this one runs *after* the module-table restore # that ``load_module`` arranges below and after the test body however it # ended -- including when it ended by raising. It is a sweep of a handful