fix(tests): restore sys.modules after a test installs a stand-in module (#660)

[JarryShaw]

Fixes #660

What was wrong

Twelve tests in tests/project/ failed with TypeError: type 'ProtocolBase' is not subscriptable depending only on what had run before them. The same three files passed in the reverse order.

pytest -p no:cacheprovider -q tests/corekit/test_protochain.py \
    tests/project/test_public_api.py tests/project/test_documentation_claims.py
-> EXIT=1 ... 12 failed, 5 passed, 1 warning, 2 subtests passed in 1.44s

tests/_support.py's fake-module helpers isolated a test by purging sys.modules on entry and never restoring on exit. tests/corekit/test_protochain.py installed a stand-in ProtocolBase that is not Generic and finished without removing it, so every later class X(Protocol[...]) raised at pcapkit/protocols/misc/pcap/frame.py:59.

The design choice, and why the alternatives were rejected

The invariant is a test leaves the pcapkit region of sys.modules exactly as it found it. Three ways to get there, and this PR does the first two:

1. Restore what was displaced. snapshot_modules/restore_modules plus isolate_modules(self), which purges on entry and restores on teardown through addCleanup. addCleanup rather than tearDown so it also runs when setUp raises half-way through installing the stand-ins.
2. Make the leak structurally impossible, not merely unlikely. An autouse fixture in tests/conftest.py restores the region after every test. This is load-bearing rather than belt-and-braces: three separate files leak this way, and one of them — tests/cli/test_main.py — rolls its own purge loop and never imports tests._support, so nothing in that module could have fixed it. It is left unchanged on purpose, as the standing witness that the guard covers a file which has not opted into anything. The installers additionally now require the running test and refuse to bind a stand-in without active isolation, so the same mistake is a RuntimeError at the call site rather than failures in another directory.

Rejected: adding the missing tearDown. It fixes the files that have it and nothing else. The next file written without one reintroduces the identical order-dependent failure — that is unlikely, not impossible. tests/cli/test_main.py shows the pattern already recurred independently.

Rejected: removing the fake-install idiom entirely. Not achievable. load_module/bootstrap_core_modules must bind into sys.modules, because that is how the module under test resolves its own from pcapkit... import ... lines.

Rejected, and it would have been the wrong fix: moving test_protochain.py so it sorts elsewhere, or relying on a neighbouring test's purge to heal the state. Accidental healing by a neighbour is exactly why this stayed hidden — the file sorts last inside tests/corekit/, and the full CI selection passes only because unrelated directories sort between it and tests/project/. A fix of that shape re-hides the defect and the next file added between the two re-exposes it.

purge_modules keeps its purge-only behaviour, documented as such: it is correct for the ~120 callers that purge and then import only the real package.

tests/integration/ — it does share the defect

Flagged as unknown in the issue. Checked, and it leaks, with a different failure mode. tests/integration/test_module_loading.py calls purge_modules + bootstrap_core_modules() inside the test body with no cleanup at all, leaving a stub pcapkit carrying only __path__:

tests/integration/test_module_loading.py tests/project/test_public_api.py
-> EXIT=1   3 failed, 8 passed          # AssertionError, __all__ entries resolve to nothing
the same two reversed
-> EXIT=0   11 passed

It is healed by accident too — EndToEndTestCase.setUpClass purges — so the tier passes as a whole. Fixed at the call site here.

Evidence

Exit codes read from files, .venv/bin/python 3.14.7, pytest 9.1.1.

selection	before	after
the three files from the issue	EXIT=1 12 failed, 5 passed	EXIT=0 17 passed
the same three, reversed	EXIT=0 17 passed	EXIT=0 17 passed
test_module_loading.py then test_public_api.py	EXIT=1 3 failed, 8 passed	EXIT=0 11 passed
tests/cli/test_main.py then test_public_api.py	EXIT=1 10 failed, 6 passed	EXIT=0 16 passed

Wide cross-tier selection (tests/test_support_helpers.py tests/project tests/cli tests/const tests/dumpkit + the two polluters), both EXIT=0: 156 passed / 722 subtests before, 165 passed / 722 subtests after — the +9 are the new tests. tests/corekit tests/utilities tests/interface together: 292 passed, EXIT=0, 315 MB peak RSS.

Nine new tests, all of which fail on main:

• tests/project/test_module_isolation.py runs pytest in a subprocess over each selection above. Cross-file ordering is not observable from inside a single test, so this is the only way to state the invariant.
• tests/test_support_helpers.py::StandInsDoNotOutliveTheirTestTests runs the real polluting test case through unittest.TestResult and diffs sys.modules across it. Deliberately not via pytest: the conftest fixture would clean up either way and the assertion would pass whether or not the test case itself had been fixed.
• tests/test_support_helpers.py::SnapshotRestoreTests covers the restore as an exact inverse over all three cases — a name added, a name dropped, a name rebound. The rebind is the one twelve tests/project/ tests fail with TypeError: type 'ProtocolBase' is not subscriptable depending on what ran first: the fake-module helpers purge on entry and never restore on exit #660 turned on.

Performance

The guard makes each test that does not purge for itself re-import the package, unless the table starts warm — so pytest_sessionstart imports it once. Over the 96 tests of tests/project/: 1.59s unguarded, 9.34s guarded from a cold table, 0.35s guarded from a warm one. Measured, and the numbers are in the docstring.

Warming does not help a class that purges in setUpClass, and that is worth being explicit about because the obvious reading is wrong: such a class purges after the hook and before the first snapshot, so its snapshot is whatever it left behind. Where the class loads something straight after its own purge the snapshot is populated anyway; where it purges and defers the import to its test methods, every method re-imports.

Exactly one class in the suite did the latter — tests/integration/_helpers.py::EndToEndTestCase — so it now re-imports in its own setUpClass. Without that, the tier's 92 test methods across 28 classes each paid a measured 0.707s re-import instead of one per class, some 45s. Probed with a throwaway subclass whose second and third methods report whether pcapkit is still in sys.modules:

tree	method 1 entry	method 2	method 3
unmodified main	cold	warm	warm
this branch without the re-import	cold	cold	cold
this branch as it stands	warm	warm	warm

The re-import is wrapped in except Exception because it is an optimisation and must not change a failure mode. Three of the tier's 28 classes carry no @skipUnless(HAS_RUNTIME, ...), two of which subclass EndToEndTestCase, so on a checkout without the runtime dependencies an unguarded import would raise out of setUpClass and error those classes where before their individual tests failed. Verified by patching importlib.import_module to raise: unguarded propagates, guarded survives and the table simply stays cold. BaseException is not caught.

Coverage

No pcapkit line changes, so [tool.coverage.run] source = ["pcapkit"] cannot move. The test-side measure is the count: +9 tests, and subtest totals unchanged at 722 in the cross-tier selection and 432 in the three-file selection, i.e. nothing was narrowed to make this pass.

Notes

• tests/cli/test_main.py is knowingly left leaking at the source level, covered by the conftest guard. Fixing it inline would remove the only witness that the structural half works; it has no tests._support dependency to hang isolate_modules off without restructuring the file.
• Two leaks are out of scope and unfixed: tests/project/test_setup.py binds a fake setuptools, and tests/cli/test_main.py pops emoji. Neither is under a pcapkit prefix, so neither is covered by the guard's default prefixes, and neither has been observed to break anything.

[JarryShaw]

NEEDS CHANGES

Cross-review by an independent agent on a different model (Sonnet), briefed to falsify rather than confirm, per the house rule that an agent-raised PR gets a cross-review on a model other than the authoring one (this PR was authored by Opus). It ran read-only, in its own git worktree at the reviewed commit 9abc1f225, with unmodified main extracted separately for the before side.

The required change

tests/conftest.py, pytest_sessionstart docstring — the claim that warming helps "the tiers that purge once per class" is false for tests/integration/_helpers.py::EndToEndTestCase, and that tier regresses by ~50-58s.

That class's setUpClass purges pcapkit and deliberately defers the re-import to each test method — its own docstring says so. Every other setUpClass in the suite (test_const_enum_get.py, test_const_enum_lookup.py, test_dispatch_registry_unit.py, test_option_roundtrip_unit.py) purges and immediately re-imports, so the function-scoped snapshot captures a populated table and the docstring's claim does hold for them. EndToEndTestCase is the one exception, and it is the one the sentence was meant to cover.

Measured by subclassing EndToEndTestCase with three dummy test methods that need no sample capture and log 'pcapkit' in sys.modules on entry: method 1 sees False (expected, first import), but methods 2 and 3 also see False — the guard restores to the empty snapshot after every test, so each of the tier's test methods pays a full re-import instead of one per class. 83 test methods across the 9 files that subclass it, at 0.6-0.85s per re-import.

Independently arrived at the same finding before the review landed (measured import pcapkit at 0.707s and 92 test methods across 28 classes under tests/integration/), so this is two measurements agreeing.

The reviewer's proposed fix is better than the one I had planned and is what will be applied: make EndToEndTestCase.setUpClass re-import immediately after purging, mirroring the enum tier, so the snapshot captures the populated state and the purge-once-per-class optimisation is recovered exactly — rather than merely correcting the docstring to admit the cost, and rather than complicating the fixture with a heuristic.

On that last point the reviewer also talked me out of a change I was considering: a __spec__ is None heuristic that would leave genuinely-imported modules in place and only strip hand-built stand-ins. Its argument, which I accept: that optimises the wrong axis — the driver of the regression is "did setUpClass re-import immediately or defer it", which is orthogonal to whether a module object looks synthetic — and it would risk the identity preservation that test_const_enum_get.py's cls.enums depends on across its own methods. The unconditional full restore stays.

It classes this as a performance/documentation defect, not a correctness one: no test result changes and nothing newly fails.

On the fix mechanism itself: no counter-example found

The central question put to it was whether this is a heal-by-neighbour fix in disguise. It tried orderings this PR does not:

• the polluter in the middle of three files, and first among four
• a different victim outside tests/project/ — tests/corekit/test_fields_misc_packet_context.py, which does a call-time from pcapkit.protocols.internet.mh import MH
• two polluters in sequence (test_protochain.py + test_module_loading.py) then a victim
• polluter → a deliberate "accidental healer" (a scratch file that purges and re-imports the real package without isolating) → victim, probing the exact mode that hid this bug
• a single test method from the polluter rather than the whole class, then a victim

Every ordering exited 0. No counter-example.

It also verified the two things that would have made the fix hollow:

• It does not depend on pytest. From-scratch script, no pytest and no conftest: __editable__ finder stripped from sys.meta_path, tree at sys.path[0], pcapkit.__file__ printed and asserted before anything else. On main the stand-in leaks and the next import dies on TypeError: type 'ProtocolBase' is not subscriptable; on the branch it does not. So isolate_modules's addCleanup genuinely holds under bare unittest.
• The new tests are not vacuous. tests/project/test_module_isolation.py copied verbatim onto unmodified main gives EXIT=1, 3 failed, 1 passed — the three forward-order cases fail and the reverse-order control correctly still passes. And StandInsDoNotOutliveTheirTestTests is confirmed not rescued by the conftest fixture, because the borrowed case never goes through pytest.

Numbers reproduced: 12 failed, 5 passed / EXIT=1 before and 17 passed / EXIT=0 after, matching exactly; guarded-cold 9.13s against the claimed 9.34s and guarded-warm 0.35-0.38s against 0.35s.

Two findings it raised that are not blocking, both accepted

• restore_modules is an inverse over module identity, not over in-place mutation. A test that mutates an already-warm real module's attributes — populating a registry dict in place — rebinds nothing and is not rolled back. Not what twelve tests/project/ tests fail with TypeError: type 'ProtocolBase' is not subscriptable depending on what ran first: the fake-module helpers purge on entry and never restore on exit #660 was about and not a regression, but the docstring's "exact inverse" framing does not cover it and will get a caveat.
• _method_names's sorted(...)[0] would raise IndexError if ProtoChainTests ever had zero test methods. A loud failure rather than a silent pass, so low risk.

Where it disagreed with its own brief, and one number it could not reproduce

Reported honestly rather than folded away: it measured the wide cross-tier selection as 150 → 159 passed where this PR says 156 → 165. The delta is +9 either way and subtests match exactly at 722, so the mechanism agrees; the absolute counts differ, most likely an optional-dependency skip/collection difference between environments. It could not pin the cause down and says so. It also did not reproduce the supplementary 292 passed / 315 MB figure for tests/corekit tests/utilities tests/interface, for time reasons.

One process note, in fairness to it: I switched the shared worktree to another branch shortly after dispatching it, which could have invalidated its measurements. It had already detected the mismatch itself and built its own verified worktree, so nothing in its report is an artefact of my error — and when I messaged it to redirect, it declined and pushed back on one of my two technical claims, correctly.

Scope verified: one commit, 9abc1f225, authored Jarry Shaw <jarryshaw@icloud.com>; nine files, all under tests/; none of the off-limits files and nothing under .github/. It diffed against the PR's actual base 0c7f2b7c9 rather than the local main ref, which is ahead.

The docstring and EndToEndTestCase.setUpClass changes will be pushed as an amendment to this branch, and re-verified.

[JarryShaw]

NEEDS CHANGES addressed — pushed as 08dc75caf

All three items from the cross-review are in, one commit as before (force-with-lease amend, nothing else on the branch).

1. EndToEndTestCase.setUpClass now re-imports after purging

Took the reviewer's fix rather than merely correcting the docstring, because it recovers the optimisation instead of documenting its loss. tests/integration/_helpers.py:

purge_modules(['pcapkit'])
importlib.import_module('pcapkit')

importlib is already bound in that module via import importlib.util, so no new import statement.

Re-measured independently with a throwaway EndToEndTestCase subclass whose first method imports the package and whose second and third only report whether it is still in sys.modules — no sample captures needed, so it runs in this checkout. Three trees:

tree	a_enter	a_exit	b_warm	c_warm
unmodified main	False	True	True	True
this branch as first pushed	False	True	False	False
this branch, amended	True	True	True	True

The middle row is the regression, reproduced independently of the reviewer. The bottom row restores main's behaviour and slightly improves on it, since a_enter is warm too.

Two measurements agreeing on the cost, by different routes: the reviewer counted 83 methods across the 9 files that subclass it at 0.6-0.85s per re-import; I counted 92 test methods across 28 classes in tests/integration/ by AST and measured import pcapkit at 0.707s (0.736s process total less 0.029s interpreter startup, median of 5). Either way ~45-58s, now zero.

2. The pytest_sessionstart docstring

Rewritten, and it now says the thing that is actually true rather than a softened version of the wrong thing. It no longer claims warming helps a class that purges per class, and explains why the obvious reading fails: such a class purges after the hook and before the first snapshot, so warming the table earlier cannot affect its snapshot at all. It distinguishes the two shapes — a class that loads something straight after its own purge (snapshot populated, nothing lost) from one that defers the import to its methods (every method re-imports) — names EndToEndTestCase as the sole instance of the latter, records the cost, and tells a future per-class purger to re-import.

3. restore_modules — the identity-versus-mutation caveat

Accepted and documented. The docstring now says it is an inverse over the bindings and not over module contents: a test that mutates an already-imported module in place rebinds nothing, so there is nothing for this to undo, and isolating against that needs a purge so the next import rebuilds from source — which is what purge_modules' callers are doing. #660 was a rebinding, which is why this is the right shape for it.

Not changed, with reasons

• The __spec__ is None heuristic stays rejected. I had been considering it; the reviewer's argument against is better than my reason for and is now on the record in the previous comment. The unconditional full restore is unchanged.
• _method_names' sorted(...)[0] is left as is. With zero test methods on ProtoChainTests it raises IndexError — loud, immediate, and in a test whose entire purpose is that class. Guarding it would trade a loud failure for a skip.
• tests/cli/test_main.py still leaks at the source level, deliberately, as the witness that the structural guard covers a file with no tests._support dependency.

Re-verification after the amendment

Exit codes read from files.

• the three-file order from the issue: EXIT=0, 17 passed, 432 subtests in 0.13s
• test_module_isolation.py + test_support_helpers.py + test_protochain.py + tests/const + test_module_loading.py: EXIT=0, 48 passed, 249 subtests, 208 MB peak RSS
• the subclass probe above: 3 passed, warm on every method

On the reviewer's one unreproduced number — it measured the wide cross-tier selection as 150 → 159 where this PR says 156 → 165, with the delta (+9) and the subtest count (722) matching exactly both times. That is an absolute collection count differing between environments, not a disagreement about the change, and it is recorded rather than reconciled.

[JarryShaw]

GOOD TO GO — cross-review, second pass

The same Sonnet reviewer re-ran its checks against the amended commit in a fresh worktree it built itself, and returned GOOD TO GO. Its earlier NEEDS CHANGES item is closed.

What it re-verified independently, with its own probes rather than mine:

• EndToEndTestCase warmth restored. Its own dummy subclass sees 'pcapkit' in sys.modules True at entry to methods 1, 2 and 3 (False for 2 and 3 before). Its 3-method probe dropped from 2.27s to 0.65s — one re-import instead of three.
• The twelve tests/project/ tests fail with TypeError: type 'ProtocolBase' is not subscriptable depending on what ran first: the fake-module helpers purge on entry and never restore on exit #660 fix survived the amendment. The three-file order from the issue (EXIT=0, 17 passed), its own "polluter in the middle" and "polluter → accidental healer → victim" orderings (EXIT=0 both), and the bare-unittest/no-pytest leak probe with pcapkit.__file__ printed and asserted.
• All three amended docstrings re-read, no remaining inaccuracies. It specifically stress-tested the categorical claim "this does not help a class that purges in setUpClass" and found it holds.
• Scope re-checked against the real base 0c7f2b7c9: one commit, ten files, all under tests/.

It also had no objection to the two things left as they were — _method_names' IndexError on an empty class, and tests/cli/test_main.py's deliberate source-level leak.

One disagreement, and I have kept my change over its advice

Its verdict was given on 08dc75caf, in which setUpClass's new re-import was unguarded. It judged that correct as shipped, on two grounds. I have since guarded it anyway — 6e7c998df — because the first ground is factually wrong.

It wrote that "every concrete subclass across tests/integration/ already carries @unittest.skipUnless(HAS_RUNTIME, ...)", which would mean setUpClass is never reached without the runtime dependencies. Counted by AST over the tier, 3 of the 28 test classes carry no HAS_RUNTIME skip, and two of those three subclass EndToEndTestCase:

test_output_formats.py::PlistRoundTripTests        decorators=[]
test_pcapng_end_to_end.py::PcapngUnescapedKeyTests decorators=[]
test_module_loading.py::ModuleLoadingIntegrationTests  (not an EndToEndTestCase)

So the line is reachable without the dependencies, through those two. Forced the failure directly, by patching importlib.import_module to raise ModuleNotFoundError for pcapkit and calling setUpClass:

UNGUARDED: setUpClass PROPAGATED ModuleNotFoundError: No module named 'tbtrim'
             -> on a deps-less checkout this errors the whole class
GUARDED:   setUpClass survived the failing import
             -> table stays cold; individual tests fail as they did before

Its second ground — that failing loudly is more informative — is a fair argument, and it verified there is no partial sys.modules residue either way. I still come down the other side: this line is a performance optimisation, and an optimisation should not change a tier's failure mode. On main a dependency-less checkout gives those two classes individual test failures; unguarded it gives setUpClass errors instead. Nothing is hidden by swallowing, because those tests import the package themselves in the body and surface the same underlying error. So the guard preserves existing behaviour and costs only the optimisation on a checkout that cannot run the tier anyway. BaseException is deliberately still not caught.

The two classes are named in the docstring, so the next reader does not have to rediscover which subclasses make the branch reachable.

Verified after guarding: three-file order EXIT=0 17 passed; test_module_isolation.py + test_support_helpers.py + tests/const + test_module_loading.py EXIT=0, 44 passed / 249 subtests; the warmth probe still True on all three methods.

Final state: 6e7c998df, one commit, ten files, all under tests/, unmerged and awaiting your review.