Skip to content

fix(tests): complete the warm-up list #693 left resting on someone else's imports - #698

Open
JarryShaw wants to merge 1 commit into
mainfrom
fix/compat-warm-list-completeness
Open

JarryShaw wants to merge 1 commit into
mainfrom
fix/compat-warm-list-completeness

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort)
  • make test passes, and a test case covers the change
  • Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md, if the change is user-visible

What is the purpose of your pull request?

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • chore — anything else

The lint targets take pcapkit as their only argument and no pcapkit/ file changes
here, 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_FAKING held the five modules that were observed to appear in the
window, not the ones pcapkit/utilities/compat.py actually imports there.
Reading
the guards off that file, every import reachable at (3, 5) is:

line import on the old list?
34 from collections.abc import Container, Iterable, Sized no
63 import pathlib — unconditional, the <= (3, 4) branch not being taken no
67 from threading import RLock yes
4, 68, 126 typing yes
143 from aenum import StrEnum yes
148, 202 typing_extensions yes
153 from contextlib import contextmanager no
154 from decimal import localcontext yes
179 from enum import Enum no

The assertion never fired on the four missing ones because site and
tests/_support.py import all four long before this file runs — which is exactly the
unenforced ordering the fix was supposed to have removed. Measured, with the candidate
purged from sys.modules first:

warmed ('aenum','decimal','threading','typing','typing_extensions'), pathlib cold
  → delta ['pathlib', 'pathlib._os']                                      NON-EMPTY

warmed all nine, with pathlib/decimal/threading/typing_extensions/aenum cold
  → delta []                                                              EMPTY

So the list is now derived by reading compat.py rather than by running the test until
it 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.modules
lookup, so the four cost nothing measurable.

Also recorded: warming aenum is load-bearing below 3.11 too

#687 reads as a >= 3.11 defect, on the grounds that below 3.11 compat.py's real
branch is itself from aenum import StrEnum and so warms the cache honestly. That is
true of this test file — setUp executes the real module before every test — and the
file 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:

ImportError: cannot import name 'FlagBoundary' from 'enum'
AttributeError: 'FlagBoundary' object has no attribute '__set_name__'

from aenum's own fallback definition of FlagBoundary. So the only thing protecting
3.10 is an ordering inside setUp. That is now in the docstring rather than only in a
comment thread.

Verification

what result
pytest -q tests/utilities/test_compat.py tests/cli/test_main.py tests/project/test_module_isolation.py rc=0, 17 passed
python -m unittest tests.utilities.test_compat tests.project.test_public_api rc=0, Ran 15 tests, OK
pytest -p no:randomly --noconftest -q over the same two files rc=0, 15 passed, 432 subtests passed
warm-up window, CPython 3.10.21 / 3.11.15 / 3.12.13 / 3.14.7 delta [] on all four
addCleanup LIFO, same four interpreters holds on all four
coverage report over #693's five-file selection, against f0999858e diff exit 0, byte-identical, 40 tests / 566 subtests

Exit 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.rst corresponds
to a pcapkit/ change and none is made here.

Refs #687.

@JarryShaw JarryShaw added the test Pull requests that add or correct tests (test: subject prefix) label Sep 23, 2026
@JarryShaw
JarryShaw force-pushed the fix/compat-warm-list-completeness branch from 74c36fa to 8168dee Compare September 23, 2026 04:04
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO — cross-review verdict (Sonnet), two non-blocking imprecisions now fixed

This was raised by an agent, so it got an independent cross-review on a different
model, briefed to falsify: a verdict per load-bearing claim, each with evidence the
reviewer obtained itself. It ran read-only and reported back rather than editing
anything.

Verdict: GOOD TO GO. Both of its non-blocking findings were about docstring prose
and are fixed in 8168dee61, which replaces 74c36fa92.

Confirmed independently:

  • The line-by-line table of imports reachable at (3, 5) is complete and accurate.
    It wrote its own AST walker that evaluates each sys.version_info guard against
    (3, 5), skips if TYPE_CHECKING: bodies and function/class bodies, and lists the
    reachable import statements. Output matched the table line for line, with the
    <= (3, 4) pathlib2 branch and every newer-interpreter else correctly excluded.
    It also did the trace by hand first, and the two agreed.
  • The leak and the fix, with its own probe rather than this one: old five-name list
    with pathlib cold → ['pathlib', 'pathlib._os']; new nine-name list with five of
    the nine cold → []. It then went further than this pull request did and forced
    all nine cold on 3.10.21, 3.11.15, 3.12.13 and 3.14.7 — empty window on all four.
  • The four extra names are free and cannot interact with _reset_abc_caches. Read
    tests/_support.py:389-413 and :802-836: that function is reached only from
    purge_modules/restore_modules, never from importlib.import_module, so there is
    no code path connecting warming collections.abc to it. Tuple order is irrelevant —
    four orderings (alphabetical, reverse, aenum-last, pathlib-first) all give an empty
    window.
  • The CPython 3.10 claim, both halves, on a fresh 3.10.21 venv carrying only
    aenum 3.1.17 and typing-extensions. Faking (3, 5) around aenum's first import
    reproduces the chain exactly — ImportError: cannot import name 'FlagBoundary' from 'enum' at aenum/_enum.py:2869, caught internally, then AttributeError: 'FlagBoundary' object has no attribute '__set_name__' at aenum/_enum.py:1640 while
    aenum builds its own fallback class, traced through __context__. It also confirmed
    the fake is the cause rather than 3.10 itself (a real import aenum on 3.10.21
    succeeds with pyver == (3, 10)), and the weaker sub-claim that loading compat.py
    unfaked on 3.10.21 does from aenum import StrEnum honestly, landing
    aenum.pyver == (3, 10) before any fake exists.
  • Nothing else changed: one file, 27/1, no pcapkit/ file, and the only
    non-comment line changed is the tuple itself.
  • The file still catches the original defect. Without editing the tree, it exec'd
    this module's own source into a scratch namespace, overrode WARM_BEFORE_FAKING to
    () afterwards (the functions close over the module dict, so it takes effect) and ran
    it through unittest in its own subprocess: FAILED (failures=5), the leaked
    assertion firing first and every subsequent test's poisoning cleanup after it.

The two findings, both about the docstring, both reproduced here before acting:

  • It said site and tests/_support.py import the four "long before this file runs".
    The site half is an artefact of this checkout being an editable install — the
    reviewer traced it through the editable finder with -X importtime. Verified here:
    python -S -c shows none of the four in sys.modules, so the runtime is not what
    keeps them warm.
  • tests/_support.py imports collections.abc, contextlib and pathlib at module
    scope (lines 4, 5, 9) but has no import enum; enum is warm because
    test_compat.py itself imports it at line 54. The old sentence overstated the
    attribution.

The docstring now names the three from tests._support and the one from this module,
and records the python -S measurement — which makes the point sharper rather than
weaker: the reason the assertion never fired is that one file's correctness was resting
on another file's import list.

It took the most interesting objection available seriously and could not make it stick:
adding names to the allowlist cannot mask a poisoning defect, because the warm-up loop
runs strictly before the fake is installed, so anything on the list is warmed under
the true version by construction. A tenth reachable import appearing in compat.py
tomorrow still fails the assertion.

One thing it did not verify, stated rather than glossed: the byte-identical coverage report claim, which it deprioritised as out of the listed scope. That one was measured
here — diff exit 0 against f0999858e, 40 tests, 566 subtests.

…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
@JarryShaw
JarryShaw force-pushed the fix/compat-warm-list-completeness branch from 8168dee to e686906 Compare September 23, 2026 13:30

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test Pull requests that add or correct tests (test: subject prefix)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tests/utilities/test_compat.py poisons aenum._common.pyver for the whole process, breaking a later import pcapkit

1 participant