Conversation
…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
74c36fa to
8168dee
Compare
GOOD TO GO — cross-review verdict (Sonnet), two non-blocking imprecisions now fixedThis was raised by an agent, so it got an independent cross-review on a different Verdict: GOOD TO GO. Both of its non-blocking findings were about docstring prose Confirmed independently:
The two findings, both about the docstring, both reproduced here before acting:
The docstring now names the three from It took the most interesting objection available seriously and could not make it stick: One thing it did not verify, stated rather than glossed: the byte-identical |
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 lint targets take
pcapkitas their only argument and nopcapkit/file changeshere, so the first box is ticked vacuously. The third is ticked for the selections in
the table below rather than for a full-suite run, which needs tens of gigabytes of
resident memory on this machine.
Follow-up to #693, from a cross-review that finished after it merged
#693 fixed #687 by importing what the faked-version window needs before installing
the fake, and asserting that nothing at all was first-imported inside the window. The
claim in its docstring was that this "turns the ordering from relied-on into checked".
A cross-review on a second model pointed out that it was not quite that yet, and it
was right.
WARM_BEFORE_FAKINGheld the five modules that were observed to appear in thewindow, not the ones
pcapkit/utilities/compat.pyactually imports there. Readingthe guards off that file, every import reachable at
(3, 5)is:from collections.abc import Container, Iterable, Sizedimport pathlib— unconditional, the<= (3, 4)branch not being takenfrom threading import RLocktypingfrom aenum import StrEnumtyping_extensionsfrom contextlib import contextmanagerfrom decimal import localcontextfrom enum import EnumThe assertion never fired on the four missing ones because
siteandtests/_support.pyimport all four long before this file runs — which is exactly theunenforced ordering the fix was supposed to have removed. Measured, with the candidate
purged from
sys.modulesfirst:So the list is now derived by reading
compat.pyrather than by running the test untilit stopped complaining, and the docstring says which four never surface today and why
they are on the list anyway. Warming an already-imported module is a
sys.moduleslookup, so the four cost nothing measurable.
Also recorded: warming
aenumis load-bearing below 3.11 too#687 reads as a
>= 3.11defect, on the grounds that below 3.11compat.py's realbranch is itself
from aenum import StrEnumand so warms the cache honestly. That istrue of this test file —
setUpexecutes the real module before every test — and thefile passed on 3.10 before #693. It is not true of the hazard. Faking the version
around aenum's first ever import in a fresh CPython 3.10.21 process does not quietly
poison a cache; it takes the import down:
from aenum's own fallback definition of
FlagBoundary. So the only thing protecting3.10 is an ordering inside
setUp. That is now in the docstring rather than only in acomment thread.
Verification
pytest -q tests/utilities/test_compat.py tests/cli/test_main.py tests/project/test_module_isolation.py17 passedpython -m unittest tests.utilities.test_compat tests.project.test_public_apiRan 15 tests,OKpytest -p no:randomly --noconftest -qover the same two files15 passed, 432 subtests passed[]on all fouraddCleanupLIFO, same four interpreterscoverage reportover #693's five-file selection, againstf0999858ediffexit 0, byte-identical, 40 tests / 566 subtestsExit codes were read from files rather than from a pipeline. No changelog entry, for
#686's and #693's reason: every entry in
docs/source/changelog/1.5.0.rstcorrespondsto a
pcapkit/change and none is made here.Refs #687.