From 39552a51e5b4512cbf49d67e1f2fdd0f620e2df2 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 22 Sep 2026 23:10:53 -0400 Subject: [PATCH] fix(tests): keep a faked version and hand-bound stand-ins inside the test that made them (#687, #688) Two test-isolation defects found while auditing #674, neither of which a normal `pytest` run can see: `tests/conftest.py` repairs one of them after every test and warms the cache the other one needs cold. * `tests/utilities/test_compat.py` faked `sys.version_info` around a real `import`, so the `< 3.11` branch of `pcapkit/utilities/compat.py` performed the first `import aenum` of the process under the lie. `aenum/_common.py` memoises `pyver = sys.version_info[:2]` at import time, and every other aenum module takes a *copy* through `from ._common import *`, so `aenum/_enum.py:1640` then called `__set_name__` on every member of every later enumeration. Restoring `sys.version_info` undoes none of that -- the value is already memoised. It now imports what the faked branches need *before* installing the fake, and asserts that nothing at all was first-imported under it, so the ordering is checked rather than relied on. Reloading `aenum._common` was rejected as a fix because it repairs `_common.pyver` and leaves `_enum.pyver`, the copy that decides the branch; a subprocess was rejected because coverage keys on file path, so the in-process load is what credits compat.py's `< 3.6` lines today and this repo wires up no subprocess coverage. * `tests/cli/test_main.py` wrote nine names into `sys.modules` -- seven stand-ins, a bare parent package and `pcapkit.__main__` -- and restored none, so the next test to import the real library got a `pcapkit.utilities.compat` carrying one name. It now goes through `tests._support.isolate_modules`, widened to cover `'emoji'`: the faked `emoji` was outside `ISOLATED_PREFIXES`, so it was not even masked and survived a normal pytest run. * Both files now assert the invariant they used to break, in a cleanup registered ahead of the restore so it runs after it: aenum's `pyver` readings across every aenum module, and the `sys.modules` region unchanged in all three directions -- added, removed and rebound -- so absence is checked as well as presence. * `tests/project/test_module_isolation.py` gains `UnmaskedOrderTests`, which runs both pairings under the stdlib `unittest` runner. For #688 that is the cross-file half of a claim the file itself already makes; for #687 it is the only place a regression is caught at all, since `pytest_sessionstart`'s eager `import pcapkit` warms aenum and makes the file's own guards pass either way. * Three docstrings corrected, all of which had become false. `tests/conftest.py` claimed all three known #660 leaks were fixed at their call sites; `tests/project/test_module_isolation.py` recorded `tests/cli/test_main.py` as "deliberately left as it was", the standing witness for the conftest guard -- which is precisely why the leak survived unnoticed; and `tests/const/test_const_enum_lookup.py` named that file as a live hazard. Measured on CPython 3.14.7. `python -m unittest tests.utilities.test_compat tests.project.test_public_api` goes from `FAILED (errors=10)` to `OK`, and `tests.cli.test_main tests.project.test_public_api` likewise; both pairings also go from `10 failed` to all passing under `pytest --noconftest`. With the warm-up list emptied the compat file reports `FAILED (failures=5)` under `unittest`, and with the isolation replaced by the old purge loop the CLI file reports `6 failed` under plain `pytest`. No `pcapkit/` line changed, and `coverage report` over the touched selection is byte-identical before and after (`diff` exit 0); the same selection goes from 37 to 40 tests with 566 subtests unchanged. Fixes #687 Fixes #688 --- tests/cli/test_main.py | 132 +++++++++++- tests/conftest.py | 15 +- tests/const/test_const_enum_lookup.py | 8 +- tests/project/test_module_isolation.py | 163 +++++++++++++-- tests/utilities/test_compat.py | 268 ++++++++++++++++++++++++- 5 files changed, 560 insertions(+), 26 deletions(-) diff --git a/tests/cli/test_main.py b/tests/cli/test_main.py index 8d673a17fb..11a0d585d8 100644 --- a/tests/cli/test_main.py +++ b/tests/cli/test_main.py @@ -1,3 +1,40 @@ +# -*- coding: utf-8 -*- +""":mod:`pcapkit.__main__`, against stand-ins for everything it imports. + +The command line tool is tested without the library behind it: :mod:`pcapkit`, +:mod:`pcapkit.foundation.extraction`, :mod:`pcapkit.interface`, three +:mod:`pcapkit.utilities` modules and :mod:`emoji` are all replaced with +stand-ins, so what is under test is the argument wiring and nothing else. The +real thing runs in :mod:`tests.integration.test_cli_subprocess`. + +:data:`sys.modules` is process-global, so binding a stand-in over a real module +name is a write that outlives the test unless something undoes it -- and these +stand-ins are *emptier* than what they replace, which is the quiet kind. Issue +#688: this file wrote nine names and put none of them back -- the seven stand-ins +above, the bare ``pcapkit.foundation`` parent they hang from, and +``pcapkit.__main__`` itself. Pairing it with :mod:`tests.project.test_public_api` +gave ten errors: nine ``ImportError: cannot import name 'show_flag_values' from +'pcapkit.utilities.compat'`` and one for ``SeekError``, because the real library's +next import found a ``pcapkit.utilities.compat`` carrying exactly one name. + +That is the same defect as issues #660 and #674, and it is fixed the same way: +:func:`tests._support.isolate_modules` in ``setUp``, which purges the region on +the way in and restores it exactly on the way out. What this file used to do +instead was roll its own purge loop, which protects this file from whatever ran +before it and promises nothing to whatever runs after -- see +:func:`tests._support.purge_modules` for why restoration is owed by the code that +binds. + +:func:`tests.conftest.restore_module_table` masked the symptom under +:program:`pytest` throughout, which is why it took an audit to find: it surfaces +only under ``--noconftest``, under the stdlib :mod:`unittest` runner, or on any +route that does not load that conftest. +:meth:`CLIMainTests.assert_module_table_restored` is checked in a cleanup +registered *before* the isolation, so it runs after the restore and is not +masked by that fixture -- a regression here fails under a plain +:program:`pytest` run rather than waiting for someone to try another runner. + +""" from __future__ import annotations import importlib.util @@ -8,15 +45,99 @@ import unittest from unittest import mock +from tests._support import ISOLATED_PREFIXES, isolate_modules, snapshot_modules + ROOT = pathlib.Path(__file__).resolve().parents[2] +#: The :data:`sys.modules` region this file stands things in for, and therefore +#: the region it has to put back. +#: +#: Wider than :data:`tests._support.ISOLATED_PREFIXES` by ``'emoji'``, which is +#: the one stand-in here that is not part of :mod:`pcapkit`. +#: :func:`tests.conftest.restore_module_table` covers only the default prefixes, +#: so the faked ``emoji`` was not merely unrestored but *unmasked* as well: it +#: survived even a normal :program:`pytest` run, leaving whatever imported +#: :mod:`emoji` next with a :class:`~types.SimpleNamespace` carrying nothing but +#: ``emojize`` -- or, from the last test here, a class whose ``emojize`` raises +#: :exc:`UnicodeEncodeError`. +ISOLATED = ISOLATED_PREFIXES + ('emoji',) + +#: How many module names :func:`summarise_names` spells out before it starts +#: counting instead. Bounded because the interesting list is long: on a warm +#: module table the :mod:`pcapkit` region holds some three hundred names, so an +#: unbounded report of what a leak did to it is twelve kilobytes of failure +#: message that nobody reads -- measured, at 12089 characters. +NAMES_IN_FAILURE_MESSAGE = 8 + + +def summarise_names(label: str, names: 'list[str]') -> str: + """``label``, how many of ``names`` there are, and the first few of them. + + Args: + label: What went wrong with these names -- ``'added'`` and its siblings. + names: The names, already sorted. + + Returns: + A one-line summary, or the empty string when ``names`` is empty, so that + the caller can drop the directions that are fine. + + """ + if not names: + return '' + shown = ', '.join(names[:NAMES_IN_FAILURE_MESSAGE]) + extra = len(names) - NAMES_IN_FAILURE_MESSAGE + return f'{label} {len(names)} ({shown}{f", +{extra} more" if extra > 0 else ""})' + class CLIMainTests(unittest.TestCase): - def _load_cli_module(self, *, emoji_module=Ellipsis): - for name in list(sys.modules): - if name == 'pcapkit' or name.startswith('pcapkit.') or name == 'emoji': - sys.modules.pop(name, None) + def setUp(self) -> None: + # Registered *before* ``isolate_modules`` and deliberately so: cleanups + # run last-in-first-out, so this one runs after the restore that + # ``isolate_modules`` registers and can check that the restore actually + # happened. Registered the other way round it would run first, see the + # stand-ins still bound, and fail every test. + self.addCleanup(self.assert_module_table_restored, snapshot_modules(ISOLATED)) + + # ``isolate_modules`` rather than a purge loop of this file's own: the + # stand-ins below are bound over real module names, and purging protects + # only this test while restoring is what the *next* one needs (#688). + isolate_modules(self, ISOLATED) + + def assert_module_table_restored(self, before: 'dict[str, types.ModuleType]') -> None: + """The :data:`ISOLATED` region of :data:`sys.modules` is as it was found. + + The regression assertion for issue #688, and exact in all three + directions a restore can be wrong: a name this test added, a name it + dropped, and a name it rebound to something else. Absence matters as much + as presence -- ``pcapkit.utilities.compat`` did not exist before a test + here ran on a cold table, so it must not exist after one either, and a + check written as a :meth:`dict.update` of the snapshot would have missed + exactly that. + + Args: + before: The snapshot taken in ``setUp``, before anything was purged + or bound. + """ + after = snapshot_modules(ISOLATED) + problems = [summary for summary in ( + summarise_names('added', sorted(set(after) - set(before))), + summarise_names('removed', sorted(set(before) - set(after))), + summarise_names('rebound', sorted(name for name in set(before) & set(after) + if after[name] is not before[name])), + ) if summary] + + self.assertEqual( + problems, [], + f'this test left the {ISOLATED} region of sys.modules different from how it ' + f'found it -- {"; ".join(problems)}. The stand-ins bound here are emptier ' + f'than the modules they replace, so whatever imports the real library next ' + f'gets a package with almost no attributes rather than an error naming this ' + f'file. See issue #688.') + + def _load_cli_module(self, *, emoji_module=Ellipsis): + # No purge here: ``setUp``'s ``isolate_modules`` has already emptied the + # region, and it is the half that also puts it back afterwards. pcapkit_pkg = types.ModuleType('pcapkit') pcapkit_pkg.__version__ = '9.9.9' pcapkit_pkg.__path__ = [str(ROOT / 'pcapkit')] @@ -64,6 +185,9 @@ def __iter__(self): if emoji_module is not Ellipsis: if emoji_module is None: + # A no-op since the isolation covers ``'emoji'`` -- the name is + # already gone. Kept as the explicit spelling of "leave nothing + # bound", so the caller reads the same either way. sys.modules.pop('emoji', None) else: sys.modules['emoji'] = emoji_module diff --git a/tests/conftest.py b/tests/conftest.py index 417e0b531b..6c23053a73 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -110,10 +110,11 @@ def restore_module_table() -> 'Iterator[None]': ``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. + which -- :mod:`tests.cli.test_main` -- did 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 + Hence a guard here rather than a ``tearDown`` in each of them. Every known + leak is also fixed at its call site, 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 @@ -121,6 +122,14 @@ def restore_module_table() -> 'Iterator[None]': that will be written, which is the difference between the failure being unlikely and being impossible. + Two of the three were fixed at their call sites when this guard landed, and + the third was not: :mod:`tests.cli.test_main` kept its own purge loop, which + purges and restores nothing, and this fixture went on quietly healing it for + every test. That is issue #688 -- it took an audit rather than a failing run + to find, because a guard that repairs a leak also hides it. It is now fixed + at its call site too, which is what makes the paragraph above true of all + three rather than of two. + 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 diff --git a/tests/const/test_const_enum_lookup.py b/tests/const/test_const_enum_lookup.py index 04a95cabd1..131524fc34 100644 --- a/tests/const/test_const_enum_lookup.py +++ b/tests/const/test_const_enum_lookup.py @@ -49,8 +49,12 @@ :mod:`pcapkit.const.reg.apptype`). ``tests/cli/test_main.py`` stubs pieces of :mod:`pcapkit.utilities.compat` and :mod:`pcapkit.utilities.exceptions` straight into :data:`sys.modules` for its own isolation and, depending on -suite order, that stub can still be sitting there when this module runs -- -purging first forces a clean re-import instead of tripping over it. +suite order, those stubs used to still be sitting there when this module ran -- +purging first forces a clean re-import instead of tripping over them. That file +has since been put under :func:`tests._support.isolate_modules` and puts them +back itself (issue #688), so it is no longer the specific hazard; the purge stays +because the convention is worth keeping and because the *next* stand-in written +without the helpers would be. """ from __future__ import annotations diff --git a/tests/project/test_module_isolation.py b/tests/project/test_module_isolation.py index 9e5c696e3d..1d5a5fa7d1 100644 --- a/tests/project/test_module_isolation.py +++ b/tests/project/test_module_isolation.py @@ -29,13 +29,50 @@ 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. +* :meth:`OrderIndependenceTests.test_the_cli_polluter_order_passes` is the third + leak, in ``tests/cli/test_main.py``. It used to pin the *structural* half of the + fix: that file rolled its own purge loop and did not import + :mod:`tests._support`, so nothing in that module could have fixed it and only + :func:`tests.conftest.restore_module_table` did, and it was deliberately left + that way as the standing witness that the guard covers a file which has not + opted into anything. That turned out to be the wrong trade. The guard healed the + leak on every run, which is exactly why it took an audit rather than a red test + to notice the leak was still there -- issue #688. The file now calls + :func:`tests._support.isolate_modules` like its neighbours, so this case is a + control on the pairing rather than a witness for the guard, and the witness role + is left vacant on purpose: a test whose job is to leave :data:`sys.modules` + broken is a defect the suite would have to maintain deliberately, and the next + file written without the helpers is covered by the guard whether or not such a + test exists. + +The guard is also why the :program:`pytest` runs above cannot see a leak at all. +What they assert is that the *guarded* suite is order-independent -- which it was +throughout, while #660, #674 and #688 were each live. So :data:`UNMASKED_ORDERS` +runs the same shape of pairing under the stdlib :mod:`unittest` runner, where no +conftest is loaded and nothing heals anything between tests. Both defects the #674 +audit turned up are covered there: + +* ``tests/utilities/test_compat.py`` faking :data:`sys.version_info` around a real + ``import``, which memoises the fake in :mod:`aenum`'s import-time version cache + for the rest of the process -- issue #687. This one is not a + :data:`sys.modules` leak at all and :func:`tests.conftest.restore_module_table` + could not have healed it; what hid it was + :func:`tests.conftest.pytest_sessionstart` importing :mod:`pcapkit`, and so + :mod:`aenum`, before any test ran. **This is the only place a #687 regression is + caught**, and that is why it is here rather than left to the file's own + assertions: the same warm import makes those assertions vacuous under + :program:`pytest`, so they pass whether or not the fix is in place. +* ``tests/cli/test_main.py`` leaving its stand-ins bound -- issue #688. That one + *is* caught in the file itself, by + :meth:`tests.cli.test_main.CLIMainTests.assert_module_table_restored`, which is + registered as a cleanup ahead of the isolation and so runs before the conftest + fixture gets to heal anything. This case is the cross-file half of the same + claim, and cheap enough to keep for the symmetry. + +``pytest --noconftest`` over the same files shows both as well and is the quicker +thing to reach for by hand. :mod:`unittest` is what is automated here because it +needs no flag to get there, and because a second runner is worth exercising for +its own sake. 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 @@ -77,7 +114,7 @@ ], ), ( - 'a polluter that does not use tests._support', + 'the CLI stand-in leak', [ 'tests/cli/test_main.py::CLIMainTests::test_get_parser_parses_expected_arguments', 'tests/project/test_public_api.py', @@ -85,15 +122,35 @@ ), ] +#: Pairings that :func:`tests.conftest.restore_module_table` and +#: :func:`tests.conftest.pytest_sessionstart` between them hide, so they are run +#: under the stdlib :mod:`unittest` runner instead -- which loads no conftest and +#: heals nothing. Each is (label, dotted module names, the symptom to look for). +#: +#: The symptom is asserted as well as the exit status because the two say different +#: things. A non-zero exit says *something* failed; the symptom says the failure +#: was this one, and not some unrelated breakage in a file that happens to be in +#: the selection. +UNMASKED_ORDERS = [ + ( + 'the aenum import-time version cache (#687)', + ['tests.utilities.test_compat', 'tests.project.test_public_api'], + "object has no attribute '__set_name__'", + ), + ( + 'the CLI stand-ins left in sys.modules (#688)', + ['tests.cli.test_main', 'tests.project.test_public_api'], + "cannot import name 'show_flag_values' from 'pcapkit.utilities.compat'", + ), +] -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. +def child_environ() -> 'dict[str, str]': + """The environment a runner subprocess should inherit. Returns: - The finished process, with output captured. + A copy of this process's environment, with the repository root on + :envvar:`PYTHONPATH` and the :program:`pytest` session variables removed. """ environ = dict(os.environ) @@ -107,10 +164,51 @@ def run_pytest(selection: 'list[str]') -> 'subprocess.CompletedProcess[str]': # cache directory as the run that started it. environ.pop('PYTEST_ADDOPTS', None) environ.pop('PYTEST_CURRENT_TEST', None) + return environ + + +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. + """ return subprocess.run( [sys.executable, '-m', 'pytest', '-p', 'no:cacheprovider', '-q', *selection], - cwd=str(ROOT), env=environ, capture_output=True, text=True, + cwd=str(ROOT), env=child_environ(), capture_output=True, text=True, + timeout=600, check=False, + ) + + +def run_unittest(modules: 'list[str]') -> 'subprocess.CompletedProcess[str]': + """Run the stdlib :mod:`unittest` runner over ``modules`` in a subprocess. + + The point of using this runner rather than :program:`pytest` is everything it + does *not* do: it loads no ``conftest.py``, so neither + :func:`tests.conftest.restore_module_table` nor + :func:`tests.conftest.pytest_sessionstart` runs, and a test that leaves the + process in a worse state than it found it gets no help. ``pytest + --noconftest`` reaches the same place; this needs no flag to get there. + + Modules run in the order given, which is the whole point -- the polluting one + comes first. + + Args: + modules: Dotted module names, e.g. ``'tests.cli.test_main'``. Not paths: + the :mod:`unittest` runner takes names. + + Returns: + The finished process, with output captured. :mod:`unittest` writes its + report to stderr rather than stdout. + + """ + return subprocess.run( + [sys.executable, '-m', 'unittest', *modules], + cwd=str(ROOT), env=child_environ(), capture_output=True, text=True, timeout=600, check=False, ) @@ -139,7 +237,7 @@ 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: + def test_the_cli_polluter_order_passes(self) -> None: label, selection = POLLUTING_ORDERS[2] self.assert_selection_passes(label, selection) @@ -155,5 +253,40 @@ def test_the_reverse_order_passes_too(self) -> None: self.assert_selection_passes(f'{label}, reversed', list(reversed(selection))) +class UnmaskedOrderTests(unittest.TestCase): + """The pairings the conftest hides pass without it. + + Separate from :class:`OrderIndependenceTests` because the claim is a different + one. That class asserts the *guarded* suite is order-independent, which it was + even while #660, #674, #687 and #688 were live. This one asserts the files + themselves are, with nothing healing them -- which is what makes the per-file + fixes real rather than merely masked. + + """ + + def assert_unittest_selection_passes(self, label: str, modules: 'list[str]', + symptom: str) -> None: + """Run ``modules`` under :mod:`unittest` and fail with its output if not clean.""" + finished = run_unittest(modules) + output = finished.stdout + finished.stderr + + self.assertEqual( + finished.returncode, 0, + f'the unittest runner exited {finished.returncode} on {label} -- the files ' + f'are not isolated from each other, they were only being healed by ' + f'tests/conftest.py.\n\n' + f'selection: {" ".join(modules)}\n\n' + f'output:\n{output[-6000:]}') + self.assertNotIn(symptom, output, f'{label} still reaches a later test') + + def test_the_version_fake_does_not_poison_a_later_import(self) -> None: + label, modules, symptom = UNMASKED_ORDERS[0] + self.assert_unittest_selection_passes(label, modules, symptom) + + def test_the_cli_stand_ins_do_not_reach_a_later_test(self) -> None: + label, modules, symptom = UNMASKED_ORDERS[1] + self.assert_unittest_selection_passes(label, modules, symptom) + + if __name__ == '__main__': unittest.main() diff --git a/tests/utilities/test_compat.py b/tests/utilities/test_compat.py index c0e3c6d4e0..10ad5b8f54 100644 --- a/tests/utilities/test_compat.py +++ b/tests/utilities/test_compat.py @@ -1,19 +1,237 @@ +# -*- coding: utf-8 -*- +"""The compatibility shims in :mod:`pcapkit.utilities.compat`. + +Every name that module exports has two implementations: the one the running +interpreter provides, and a fallback for the oldest interpreter the package +supports. Only one of the two can run on any given interpreter, so the only way +to test the other is to lie to the module about which interpreter it is on -- +:meth:`CompatTests.load_compat_as_python35` executes the file a second time with +:data:`sys.version_info` faked to ``(3, 5)``. + +That lie is not free, and containing its cost is most of what this module is +about. A fake version is process-global for as long as it is installed, so *any* +module first imported while it is in place memoises the false value -- and +restoring :data:`sys.version_info` afterwards does not undo a memoisation that +has already happened. Issue #687 is the bill: ``aenum/_common.py`` caches +``pyver = sys.version_info[:2]`` at import time, the ``< 3.11`` branch of +:file:`pcapkit/utilities/compat.py` does ``from aenum import StrEnum``, and on +Python 3.11 and later nothing this file imports has pulled :mod:`aenum` in already +-- the branch the *real* interpreter takes is ``from enum import StrEnum`` -- so the +faked load performs the first ``import aenum`` of the process. Every +:mod:`aenum` enumeration built afterwards -- including +``pcapkit/const/reg/apptype.py:26``'s ``TransportProtocol`` -- then took the +``pyver < PY3_6`` path in ``aenum/_enum.py`` and died with ``AttributeError: +'TransportProtocol' object has no attribute '__set_name__'``, so a later +``import pcapkit`` failed outright. + +Hence the two guards here, neither of which is about the shims themselves: + +#. :data:`WARM_BEFORE_FAKING` is imported *before* the fake goes in, so the + memoised value is the true one, and + :meth:`CompatTests.load_compat_as_python35` **asserts** that nothing at all + was first-imported under the fake -- the ordering is checked rather than + assumed. +#. :meth:`CompatTests.assert_aenum_is_not_poisoned` runs as a cleanup on every + test in this file, so the invariant is re-checked after each one rather than + only where it is broken. + +One honest limitation, because it decides where the regression is actually caught. +Both guards are **vacuous under a normal** :program:`pytest` **run**, and not by +accident: :func:`tests.conftest.pytest_sessionstart` imports :mod:`pcapkit` before +the first test, which imports :mod:`aenum` under the true version, so there is no +cold cache left for the fake to poison. Measured -- with +:data:`WARM_BEFORE_FAKING` emptied, ``pytest -q tests/utilities/test_compat.py`` +still reports ``5 passed``, while ``python -m unittest tests.utilities.test_compat`` +reports ``FAILED (failures=5)``. So the route that regresses is the stdlib +:mod:`unittest` runner, ``pytest --noconftest``, or a bare script, and +:class:`tests.project.test_module_isolation.UnmaskedOrderTests` is what runs one of +those in CI on this file's behalf. + +""" from __future__ import annotations import decimal import enum +import importlib import sys import unittest from unittest import mock from tests._support import load_module, purge_modules +# NOTE: :mod:`aenum` is deliberately *not* imported at module scope, however +# convenient that would be for the assertions below. A module-level import would +# warm the cache issue #687 is about as a side effect of this file being +# collected, which is the fix -- so it would also make the fix impossible to +# switch off and the regression test impossible to fail. The warming is done by +# :data:`WARM_BEFORE_FAKING` alone, where it is visible and named, and the +# assertions reach :mod:`aenum` through :data:`sys.modules` instead. + +#: Modules to import before :data:`sys.version_info` is faked, so that whatever +#: they memoise about the interpreter at import time is the *real* answer. +#: +#: :mod:`aenum` is the one that bit, and it bit hard enough to be worth spelling +#: out. ``aenum/_common.py`` caches ``pyver = sys.version_info[:2]`` at import +#: time; every other module of the package then takes a **copy** of that value +#: through ``from ._common import *``. With the copy reading ``(3, 5)``, +#: ``aenum/_enum.py`` believes the interpreter predates +#: :meth:`~object.__set_name__` and calls it on every member of every enumeration +#: built from then on. See issue #687, and note that this is also why *reloading* +#: ``aenum._common`` is not a fix: it repairs ``aenum._common.pyver`` and leaves +#: ``aenum._enum.pyver``, the copy that decides the branch, still reading +#: ``(3, 5)``. Measured on CPython 3.14.7 with ``aenum`` 3.1.17, and structural +#: rather than version-specific: the copy is made by ``from ._common import *`` at +#: the top of every one of :mod:`aenum`'s own modules. +#: +#: The rest are here because the faked branches import them too. Whether each one +#: memoises anything version-dependent is deliberately *not* the question: the +#: invariant :meth:`CompatTests.load_compat_as_python35` asserts is that nothing +#: whatever is first-imported under the fake, which needs no judgement about +#: which third-party caches are dangerous. A future interpreter or :mod:`aenum` +#: release that pulls in one more module fails that assertion here, at the line +#: that caused it, instead of poisoning something three directories away. +WARM_BEFORE_FAKING = ('aenum', 'decimal', 'threading', 'typing', 'typing_extensions') + +#: Prefix of the names the loaders themselves write, which are excluded from the +#: "nothing was imported under the fake" check. :func:`tests._support.load_module` +#: binds the module it executes and a stub package per parent of its name, and +#: those writes are the loader's own -- :func:`tests._support.restore_modules_after` +#: already owes them (issue #674). +#: +#: Excluding them is safe for a reason that does not extend to the third-party +#: modules this file *does* check, and the distinction is the point. Four +#: :mod:`pcapkit` modules do memoise the interpreter version at import time in the +#: same shape :mod:`aenum` does -- ``py37``/``py38`` in +#: :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 +#: module is never purged, which is exactly why its cache is permanent and why the +#: assertion is aimed there. +OWN_MODULE_PREFIX = 'pcapkit' + + +def aenum_pyver_readings() -> 'dict[str, tuple[int, ...]]': + """``pyver`` as every imported :mod:`aenum` module currently sees it. + + Swept out of :data:`sys.modules` rather than named module by module, because + ``pyver`` is not one value: ``aenum/_common.py`` computes it, and every other + module of the package takes a **copy** through ``from ._common import *``. + The copies can disagree with the original -- reloading ``aenum._common`` + repairs it and leaves all of them -- and it is ``aenum._enum``'s copy that + ``aenum/_enum.py:1640`` branches on when it decides whether to call + :meth:`~object.__set_name__` by hand. Sweeping covers ``_py3``, ``_tuple`` + and ``_constant`` too, without this file having to keep a list of which of + :mod:`aenum`'s private modules hold one. + + Returns: + Each :mod:`aenum` module that has a tuple ``pyver``, mapped to its value. + Empty when :mod:`aenum` has not been imported at all, which is a real + state under the stdlib :mod:`unittest` runner: nothing has cached + anything yet, so there is nothing to be wrong. + + """ + return {name: module.pyver + for name, module in sorted(sys.modules.items()) + if (name == 'aenum' or name.startswith('aenum.')) + and isinstance(getattr(module, 'pyver', None), tuple)} + class CompatTests(unittest.TestCase): def setUp(self) -> None: + # Registered before anything else, and deliberately first: 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 + # of module attributes, so paying it on the tests that fake nothing costs + # nothing and covers the case where a future test in this file learns to + # fake a version without going through ``load_compat_as_python35``. + self.addCleanup(self.assert_aenum_is_not_poisoned) + purge_modules(['pcapkit']) self.compat = load_module('pcapkit.utilities.compat', 'pcapkit/utilities/compat.py') + def assert_aenum_is_not_poisoned(self) -> None: + """:mod:`aenum` still believes it is on the interpreter it is on. + + The invariant issue #687 broke, checked across every reading + :func:`aenum_pyver_readings` can find rather than in one place, for the + reason that function gives: the value is copied into each of + :mod:`aenum`'s modules at import time and they can disagree. + + Checked here, immediately, rather than left to show up as the symptom, + because the symptom is unrecoverable and lands somewhere else. Nothing + this file can do afterwards puts a memoised value back, so the useful + moment to notice is the test that caused it -- not the ten tests of + :mod:`tests.project.test_public_api` that failed on it three directories + away. + + """ + real = sys.version_info[:2] + wrong = {name: value for name, value in aenum_pyver_readings().items() + if value != real} + self.assertEqual( + wrong, {}, + f'{sorted(wrong)} read pyver as {sorted(set(wrong.values()))} rather than ' + f'{real!r} -- aenum was first imported while this file had sys.version_info ' + f'faked, so its import-time version cache is now permanently wrong and every ' + f'aenum enumeration built from here on takes the pre-3.6 __set_name__ path ' + f'and raises AttributeError. Nothing undoes this: restoring sys.version_info ' + f'does not un-memoise a value. See issue #687 and WARM_BEFORE_FAKING.') + + def load_compat_as_python35(self, module_name: str) -> 'object': + """Execute :file:`pcapkit/utilities/compat.py` as if on Python 3.5. + + Two things have to hold for that to be safe, and the second is the whole + of issue #687. + + The interpreter has to *look* like 3.5 while the module body runs, which + is what the :func:`~unittest.mock.patch.object` here does. And nothing may + be imported for the first time while it looks that way, because an + import-time cache of the faked value is not something restoring + :data:`sys.version_info` can undo -- the value is already memoised inside + a module this suite does not own. + + So the imports the faked branches need are done first, under the true + version, and then the window is *measured*: any name that appears in + :data:`sys.modules` while the fake is installed is a name whose + import-time view of the interpreter is now wrong, and the assertion below + says so. That is what turns "import :mod:`aenum` early and hope the + ordering holds" into an ordering that is checked. Names under + :data:`OWN_MODULE_PREFIX` are excluded: those are the loader's own writes, + already owed back by :func:`tests._support.restore_modules_after`. + + Args: + module_name: Dotted name to execute the file under. Must not be the + name the real module holds, or the 3.5-flavoured module would be + bound over it. + + Returns: + The executed module, with every ``< 3.6`` fallback in place of the + implementation this interpreter would otherwise have supplied. + + """ + for name in WARM_BEFORE_FAKING: + importlib.import_module(name) + + before = frozenset(sys.modules) + with mock.patch.object(sys, 'version_info', (3, 5)): + compat = load_module(module_name, 'pcapkit/utilities/compat.py') + + leaked = sorted( + name for name in set(sys.modules) - before + if not (name == OWN_MODULE_PREFIX or name.startswith(OWN_MODULE_PREFIX + '.')) + ) + self.assertEqual( + leaked, [], + f'{leaked} were imported for the first time while sys.version_info was ' + f'faked to (3, 5), so whatever any of them memoised about the interpreter ' + f'at import time is now wrong for the rest of the process -- and restoring ' + f'sys.version_info does not undo it. Add them to WARM_BEFORE_FAKING so ' + f'they are imported under the real version first. See issue #687.') + return compat + def test_cached_property_only_computes_once(self) -> None: class Demo: def __init__(self) -> None: @@ -44,10 +262,56 @@ class DemoFlag(enum.IntFlag): self.assertEqual(self.compat.show_flag_values(DemoFlag.READ | DemoFlag.EXEC), [1, 4]) + def test_faking_the_version_leaves_aenum_reading_the_real_one(self) -> None: + """The regression test for issue #687. + + Named rather than folded into + :meth:`test_python35_fallback_implementations`, because the invariant it + pins has nothing to do with the shims: it is about what faking a version + around a real ``import`` costs whatever else is in the process. + + Self-contained on purpose: it installs the fake itself rather than relying + on :meth:`test_python35_fallback_implementations` having run first. Under + :mod:`unittest` a class's methods run in alphabetical order and under + :program:`pytest` in definition order, so which of the two goes first is a + property of their names and their position in this file -- neither of + which is a thing a regression test should rest on. A check that only fails + when it happens to run second is a check that a rename switches off. + + The closing lines are the symptom rather than a proxy for it. Building + an :class:`aenum.IntFlag` subclass is exactly what + ``pcapkit/const/reg/apptype.py:26`` does, and it is the statement that + raised ``AttributeError: 'TransportProtocol' object has no attribute + '__set_name__'`` for ten tests of :mod:`tests.project.test_public_api` + when this file ran before them under :mod:`unittest`. Asserting on it + here costs two lines and no ``import pcapkit``, which keeps this module in + the unit tier (see :mod:`tests._tiers`). + + """ + purge_modules(['pcapkit.utilities.compat_py35_probe']) + self.load_compat_as_python35('pcapkit.utilities.compat_py35_probe') + + # Not vacuous: the branch just executed did ``from aenum import StrEnum``, + # so aenum is in sys.modules by now whether or not the warm-up put it + # there. An empty sweep would mean this test had stopped checking + # anything, which is worth failing on rather than passing quietly. + readings = aenum_pyver_readings() + self.assertNotEqual( + readings, {}, + 'no aenum module reported a pyver, so this test checked nothing -- the ' + '< 3.11 branch of compat.py is supposed to have imported aenum (#687)') + self.assertEqual(sorted(set(readings.values())), [sys.version_info[:2]]) + + aenum = importlib.import_module('aenum') + + class TransportProtocolLike(aenum.IntFlag): + TCP = 6 + + self.assertEqual(TransportProtocolLike.TCP.value, 6) + def test_python35_fallback_implementations(self) -> None: purge_modules(['pcapkit.utilities.compat_py35']) - with mock.patch.object(sys, 'version_info', (3, 5)): - compat = load_module('pcapkit.utilities.compat_py35', 'pcapkit/utilities/compat.py') + compat = self.load_compat_as_python35('pcapkit.utilities.compat_py35') self.assertTrue(issubclass(compat.ModuleNotFoundError, ImportError))