fix(tests): put sys.modules back after a test loads modules from source (#674)

[JarryShaw]

Fixes #674.

What the defect actually is, and what #662 already changed about it

tests/_support.py's load_module writes to sys.modules twice over — the
module it was asked for, and a bare stub package for each parent of that
module's dotted name, via ensure_package (tests/_support.py:160) — and took
neither back off again. The stub carries nothing but a __path__, which is what
made this so quiet: it is emptier than the real package rather than
differently-shaped, so the next test to walk pcapkit.__all__ did not fail on
anything that looked like pollution. It reported the library as declaring no
exports.

The repro in the issue no longer fails as written, and that matters. Measured
on the issue's base and on b34f132f6:

$ python -m pytest tests/corekit/test_multidict.py tests/project/test_public_api.py -p no:cacheprovider -q
19 passed, 432 subtests passed in 0.16s
exit 0            # read from a file

#662 added restore_module_table, an autouse fixture in tests/conftest.py that
snapshots and restores the pcapkit region around every test. That repairs
the table after the polluting test whether or not the helper was ever fixed, so it
masked the symptom. The mechanism was untouched, and it is still observable by two
routes the fixture does not cover:

$ python -m pytest tests/corekit/test_multidict.py tests/project/test_public_api.py -p no:cacheprovider -q --noconftest
3 failed, 16 passed, 429 subtests passed
exit 1            # read from a file

$ python -m unittest tests.corekit.test_multidict tests.project.test_public_api
FAILED (failures=3)
exit 1            # read from a file

The three failures are the ones the issue names, and unittest needs no flag to
show them: the stdlib runner reads no conftest.py at all, and it is a supported
way to run this suite. So the honest statement is that #662 hid the symptom from
pytest and left the defect in the helper — which is exactly the shape of
accidental healing #662's own docstring warns against.

The blast radius was wider than the issue's one file

Pairing each suspect with tests.project.test_public_api under unittest, on
b34f132f6:

module	route into the loaders	before	after
tests.corekit.test_multidict	bootstrap_core_modules	3 failures	OK
tests.corekit.test_io	bootstrap_core_modules + load_module	3 failures	OK
tests.utilities.test_exceptions_warnings	bootstrap_core_modules	3 failures	OK
tests.corekit.test_module	load_module alone	3 failures	OK
tests.utilities.test_compat	load_module alone	10 errors	10 errors — see below

Two of the five leak through load_module on its own, never touching
bootstrap_core_modules. Fixing only the helper the issue names would have left
them leaking, which is why the fix sits at load_module/ensure_package.

The mechanism chosen, and how it handles absence

restore_modules_after(test) snapshots the pcapkit region and puts it back on
teardown through addCleanup. The restoring itself is restore_modules, already
in the file from #662 and already exact in both directions: it pops any name
under the prefixes that the snapshot does not hold, and rebinds the ones it does.
Absence is therefore restored as absence — pcapkit, pcapkit.corekit and
pcapkit.utilities did not exist before the test ran and do not exist after it.
A dict.update(snapshot) would pass the other two directions and fail this one,
and that is the direction the defect actually was.

Registered once per test however many times it is called, so
bootstrap_core_modules announcing itself and then making seven load_module
calls yields one cleanup, to the earliest snapshot — the only one taken before any
load had written anything. Re-snapshotting on the later calls would restore to a
table already holding the earlier loads, which is the original bug with more
steps. ArrangedRestoreTests.test_the_first_snapshot_is_the_one_kept pins that.

No call site changes. The loaders find the running TestCase from the calling
frames, which is the technique sample_path in the same module already uses and
documents: a helper that must be handed the running test is a helper every call
site can forget to hand it to, and the forgetting is silent — five call sites'
worth, none of which looked wrong. The nearest enclosing self wins, which also
resolves correctly for tests/utilities/_harness.py's bootstrap(), an
intermediate frame with no self of its own. A test= parameter is the escape
hatch for a frame that genuinely has none.

The walk takes the nearest running test rather than merely the nearest local
named self — see "what the cross-review changed" below for why that distinction
is load-bearing.

It also avoids touching tests/corekit/test_multidict.py. That began as a
coordination constraint — #667 was editing the same two setUp lines — and #667
has since been closed unmerged, so the constraint is gone; the no-call-site-changes
design stands on its own merits and is unchanged.

Every caller, and whether it is now safe

bootstrap_core_modules — 7 call sites, all safe:

caller	before	now
tests/corekit/test_multidict.py:12	leaked	restore arranged by the helper
tests/corekit/test_io.py:19	leaked	restore arranged by the helper
tests/utilities/test_exceptions_warnings.py:14	leaked	restore arranged by the helper
tests/utilities/_harness.py:91 (→ test_warning_emission, test_warning_filters, test_quiet_exceptions)	did not leak — each caller purges in tearDown	restore arranged as well, no longer dependent on the caller's tearDown
tests/corekit/test_protochain.py:15	already under isolate_modules (#662)	unchanged; the isolation covers it
tests/integration/test_module_loading.py:15	already under isolate_modules	unchanged
tests/utilities/test_decorators.py:16	already under isolate_modules	unchanged

One correction to the issue: tests/corekit/test_protochain.py is described as
calling bootstrap_core_modules in setUp with no tearDown. That was true
before #662; it now calls isolate_modules(self) first and was already safe.

load_module — the additional callers, all safe:
tests/corekit/test_module.py:14 and tests/utilities/test_compat.py:15,50
leaked and no longer do. tests/utilities/test_logging.py (11 sites) did not
leak, because its tearDown purges — it is now covered by the helper regardless.
tests/interface/test_core.py:130,133,184 was already under isolate_modules.

purge_modules — ~120 call sites, and it stays purge-only. Dropping a
real module is not a change another test can observe: the next one that wants it
imports it again and gets the same thing from the same source. Binding something
else over the name is what re-importing cannot undo, so restoration is owed by
the helpers that bind, not by this one. Its docstring now says so, rather than
leaving the asymmetry looking like an oversight. The unsafe thing was never the
purge — it was what a caller did after purging, and that is now guarded.

Guards, so a future caller cannot get it wrong quietly. ensure_package
refuses to bind a stub for a test that has arranged neither a restore nor
isolation, and the loaders refuse when no running test can be found. Both name the
helper that fixes it.

Evidence

Fails without the fix, passes with it. The defect-level runs are in the tables
above. For the new tests specifically, the helper was re-run with its restore
neutered — post-fix signatures, pre-fix behaviour, so each assertion fails on its
own terms rather than the module failing to import:

# restore_modules_after and the ensure_package guard neutered
$ python -m pytest tests/test_support_helpers.py -p no:cacheprovider -q
11 failed, 22 passed in 3.04s
exit 1            # read from a file

# as committed
$ python -m pytest tests/test_support_helpers.py -p no:cacheprovider -q
25 passed, 8 subtests passed in 2.93s
exit 0            # read from a file

The 11 are every new assertion the neutering reaches: all five borrowed cases, all
three stub names, and three of the four ArrangedRestoreTests cases.

Restoration asserted directly, not only the symptom.
_assert_module_table_unchanged compares the whole pcapkit region in all three
directions — additions gone, drops back, rebinds undone — and
test_the_stub_packages_are_absent_again_afterwards asserts the absence of the
three specific names, with a recorded flag proving the stubs were actually bound
while the probe ran so the assertion cannot pass vacuously.

Coverage does not go backwards. [tool.coverage.run] source = ["pcapkit"],
and no pcapkit line changed. Measured anyway over
tests/corekit tests/utilities tests/test_support_helpers.py: the coverage report output is byte-identical before and after (TOTAL 40227 16348 9404 958 51%; diff of the two reports exits 0 with no output — re-measured after
the rebase, so both sides share the same pcapkit). Test counts rose from
298 passed / 507 subtests to 304 / 515 — six tests and eight subtests added.

Version sensitivity. The frame walk and TestCase.doCleanups() outside run
were exercised on CPython 3.10.21, 3.11.15, 3.12.14, 3.13.15 and 3.14.7; both
changed files byte-compile on all five. f_locals is a dict up to 3.12 and a
FrameLocalsProxy from 3.13 (PEP 667), and .get answers on both — verified,
not assumed.

Wider run. tests/corekit tests/utilities tests/project tests/interface tests/cli tests/protocols/transport tests/integration/test_module_loading.py tests/test_support_helpers.py tests/test_docstring_contract.py tests/test_tier_guard.py → 600 passed, 1127 subtests, 7 failures, every one of
them FileNotFoundError: sample capture from sample_path (this worktree has 6
of the sample captures; the rest are generated, not committed). The integration
tier adds 51 more of the same. The full suite was deliberately not run — it
peaks over 40 GB RSS, and a full-suite green result would prove nothing here
anyway, since the full selection is precisely what masks this defect.

I am not claiming CI green, and CI cannot see this defect: every route that
exposes it either bypasses tests/conftest.py or runs the two files as the whole
selection. What the evidence above proves is that the four unittest pairings
that failed now pass, that the new tests fail against the unfixed helper, and that
the pcapkit coverage surface is untouched. What it does not prove is anything
about Python 3.15, or about the integration and runtime tiers, which need sample
captures this worktree does not have.

What the cross-review changed

A cross-review on a different model (Sonnet) was briefed to falsify rather than
bless, and returned GOOD TO GO with three findings. All three were reproduced
independently before acting on them.

1. A real hole in the frame walk, now closed. Taking "the nearest frame local
named self that is a TestCase" is not the same as "the running test". A free
function with a parameter it happens to call self presents an identical frame,
so one handed a different, already-finished TestCase won the walk — and a cleanup
registered on a finished test is never run. Measured: bootstrap_core_modules()
returned normally, raised nothing, and left ten names bound in sys.modules
after the real test had finished cleanly.

The walk now requires the candidate to be running, via TestCase._outcome, which
run() sets before setUp and clears in a finally. Verified on CPython
3.10.21 / 3.11.15 / 3.12.14 / 3.13.15 / 3.14.7 that it is set in all four phases a
loader is reachable from — setUp, the test method, tearDown, a cleanup — and
unset on an instance that has never run or has finished. An absent attribute is
distinguished from None by a sentinel, so a future runtime that drops _outcome
degrades to the previous behaviour rather than rejecting every candidate.

Two alternatives were rejected. Checking that the frame really is a method call on
the candidate (klass.__dict__[co_name].__code__ is frame.f_code) rejects a
decorated test method, whose frame belongs to the undecorated function while the
class attribute is the wrapper — a false rejection breaks a working test, which is
worse than the hole. Requiring an explicit test= at every call site forecloses it
too, but at the cost of the property that makes this fix cover callers nobody has
written yet.

ArrangedRestoreTests.test_an_idle_test_case_in_a_local_named_self_is_not_used
pins it, and fails on its own against a name-only walk (1 failed, 24 passed).

2. tests/cli/test_main.py has the same defect by an independent route, and this
PR does not fix it. It hand-rolls its own sys.modules writes and imports
tests._support not at all, which is exactly why tests/conftest.py names it as
the witness for #662's suite-wide guard. Paired under unittest:

$ python -m unittest tests.cli.test_main tests.project.test_public_api
FAILED (errors=10)      # ImportError: cannot import name 'show_flag_values'
exit 1                  # read from a file

Identical on origin/main and on this branch — measured both ways, so it is
neither caused nor cured here. Same shape as #674, different mechanism, and it
wants its own change; happy to file it.

3. The #667 rationale was stale. Corrected above.

The cross-review also independently re-derived the numbers in this description —
the 298/507 → 303/515 test counts, the byte-identical coverage TOTAL, the
f_locals type per version, #662's labels and changelog absence — and found no
overstatement beyond the #667 reference. It noted one looseness worth repeating
here: "no pcapkit line changed, so coverage cannot move" is imprecise as logic,
since a fix that changes which paths execute inside unchanged code could in
principle shift the report. The claim does not rest on that reasoning — it was
measured directly, and the reviewer reproduced the byte-identical result.

Labels, and no changelog entry

Labelled test only. fix was considered and rejected: the label set feeds
the release notes, fix is for defects in the shipped pcapkit package, and this
changes zero lines of it. The precedent is exact — #662, fix(tests): restore sys.modules after a test installs a stand-in module, carries test alone, and
issue #674 is itself labelled test.

No changelog entry, for the same reason and the same precedent: every entry in
docs/source/changelog/1.5.0.rst corresponds to a change in pcapkit/, and
neither #660 nor #662 appears there. A test-helper restoration gap is invisible to
anyone installing the library. Happy to add one if you would rather the 1.5.0 log
record it.

One separate defect found on the way, not fixed here

tests.utilities.test_compat is the fifth row of the table above and is the one
that still fails when paired with test_public_api — with the same 10 errors
before and after this change, so it is neither caused nor cured by it. It is a
different mechanism: test_python35_fallback_implementations fakes
sys.version_info = (3, 5) and, under that fake, executes
pcapkit/utilities/compat.py:143 (from aenum import StrEnum). On Python ≥ 3.11
that is the first-ever import aenum in the process, because the real-version load
takes the stdlib enum.StrEnum branch instead — so aenum/_common.py:17
(pyver = _sys.version_info[:2]) caches (3, 5) permanently. A later
import pcapkit then reaches aenum/_enum.py:1638 (if pyver < PY3_6:), takes
the Python-≤3.5 branch, and dies at line 1640 with
AttributeError: 'TransportProtocol' object has no attribute '__set_name__'.
Confirmed with a standalone two-step script involving no unittest or mock.
purge_modules/ISOLATED_PREFIXES only cover the pcapkit prefix, so nothing
purges aenum. Also masked under plain pytest, by pytest_sessionstart's eager
import pcapkit warming aenum correctly first. Not tracked by any open issue
(checked #660, #439, #575, #514, #647/#677). I have left it alone — it is not
#674 and wants its own review; say the word and I will file it.

[JarryShaw]

GOOD TO GO

Cross-review by an independent agent on a different model (Claude Sonnet) from the
one that wrote the change (Claude Opus), briefed to falsify rather than bless and to
treat disagreements as the valuable output. It ran read-only in its own throwaway
worktrees (both removed afterwards). Its verdict was GOOD TO GO with one
documentation correction and two disclosures; everything it raised was reproduced
independently before being acted on, and the two substantive findings have since been
fixed in a3d3754 — so this verdict refers to the reviewed revision plus those fixes.

Per-claim verdicts

#	Claim	Verdict	How it was checked
1	Restoration is exact, absences included	CONFIRMED	Five adversarial probes: a module raising mid-exec_module (caught and uncaught — table clean both ways), a legacy None sentinel entry round-tripping through a full purge/bootstrap/restore cycle, _under_prefix('pcapkit_extra', ('pcapkit',)) → False, and the parent-package-attribute axis (neither loader ever setattrs a parent, and restoring by object identity leaves existing wiring untouched).
2	No caller left unsafe	CONFIRMED for the loaders; one adjacent gap disclosed	Call sites re-enumerated by grep rather than read off the table above. Six pairings run under bare unittest: test_warning_emission, test_warning_filters, test_quiet_exceptions (all via _harness.bootstrap(), an intermediate frame with no self), test_logging, test_core → all OK. test_compat's 10 errors verified identical on the merge base — the separate aenum defect this PR names. Plus the tests/cli/test_main.py finding below.
3	The frame walk is sound	REFUTED as written; now fixed	A genuine hole was constructed and leaked ten names silently. See below.
4	The new tests bite	CONFIRMED both ways	Neutered run reproduced exactly, by failing test name. Also confirmed test_a_loader_with_no_test_case_on_the_stack_is_refused is not tautological despite the neutering not reaching it: the same thread probe run against the true pre-fix bootstrap_core_modules() at the merge base succeeds silently and leaks pcapkit.
5	#662 masks the pytest repro; unittest still shows it	CONFIRMED	All three routes run on the merge base and on the branch. Base: pytest → 19 passed (masked, rc 0); pytest --noconftest → 3 failed, rc 1; unittest → FAILED (failures=3), rc 1. Branch: all three rc 0.
6	No regression from the extra addCleanup / more frequent _reset_abc_caches()	CONFIRMED	run() orders tearDown() before doCleanups(), and registering in setUp makes this restore fire last among same-test cleanups (LIFO). The one existing addCleanup in the caller files (test_module.py's _register) operates on demo.* names, disjoint from the pcapkit prefix. Nothing in the suite counts cleanups. More frequent ABC resets are a superset of #439's concern, not a conflict, and run after the table is already whole.
7	The two flags have no bad path	CONFIRMED, self-correcting by construction	A double-isolate_modules scenario with real loads sandwiched between, against a warm 315-entry region, restored object-for-object with both flags False after. restore_modules is an absolute set-to-snapshot rather than a diff, and LIFO means the earliest (truest) snapshot always restores last, so nesting converges regardless of order.

What it found, and what changed

1. The frame walk took the nearest local named self, not the nearest running
test — a real hole, now closed. A free function with a parameter it happens to
call self presents the same frame as a method. Handed a different, already-finished
TestCase, it won the walk, and a cleanup registered on a finished test is never run.
Reproduced independently: bootstrap_core_modules() returned normally, raised nothing,
and left ten names bound in sys.modules after the real test had finished cleanly.

Fixed in a3d3754: the walk now requires the candidate to be running, via
TestCase._outcome, which run() sets before setUp and clears in a finally.
Verified on CPython 3.10.21 / 3.11.15 / 3.12.14 / 3.13.15 / 3.14.7 that it is set in
all four phases a loader is reachable from and unset on an instance that has never run
or has finished, with a sentinel distinguishing an absent attribute so a future runtime
that drops _outcome degrades to the previous behaviour instead of rejecting
everything. ArrangedRestoreTests.test_an_idle_test_case_in_a_local_named_self_is_not_used
pins it and fails on its own against a name-only walk (1 failed, 24 passed).

The reviewer also noted, fairly, that the RuntimeError's advice to pass test=self
does not read well for a plain function that has no self at all. The message and the
docstring now name a pytest-style function among the cases that reach it.

2. tests/cli/test_main.py has the same defect by an independent route, which this
PR does not fix. It hand-rolls its own sys.modules writes and does not import
tests._support at all — which is exactly why tests/conftest.py names it as the
witness for #662's suite-wide guard. Measured on origin/main and on this branch,
identically, so it is neither caused nor cured here:

$ python -m unittest tests.cli.test_main tests.project.test_public_api
FAILED (errors=10)      # ImportError: cannot import name 'show_flag_values'
exit 1                  # read from a file

Same shape as #674, different mechanism, and it wants its own change rather than being
folded in here. The reviewer also noted it sampled only this one file of roughly a dozen
with hand-rolled sys.modules writes, so the wider audit is open.

3. The #667 rationale in the description was stale. gh pr view 667 reports
CLOSED, mergedAt: null — closed unmerged, not open. Corrected in the description;
the design decision it justified stands on its own merits regardless.

On the description's own claims

The reviewer re-derived every specific number independently and found no overstatement
beyond the #667 reference: the test counts, the byte-identical coverage TOTAL, the
f_locals type per version, #662's labels and its absence from the changelog, and
#674's own label. It recorded one methodological caution worth repeating — checking
type(f_locals) at module scope gives a false negative, because it is a plain dict
there even on 3.13+; the type only shows as FrameLocalsProxy from inside a function
frame.

It also flagged that "no pcapkit line changed, so coverage cannot move" is imprecise
as logic, since a change in which paths execute inside unchanged code could in
principle shift the report. Agreed, and the claim does not rest on that reasoning: it
was measured directly, and the reviewer reproduced the byte-identical result.

Two things left open, stated plainly

• CI is not claimed green. The checks were still queued when this was written, and
  CI cannot see this defect in any case: every route that exposes it either bypasses
  tests/conftest.py or runs the two files as the whole selection. What the local
  evidence proves is in the description; Python 3.15 and the capture-dependent
  integration and runtime tiers are not covered by it.
• One theoretical path remains, and the reviewer and I agree it is not worth
  guarding: a KeyboardInterrupt landing between setattr(flag, True) and
  test.addCleanup(...) would leave the flag set with no cleanup registered. Not
  realistically triggerable, and no different in kind from any other two-statement
  sequence in Python.