Skip to content

test: re-import pcapkit only where isolation needs it (#1065) - #1070

Merged
JarryShaw merged 1 commit into
mainfrom
test/1065-purge-only-where-needed
Oct 6, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
test/1065-purge-only-where-needed

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 6, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

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

Description of your pull request and other information

Closes #1065. Closes #1062.

  • 204 setUp purges become tests._support.reimport_once_per_class(self). Each class gets one private pcapkit import, which its own tests share. A setUpClass purge would not work, because the conftest restore swaps the shared session import back in before every test. Plain unittest still leaves the class's import behind, so test: three modules pass alone but fail together, and pytest reports it green #981's reproduction (test_http_unit then test_base_class_contract on 32bcfba15^) still gives 1 failure and 3 errors.
  • Fixed state that tests leaked to later tests in the same class or module: pcapng option tables, the zero-pad ledger, leftover schema subclasses, an uncollected class, Extractor registries, a scapy-less import. ConstEnumRegisterFallbackTests and TCPHTTPDispatchTests keep a per-test purge.
  • tests: audit the suite for redundant cases that add no unique coverage #1062 trims: removed two duplicate tests, the pcapng sweep now runs once instead of four times, and the AppType sweep is sampled (37,173 to 882 subtests, the same 4 arc sets). The const: make EnumRegistry.get's NO_DEFAULT a sentinel object, not -1 #857 -1 test now discriminates again.
  • Times, one module per process under pytest: the 129 affected modules took 2,156 s before and 483 s after. They also pass under plain unittest, in CI-style -n 4 directory groups, all unit modules in one process (forward), reversed in four chunks, and the PyPCAPFile engine leg on 3.10.

@JarryShaw JarryShaw added test Pull requests that add or correct tests (test: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

NEEDS CHANGES on 1d986fd9f: CI shows the cross-test leakage the per-test purge used to hide. main is green on these same tests.

  • Python 3.14 (job 112112341217): 4 failed, 2604 passed in 54 s, against about 16 min on main.
    • test_option_roundtrip_unit — the pcapng-option/opt_comment_1 round trip parses as PARSE, not OK.
    • test_protocol_code_registration_unit — the backward-compatible dispatch table holds a resolved IPv4 class, not a ModuleDescriptor.
    • test_protocol_base_unit — Raw is not found in (Raw, 'RAW'): two different Raw classes, one from each import of pcapkit.
    • interface/test_core — test_every_shipped_engine_has_a_constant sees an extra engine.
  • Engines PyPCAPFile 3.10 (job 112112341590): 3 failures in test_pypcapfile_engine.py.
    • 'types.SimpleNamespace' object is not subscriptable.
    • ipv4_reassembly was called 0 times where the test expects once.
  • All five Python legs (3.10–3.14) fail, so Required checks passed fails too.

These only show up when many modules share one process. The one-module-per-process runs in the PR description could not see them. The fix has to hold under xdist with the full unit selection.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 1d986fd9f: NEEDS CHANGES (ran on Sonnet; author Opus). This is in addition to the CI failures above.

  • Root cause: under pytest the conftest fixture restores module bindings before every test, so the new setUpClass purge has no effect. Registry contents, __proto__ tables and engine tables are not restored. Any test that mutates them without cleanup now leaks into later tests in the same worker.
  • Polluters bisected (candidate plus victim in one process):
    • foundation/test_extraction.py leaves an engine registered in Extractor.__engine__. That breaks interface/test_core. I reproduced this on a clean checkout.
    • 13 modules that dispatch through IPv4 resolve its lazy __proto__ entry. That breaks test_protocol_code_registration_unit.
    • test_dispatch_default_resolution_unit leads to the Raw mismatch between import generations.
    • PyPCAPFile on 3.10 is a stale package attribute seen by pre-3.12 mock target lookup.
  • Two more order-dependences, which show up only when tests run in reverse:
    • foundation/registry/test_protocols.py leaves UNITPROTOCOL registered.
    • test_layer_placement_unit sees OSPF/RARP already resolved.
  • Holds:

@JarryShaw
JarryShaw force-pushed the test/1065-purge-only-where-needed branch from 1d986fd to 2efb76d Compare October 6, 2026 06:31
@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 88.74% (unit tier, Python 3.14, ae3ceb7ad, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18757 1035 2342 846 90.23%
pcapkit/corekit 1874 91 578 22 94.33%
pcapkit/dumpkit 136 0 40 0 100.00%
pcapkit/foundation 2422 143 842 34 92.62%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 15653 187 3942 162 98.19%
pcapkit/toolkit 487 71 144 3 84.15%
pcapkit/utilities 429 4 122 4 98.55%
pcapkit/vendor 4409 2359 1006 158 42.84%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 2efb76dbb: NEEDS CHANGES (one small fix) (ran on Sonnet; author Opus)

  • The one regression. In tests/foundation/registry/test_protocols.py, test_register_protocol_validates_and_updates_registry (:410) and the test near :461 register UNITPROTOCOL without _guard_registry.
    • Under the per-class import, test_register_protocol_stays_quiet_when_nothing_is_displaced then sees an overwrite warning.
    • I reproduced it on a clean checkout of this head: SUBFAILED(case='fresh-key').
    • The same order passes on main. CI's order happens to hide it.
    • Fix: self._guard_registry(registry.protocol_registry, 'UNITPROTOCOL') in both tests.
  • Everything else holds:
    • test: three modules pass alone but fail together, and pytest reports it green #981 is still caught: 1 failure and 3 errors on 32bcfba15^, in 2.1 s instead of 72 s.
    • All 11 run_unittest_leg.py legs pass.
    • The 170 CI unit modules pass under xdist -n 4 --dist load, in 7 chunks. Last round's polluter/victim mix passes at -n 3 and -n 4.
    • 119 changed modules pass in reversed order, apart from the one above.
    • tests/const reversed: peak memory 412 MB in 89 s, against 488 MB in 271 s on main.
    • No assertion is weakened.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 6, 2026
- 204 classes purged pcapkit in setUp, re-importing it before every test
  (~85% of serial suite time). They now call the new
  tests._support.reimport_once_per_class(self): the first test of a class
  purges, the class keeps what it imports, and later tests get that
  import back. A setUpClass purge would not do: under pytest the conftest
  restore swaps the session pin back before every test, so classes would
  share and mutate one import.
- Fix state that tests left behind for others in the same class or
  module: pcapng option registry restored by copy, zero-pad ledger, stale
  schema subclasses, an uncollected test class, unrestored Extractor
  registries, a scapy-less toolkit import. ConstEnumRegisterFallbackTests
  and TCPHTTPDispatchTests keep a per-test purge.
- #1062 trims: drop the duplicate ipv6_ext and Command.get tests, run the
  pcapng truncation sweep once per class, sample the AppType dunder sweep
  (37,173 -> 882 subtests, same arc sets), and make the #857 -1 default
  test discriminate again on a throwaway registry.

129 affected modules: 2,156 s -> 483 s, one per process under pytest; all
pass alone under pytest and plain unittest, in CI-style xdist directory
groups, and all unit modules in one process; #981 still fails 1 + 3.
@JarryShaw
JarryShaw force-pushed the test/1065-purge-only-where-needed branch from 2efb76d to ae3ceb7 Compare October 6, 2026 07:17
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on ae3ceb7ad: GOOD TO GO (ran on Sonnet; author Opus). Confirmed by this head's own CI: 72 checks pass, including Required checks passed.

  • The UNITPROTOCOL leak is fixed. The only change since 2efb76dbb is two _guard_registry(registry.protocol_registry, 'UNITPROTOCOL') lines, which I confirmed with git diff. The previously failing pair now passes on a clean checkout.
  • Reversed-order runs: each of the 129 changed modules passes run alone in reverse order, under both pytest and plain unittest (258 runs).
  • Carried over from the 2efb76dbb review:
  • Speed: the 129 changed modules take 483 s, against 2,156 s before. test_http_unit takes 1.07 s, against 77 s on main.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 6, 2026
@JarryShaw
JarryShaw merged commit 7be2b5c into main Oct 6, 2026
76 of 78 checks passed
@JarryShaw
JarryShaw deleted the test/1065-purge-only-where-needed branch October 6, 2026 13:20
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 6, 2026
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

Status: Done

1 participant