fix(tests): keep a faked version and hand-bound stand-ins inside the test that made them (#687, #688) - #693
Conversation
…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
Cross-interpreter measurement, and a correction to #687's premiseThe description above measured only CPython 3.14.7. Repeated on 3.10.21, 3.11.15 and
Three things worth recording.
#687's premise needs a correction, and it makes the fix more load-bearing, not less. So One non-blocking observation found on the way, about #686's code rather than this |
|
GOOD TO MERGE Independent, read-only cross-review performed on a different model from the one that authored this change (per this repo's cross-review convention: disagreement is the value, not confirmation). No files edited, no mutating git, nothing pushed, no formal review state — this is a comment only. Verified from a temporary local clone (
No unguarded failure paths, dropped state, or missing bookkeeping found elsewhere in the diff. No GOOD TO MERGE |
GOOD TO GO — cross-review verdict (Sonnet), with one non-blocking finding now fixed in #698This pull request was raised by an agent, so it got an independent cross-review on a Verdict: GOOD TO GO, with one substantive non-blocking finding. Confirmed independently, each with its own derivation rather than by reading the
The finding, and it is a real one. Two things the reviewer could not verify, stated rather than glossed: a byte-for-byte One last honest note, unchanged from the description: CI cannot see either defect, |
…se's imports `WARM_BEFORE_FAKING` in `tests/utilities/test_compat.py` held the five modules that were *observed* to be imported inside the faked-version window, not the ones `pcapkit/utilities/compat.py` actually imports there. Line 63 of that file is an unconditional `import pathlib` on every branch reachable at `(3, 5)`, and `pathlib` was not on the list; nor were `collections.abc` (line 34), `contextlib` (153) and `enum` (179). The assertion did not fire because another module imports all four first: `collections.abc`, `contextlib` and `pathlib` at module scope in `tests/_support.py`, and `enum` in `test_compat.py` itself. It is not the runtime -- under `python -S` none of the four is in `sys.modules` at all -- so this really was one file's correctness resting on another file's import list, which is precisely the unenforced ordering #687's fix claimed to have removed. Measured: with `pathlib` forced cold and the old five-name list, the window leaks `['pathlib', 'pathlib._os']`; with all nine, it is empty even with five of the nine forced cold. The list is now read off `compat.py`'s guards rather than tuned until the assertion stopped complaining, and its docstring says which four never surface today and why they are there anyway. Warming an already-imported module is a `sys.modules` lookup, so the four cost nothing. Also records, in the same docstring, that warming `aenum` is load-bearing below 3.11 as well. #687 reads as a `>= 3.11` defect and that is true of this file -- below 3.11 `setUp`'s real load warms aenum honestly first -- but faking the version around aenum's first import in a fresh CPython 3.10.21 process raises `ImportError: cannot import name 'FlagBoundary' from 'enum'` outright rather than poisoning a cache. `pytest -q tests/utilities/test_compat.py tests/cli/test_main.py tests/project/test_module_isolation.py` gives `17 passed`, and `python -m unittest tests.utilities.test_compat tests.project.test_public_api` gives `Ran 15 tests` / `OK`. Measured on 3.10.21, 3.11.15, 3.12.13 and 3.14.7. No `pcapkit/` line changes, and `coverage report` over the same five-file selection #693 used is byte-identical against `f0999858e` (`diff` exit 0), at 40 tests and 566 subtests. Follow-up to #693, found by cross-review after that pull request had merged. Refs #687
make pylint,make mypy,make isort)make testpasses, and a test case covers the changedocs/source/changelog/and regeneratedCHANGELOG.md, if the change is user-visibleWhat is the purpose of your pull request?
fix— corrects a defectfeat— adds a featureperf— changes performance, not behaviourrefactor— changes neither behaviour nor performancetest— tests onlydocs— documentation onlyci— workflows or build toolingchore— anything elseThe subject line reads
fix(tests):rather thantest:, matching #662 and #686 —the two immediately comparable changes — while the label is
testalone, as onboth of those. Zero
pcapkit/lines change here, sofixwould put atest-infrastructure change into the release notes as a defect in the shipped
package.
Description of your pull request and other information
Fixes #687 and Fixes #688. Both were found while auditing #674 and both were
deliberately left out of #686; they are the two remaining ways a test in this suite
leaves the process worse than it found it.
Why one pull request and not two
They are independent mechanisms in different files, which argues for two. Against
that, decisively:
tests/project/test_module_isolation.pyhas to carry a case foreach of them, so two pull requests would both edit that file and the second would
rebase over the first — and, worse, the new
UnmaskedOrderTestscase for #687 onlypasses once
tests/utilities/test_compat.pyis fixed, and the case for #688 onlyonce
tests/cli/test_main.pyis. Split, each pull request lands a test that failsuntil its sibling merges. They also share three docstring corrections, each of which
names both files.
Both defects reproduce on
f0999858e, on a routetests/conftest.pycannot maskI re-ran the measurements rather than taking the issues' word for them (both say the
numbers were not independently re-run).
python 3.14.7, worktree atsys.path[0],pcapkit.__file__asserted inside the worktree, exit codes read from.rcfiles.f0999858e)python -m unittest tests.utilities.test_compat tests.project.test_public_apiRan 14,FAILED (errors=10)Ran 15,OKpytest -p no:randomly --noconftest -q tests/utilities/test_compat.py tests/project/test_public_api.py10 failed, 4 passed15 passed, 432 subtests passedpython -m unittest tests.cli.test_main tests.project.test_public_apiRan 16,FAILED (errors=10)Ran 16,OKpytest -p no:randomly --noconftest -q tests/cli/test_main.py tests/project/test_public_api.py10 failed, 6 passed16 passed, 432 subtests passedAll ten of #687's errors were the one the issue names, verbatim:
AttributeError: 'TransportProtocol' object has no attribute '__set_name__', raisedat
aenum/_enum.py:1640frompcapkit/const/reg/apptype.py:26. #688's ten splitnine/one:
ImportError: cannot import name 'show_flag_values' from 'pcapkit.utilities.compat' (unknown location)and the same forSeekErrorfrompcapkit.utilities.exceptions. Each file alone passes before and after, which iswhat makes them ordering defects rather than broken tests.
#687: which of the three suggested directions, and why not the other two
Chosen: direction 1, warm the caches before the fake — but with the ordering
asserted rather than assumed, which is the stated weakness of that direction
turned into a checked precondition.
load_compat_as_python35importsWARM_BEFORE_FAKINGunder the true version, snapshotsfrozenset(sys.modules),installs the fake, and then asserts that the set of names added during the window is
empty. That generalises past aenum: it catches any module first-imported under
the lie, which matters because the blast radius is wider than one package. Measured
in a bare interpreter, the faked window imports 33 modules including
typing,typing_extensions,decimal,datetime,sqlite3,inspect,dis,ast,annotationlib,threadingandnumbersalongside the fiveaenummodules. Afterwarming
('aenum', 'decimal', 'threading', 'typing', 'typing_extensions')the deltais
[].Direction 3, reload
aenum._common, is dead on measurement.aenum/_enum.py:2is
from ._common import *, sopyveris copied into every aenum module'sglobals, and
_enum.py:1640branches on its own copy. Measured:importlib.reload(aenum._common)gives_common.pyver == (3, 14)and leaves_enum.pyver == (3, 5)— i.e. it repairs the reading nobody uses and leaves the onethat breaks.
Direction 2, a subprocess, is rejected on coverage and on diagnosability.
[tool.coverage.run]here issource = ["pcapkit"],branch = true, with noparallel/concurrencysetting and noCOVERAGE_PROCESS_START.pth. coverage keyson file path, so today's in-process
load_module('…compat_py35', 'pcapkit/utilities/compat.py')is what credits the< 3.6,< 3.8,< 3.9and< 3.11branches ofpcapkit/utilities/compat.py; moving that to a child processdrops the measurement, which is coverage going backwards for a test-only change.
Separately, the existing test asserts on live objects —
compat.Collection.__subclasshook__against locally defined classes,
cached_property.__set_name__raisingTypeError,decimal.getcontext().precinside the context manager — none of which crosses aprocess boundary; they would become a child script reporting pass/fail as text, and a
failure would be captured stdout instead of a named assertion.
The
aenum._common.pyverassertionassert_aenum_is_not_poisonedis registered as a cleanup insetUp, beforeanything else, so — cleanups being LIFO — it runs after the module-table restore
load_modulearranges and after the test body however it ended. It sweeps everyaenummodule holding a tuplepyverout ofsys.modulesrather than naming two,because the copies can disagree; on 3.14.7 that is
aenum,aenum._common,aenum._constant,aenum._enumandaenum._tuple.test_faking_the_version_leaves_aenum_reading_the_real_oneadditionally refuses to pass vacuously (it asserts the sweep is non-empty) and builds
a two-member
aenum.IntFlagsubclass, which is whatpcapkit/const/reg/apptype.py:26does and what raises theAttributeErrorwhen thecache is wrong — so the symptom is asserted, not a proxy for it, and without needing
import pcapkitin a unit-tier module.#688: what the writes were routed through
tests._support.isolate_modules(self, ISOLATED)insetUp, replacing thehand-rolled purge loop that used to sit at the top of
_load_cli_module. Thestand-in construction is unchanged — that part was never the defect — so the diff is
"the purge loop becomes an isolation that also restores".
ISOLATEDisISOLATED_PREFIXES + ('emoji',), which fixes a second leak nobody had filed: thefaked
emojiwas outside the prefixestests/conftest.pycovers, so it was notmerely unrestored but unmasked, surviving a normal pytest run and leaving whatever
imported
emojinext with a two-attributeSimpleNamespace— or, from one test, aclass whose
emojizeraisesUnicodeEncodeError.assert_module_table_restoredcomparessnapshot_modules(ISOLATED)taken insetUpagainst the region after the restore, in all three directions a restore can be wrong:
added, removed and rebound by identity. Absence is checked as well as presence, which
is the direction #674 actually was.
tests/_support.pyis not touched. Nothing needed extending:isolate_modulesalready takes a
prefixesargument, andsnapshot_modulesandISOLATED_PREFIXESare already public.
Yes, the docstrings needed correcting — three of them, not one
tests/conftest.pysaid "The three known leaks are also fixed at their call sites,with
isolate_modules". Onf0999858ethat was true of two. The third wastests/cli/test_main.py, which this pull request fixes, so the sentence becomestrue; the paragraph now says which one it was and why a guard that repairs a leak
also hides it.
tests/project/test_module_isolation.pyis the worse one, and the one the issue's"irony worth checking" points at. It recorded
tests/cli/test_main.pyas"deliberately left as it was, as the standing witness that the guard covers a file
which has not opted into anything" — a decision to leave the very defect tests/cli/test_main.py leaves hand-written sys.modules stand-ins behind: 10 errors when paired with test_public_api #688 is
about. That trade is now reversed and the reasoning written down: the guard healed
the leak on every run, which is exactly why it took an audit rather than a red test
to notice. The witness role is deliberately left vacant rather than replaced by a
test whose job is to leave
sys.modulesbroken.test_a_polluter_outside_the_helpers_is_coveredis renamed
test_the_cli_polluter_order_passes, since it no longer covers apolluter outside the helpers.
tests/const/test_const_enum_lookup.pyjustified its ownsetUppurge with"depending on suite order, that stub can still be sitting there when this module
runs", naming
tests/cli/test_main.py. Now it cannot. The purge stays — theconvention is worth keeping, and the next stand-in written without the helpers
would be a live hazard — but the claim is corrected.
A test that fails without each fix, exit codes read from files
Each fix was disabled in isolation (leaving the new assertions in place) and the run
re-measured:
WARM_BEFORE_FAKING = ()python -m unittest tests.utilities.test_compatRan 5,FAILED (failures=5)WARM_BEFORE_FAKING = ()python -m unittest tests.utilities.test_compat tests.project.test_public_apiFAILED (failures=5, errors=10)isolate_modules→ old purge looppytest -q tests/cli/test_main.py6 failedisolate_modules→ old purge looppython -m unittest tests.cli.test_main tests.project.test_public_apiFAILED (failures=6, errors=10)Note the asymmetry, and it is worth knowing rather than glossing. #688's assertion
fails under a plain
pytestrun, because the cleanup is registered ahead of theisolation and therefore runs before
restore_module_tablegets to heal anything.#687's does not:
pytest_sessionstartimportspcapkit, and soaenum, beforethe first test, so there is no cold cache left to poison and both guards pass whether
or not the fix is in place — measured,
5 passedwith the warm-up emptied. That iswhy
UnmaskedOrderTestsexists: it runs the pairing under the stdlibunittestrunner in a subprocess, which is the only automated route on which a #687 regression
is visible at all.
Coverage does not move
coverage run -m pytest -p no:randomly -qovertests/utilities/test_compat.py tests/cli/test_main.py tests/const/test_const_enum_lookup.py tests/project/test_public_api.py tests/project/test_module_isolation.py,run against this branch and against
f0999858e(the five changed files swapped out ofgit show HEAD:…, then swapped back andgit diff --statre-checked):What moved is the test count: 37 passed → 40 passed, 566 subtests unchanged. The
three are one new test in
test_compat.pyand two intest_module_isolation.py.What the local evidence proves, and what it does not
CI cannot see either defect, and could not before this change either — the
unit-tests.ymljobs run plainpytest, which is exactly the selection that masksboth. So no claim is made here about CI being green on the defect: what CI will now
exercise is
UnmaskedOrderTests, which spawns the unmasked runs itself, and #688'sin-file assertion. The measurements above are local, on CPython 3.14.7, and are the
only evidence offered.
On the first two checkboxes, so they are not taken for more than they are worth:
make pylint,make mypyandmake isortall takepcapkitas their only target,and this change touches no
pcapkit/file, so the coding-style box is tickedvacuously — the import ordering here follows the convention the neighbouring test
modules already use.
make testruns the suite bar the integration tier, which needs tens of gigabytes ofresident memory on this machine, and a full pass would in any case prove nothing
here — that selection is precisely what hides both defects. The test checkbox is
ticked for the narrower selections actually run:
tests/project,tests/cli,tests/utilities(231 passed, 571 subtests),
tests/const(40 passed, 798 subtests),tests/test_support_helpers.py+tests/interface+tests/corekit/test_protochain.py(51 passed, 17 subtests), plus the eight pairing runs tabulated above.
No changelog entry
Following #686's reasoning, and checked rather than assumed: every entry in
docs/source/changelog/1.5.0.rstcorresponds to apcapkit/change, and neither#660/#662 nor #674/#686 appears there. Zero
pcapkit/lines change here, so theshipped package is byte-identical and there is nothing user-visible to record.