Skip to content

test(vendor): re-resolve VendorRuntimeWarning fresh per generation (#985) - #986

Merged
JarryShaw merged 2 commits into
mainfrom
fix/985-vendor-warning-generation
Oct 2, 2026
Merged

JarryShaw merged 2 commits into
mainfrom
fix/985-vendor-warning-generation

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • test — tests only

Description

Fixes #985. test_vendor_snapshot_restore_unit.py bound VendorRuntimeWarning at module level —
whatever generation was live at import time. Under plain unittest over tests/vendor (no
conftest), a sibling module purges+reimports pcapkit mid-run, minting a new class generation
(same skew as #981). assertWarnsRegex then compares against the stale generation while the
crawler raises the current one, so it reports "not triggered" though the warning is on stderr.

Fix: resolve VendorRuntimeWarning in setUp() via importlib.import_module, alongside the
vendor_main/Vendor pair setUp already re-resolves per test — same pattern as
RegistrationGateTests.setUp() in tests/test_base_class_contract.py (#984).

Red: unittest discover -s tests/vendor → AssertionError: VendorRuntimeWarning not triggered.
Green: same command → Ran 118 tests in 82.933s / OK. Alone: Ran 4 tests in 0.774s / OK.

)

tests/vendor/test_vendor_snapshot_restore_unit.py bound VendorRuntimeWarning
at module level, captured at import/collection time. Under plain unittest
over tests/vendor (no conftest, no restore_module_table fixture), a sibling
module purges pcapkit mid-run and re-imports it, minting a new generation of
every pcapkit class -- the same skew #981 pins. The crawler under test then
raises a different-generation VendorRuntimeWarning than the one
assertWarnsRegex was still holding, so the assertion reports "not triggered"
even though the warning is visibly on stderr one line earlier.

- Drop the module-level `from pcapkit.utilities.warnings import
  VendorRuntimeWarning`.
- Resolve it fresh in setUp() via importlib.import_module, alongside the
  vendor_main/Vendor pair that setUp already re-resolves per-test -- the same
  pattern test_base_class_contract.py's RegistrationGateTests.setUp() uses
  for #981.
- Use self.VendorRuntimeWarning at the one call site.

Red: `python -m unittest discover -s tests/vendor` (plain unittest, whole
directory) failed with `AssertionError: VendorRuntimeWarning not triggered`
on test_a_failure_leaves_the_previous_file_byte_for_byte_intact.
Green: same command, Ran 118 tests in ~83s, OK. The module alone still
passes: Ran 4 tests in 0.774s, OK.
@JarryShaw JarryShaw added test Pull requests that add or correct tests (test: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 2, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO at f03f1d551 — opus cross-review. One prose correction going in on top, and it corrects a
claim I made as well as the docstring's.

assertWarnsRegex does not match by class identity. The docstring says it does; I checked the stdlib
this venv runs and the rule is isinstance(w, self.expected) — subclass semantics. The conclusion
survives for a reason worth stating precisely: the two generations are mutually unrelated, issubclass
false in both directions, so an isinstance test against the stale class rejects an instance of the
fresh one. Same outcome an identity check would give, different mechanism — and a reader who believes it is
identity will mis-predict what a genuine subclass does. A worker is fixing that one sentence; everything
else in the docstring was independently checked and is true.

The fix is complete, and it was the only instance in the directory. git grep -nE "^(from|import) pcapkit" origin/main -- tests/vendor/ returns exactly one line across all 14 modules — this file's.
So the fix makes it converge on its 13 siblings rather than patch one of three. No setUpClass, no
tearDownClass, no class-body state, so nothing is left to reintroduce the skew.

Red and green reproduced independently, Ran 118 tests in 81.159s ... FAILED (failures=1) → 81.348s ... OK, with the same single named failure.

The order dependence is now measured rather than assumed, and it is tighter than I had framed it:

order on main result
…snapshot_restore_unit alone Ran 4 … OK
test_vendor_dest_path_unit then …snapshot_restore_unit Ran 8 … FAILED
…snapshot_restore_unit then test_vendor_dest_path_unit Ran 8 … OK

The binding is captured at import; the assertion fails iff a purging sibling's test runs first.
…snapshot_restore_unit sorts last of the 14, behind all 8 purgers, so discover is always red —
which is exactly why "it passes alone" was never evidence of health. Eight modules, ten purge sites.

A correction to my own brief: I wrote that purge_modules "must be ('pcapkit',)". All ten real call
sites use ['pcapkit'], a list, which is an equally valid Iterable[str]. The trap is only the bare
string. I was right about the mechanism and wrong about this repo's spelling.

On whether this should wait for the broader fix: it should not, and the argument is better than "it is
small".
Moving reconciliation into runner-neutral tests/_support.py would mask the skew — the test
would still bind a stale class and still depend on something external restoring the module table. This
removes the coupling, so it holds under any runner, including one nobody has written. The broader change
still stands on its own merits for tests/corekit/test_sentinels_housing_unit.py (9 module-level bindings),
tests/test_base_class_contract.py (7) and tests/corekit/test_sentinel_exports_unit.py (5), which cannot
sensibly be fixed one binding at a time.

One residual it flagged, non-blocking: run() warns with the class bound in
pcapkit/vendor/__main__.py's namespace while setUp resolves from pcapkit.utilities.warnings. They
agree only because purge_modules(['pcapkit']) drops the subtree atomically. self.vendor_main. VendorRuntimeWarning would be strictly tighter and two lines shorter. Nothing in the repo purges a
narrower pcapkit.utilities prefix today, so I am not asking for it.

UNVERIFIED: pytest on this branch (deliberately — the conftest fixture masks this class, so pytest is not a
valid instrument); the full suite; and whether tests/corekit's 5 tests are affected, which this diff does
not touch.

…ing (#985)

The setUp docstring added for GitHub issue #985 explained the generation skew by
saying assertWarnsRegex "matches by class identity, not by name". unittest's
_AssertWarnsContext.__exit__ actually filters with `isinstance(w, self.expected)`,
so subclass semantics apply and a genuine subclass of the expected class matches.

The docstring's conclusion still holds, for the reason the rewrite now gives:
re-importing pcapkit re-mints VendorRuntimeWarning *and* its BaseWarning base, so
the two generations are mutually unrelated -- issubclass is false in both
directions and their MROs first converge on the builtin UserWarning. isinstance
against a stale binding therefore rejects an instance of the fresh generation.
Both carry the same __module__ and __name__, which is why the failure message
names exactly the class that was raised.

Prose only -- no code, assertion or import changed. Measured on CPython 3.14.7:
issubclass false both ways, a subclass of the live generation passes
assertWarnsRegex, an instance of the other generation fails "VendorRuntimeWarning
not triggered". `python -m unittest discover -s tests/vendor`: Ran 118 tests, OK.
@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 2, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO carries to d47a6dcb9, and I can say that with more confidence than usual: the change is
provably prose-only.
Tokenised both heads excluding comments and docstrings — 889 non-docstring tokens
before, 889 after, sequence identical.
No code, assertion or import moved, so the verdict on f03f1d551
transfers rather than needing a fresh round. Labelled review: good-to-go. tests/vendor 118 passed / 0
failures at this head.

The correction landed, and the worker added a fact better than the one I gave it. I had said
assertWarnsRegex uses isinstance, so a genuine subclass would match — true, and it asked the obvious
next question: why does that not rescue this case? Because re-importing re-mints the base class too. My
own measurement:

g1 is g2: False
issubclass(g2,g1): False | issubclass(g1,g2): False
g1 base: BaseWarning 93949313565344
g2 base: BaseWarning 93949328592240   <- re-minted, not shared
first common ancestor: UserWarning    <- the builtin
same __module__/__name__: True

So the two generations are not related classes at all; their MROs first converge on the builtin
UserWarning. isinstance against a stale binding therefore rejects an instance of the fresh generation —
and because both carry the identical __module__ and __name__, the failure message names exactly the
class that was raised
, which is why it reads as self-contradictory. The docstring now says all of that.
It also kept the original's "not by name" clause, correctly: a name-based match would have succeeded, so
that half was true and load-bearing.

One honest reservation. The setUp docstring is now 39 lines, up from 29, for a two-line fix. I am
keeping it, because every added fact is one I had to measure rather than read, and the file's own module
docstring runs to 165 lines so this is consistent with its house style rather than an outlier. But it is at
the limit, and if a later round wants it trimmed I would not defend the length — only the content.

No invented correction on the tuple point, which is right: the docstring never claimed a tuple was
required, so there was nothing to fix. It independently confirmed all ten purge_modules call sites under
tests/vendor/ spell it purge_modules(['pcapkit']).

One environmental note it flagged, worth recording because it will recur: this branch is checked out in
another agent's worktree, so it could not create the branch locally and pushed from a detached HEAD via
HEAD:refs/heads/…. That other worktree is now one commit behind the remote; it correctly did not touch it.

UNVERIFIED by it: that under discover this module is imported before any sibling purges, and that #981
pins the same skew in RegistrationGateTests — both left as written on my instruction, and both were
confirmed by the earlier round. No Sphinx render, no linters, no pytest (it masks this class).

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.

test(vendor): a stale warning-class generation makes the snapshot-restore assertion order-dependent

1 participant