fix(tests): keep a faked version and hand-bound stand-ins inside the test that made them (#687, #688)

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

tests/conftest.py

@@ -110,17 +110,26 @@ 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
     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.
 
+    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

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