tests: refuse a generated sample capture from the unit tier by JarryShaw · Pull Request #393 · JarryShaw/PyPCAPKit

JarryShaw · 2026-09-15T07:44:17Z

Closes the recurring trap you asked to have enforced with a guard.

The bug it prevents

Three PRs so far — #372, #384 and one earlier — shipped a unit-tier test that read a generated capture. Each passed locally, because the developer had already run make samples, and each failed in CI on a fresh checkout with a bare FileNotFoundError. That is the worst shape of failure available: the error blames a missing file rather than the tier rule that was actually broken, on a test that looks entirely correct.

Mechanism

The guard decides on the calling module's tier, never on whether the file happens to be on disk. That is the property that matters — it fires on the machine that made the mistake rather than on the next fresh clone. Two layers, because each covers the other's blind spot:

layer	where	catches	blind spot
collection time	`tests/conftest.py`	literal `sample_path('x.pcap')` in any unit-tier module, even if the test never runs	computed names
call time	inside `sample_path()`	computed names, e.g. `sample_path(name)` over a list	only when reached

A test skipped for a missing optional engine would hide a violation indefinitely, which is why the static pass exists; a name built at runtime is invisible to any static pass, which is why the runtime one does.

Committedness is asked of git, not hardcoded — and writing the list by hand would have been wrong immediately. examples/captures/ has six tracked files, not the two captures you'd assume: out.json, out.plist, out.txt and pcapng.txt are tracked as well. A hand-written list would also rot the moment a third capture is committed.

The opt-out is an idiom the suite already uses

GeneratedFixtureInUnitTierError subclasses FileNotFoundError, so anything already handling an absent capture keeps working unchanged — a backstop even if the detection ever misses a call site. A sample_path() call inside a try whose handler catches a missing file is tier-safe by construction and is left alone, exactly as tests/toolkit/test_dpkt_unit.py:438 and :693 already do:

try:
    path = sample_path('test.pcap')
except FileNotFoundError as exc:
    self.skipTest(str(exc))

So this needed no edits to existing tests and loses no coverage — those two still run when the fixtures are present.

Without git — an unpacked sdist, say — the guard warns once and no-ops, rather than failing a run that is probably fine.

The message teaches rather than just refusing

git does not track examples/captures/http.pcap -- it is one of the fixtures
'python examples/generators/make_samples.py' (equivalently `make samples`) writes
on demand, and a fresh clone does not have it.

The unit tier has to pass on a fresh clone with nothing but `pip install -e '.[test]'`
...
Two ways to fix it:
  1. read a committed capture instead -- git tracks dhcp.pcapng, in.pcap; or
  2. move the test into a fixture-dependent tier ... rename the module to
     *_runtime.py (or *_regression.py), or put it under tests/integration/.

Verification

check	result
planted unit-tier read, fixture absent	exit 4, aborted at collection
same, fixture present on disk (171 KB)	exit 4 — the property that matters
computed-name form	exit 1, runtime layer
real unit tier, fixtures present	478 passed, exit 0, no guard output
real unit tier, fixtures absent	475 passed, 3 skipped, exit 0, no guard output
fixture-dependent tiers	`tests/integration` + a `_runtime.py`: 74 passed, 4 skipped*, exit 0
no git at all	exit 0, one `TierGuardWarning`
guard's own tests	24 passed on 3.14 and on 3.10 (the CI floor)

The 3 skips with fixtures absent are pre-existing and unchanged — the two dpkt ones still skip with their original message, not the guard's.

Known gap, documented in the module

A test that builds the path by hand instead of calling sample_path() is invisible to both layers, as tests/protocols/misc/test_pcapng_unit.py:2924 does. It is tier-safe there (checks isfile, then skips), and the heuristic needed to catch that shape safely would risk failing correct code across the suite, so it is deliberately left out rather than half-done.

One design decision worth your call

A violation aborts the whole session (pytest.UsageError, exit 4) rather than failing just the offending test. The reasoning is that a tier violation is a hygiene error, not a test failure, and it should be impossible to skim past. The cost is that one bad file blocks running any tests until it is fixed. Happy to change it to failing only the offending items if you'd rather it were gentler.

Three PRs so far (#372, #384, and one before) shipped a unit-tier test that read a *generated* capture. Each passed locally, because the developer had already run `make samples`, and each failed in CI on a fresh checkout with a bare FileNotFoundError -- an error that blames a missing file rather than the tier rule that was broken, on a test that looks perfectly correct. The guard decides on the calling module's **tier**, never on whether the file happens to be present, so it fires on the machine that made the mistake instead of on the next fresh clone. Two layers, because each covers the other's blind spot: * collection time, in tests/conftest.py: every unit-tier module is AST-parsed and a `sample_path('literal')` naming an untracked capture aborts the session. This sees violations in tests that never execute -- one skipped for a missing optional engine would otherwise hide indefinitely. * call time, inside sample_path(): the caller's module is read out of the frame, so no test declares its own tier and none can declare it wrongly. This catches computed names, e.g. `sample_path(name)` over a list, which no static pass can. Committedness is asked of git (`git ls-files`), not hardcoded. Writing the list by hand would have been wrong immediately: examples/captures/ has **six** tracked files, not the two captures one assumes -- out.json, out.plist, out.txt and pcapng.txt are tracked too -- and it would rot the moment a third capture lands. GeneratedFixtureInUnitTierError subclasses FileNotFoundError so that anything already handling an absent capture keeps working unchanged. That is also the documented opt-out, and it is the idiom the suite already uses at tests/toolkit/test_dpkt_unit.py:438 and :693: a `sample_path()` call inside a `try` whose handler catches a missing file is tier-safe by construction and is left alone. So this needed no edits to existing tests and loses no coverage -- those two still run when the fixtures are there. Without git -- an unpacked sdist, say -- the guard warns once and no-ops rather than failing a run that is probably fine. Verified: a planted unit-tier read of a generated capture exits 4 with the fixture absent *and* with it present on disk; the computed-name form exits 1. The real unit tier is silent -- 478 passed with fixtures present, 475 passed and 3 skipped without, both exit 0, no guard output either way. Fixture-dependent tiers are untouched: tests/integration plus a *_runtime.py, 74 passed, 4 skipped. The guard's own 24 tests pass on 3.14 and on 3.10, the CI floor. Known gap, documented in the module: a test that builds the path by hand instead of calling sample_path() is invisible to both layers, as tests/protocols/misc/test_pcapng_unit.py:2924 does. It is tier-safe there (it checks isfile and skips), and the heuristic needed to catch that shape safely would risk failing correct code, so it is deliberately left out.

Copilot

🟡 Changes recommended

tests/_tiers.py::sample_path_calls() documents “source order” but uses ast.walk() (non-ordered), leading to potentially nondeterministic violation output and an inaccurate contract.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds an enforced “tier guard” to prevent unit-tier tests from depending on generated sample captures (which causes local-pass / CI-fail FileNotFoundError traps on fresh checkouts). The guard is enforced at pytest collection time (static scan of collected unit-tier modules) and at runtime via tests._support.sample_path() (to catch computed capture names).

Changes:

Add tests/_tiers.py implementing tier classification, git-tracked capture detection, static auditing of sample_path('literal') calls, and runtime checking.
Add tests/conftest.py to abort the session at collection time (pytest.UsageError) when a unit-tier module reads a generated capture.
Update tests/_support.py::sample_path() to consult the runtime guard and raise a FileNotFoundError-subclass (GeneratedFixtureInUnitTierError) on violations.
Add tests/test_tier_guard.py to test tier classification, git-based committedness, auditing behavior, runtime checks, and degradation when git is unavailable.

File summaries

File	Description
tests/test_tier_guard.py	New unit tests validating the guard’s classification, git integration, auditing, and runtime behavior.
tests/conftest.py	New pytest collection hook enforcing the static audit and failing fast on violations.
tests/_tiers.py	New tier-guard implementation (tier classification, git-tracked capture discovery, static + runtime checks, messages).
tests/_support.py	Routes `sample_path()` through the runtime guard and reuses shared constants from `tests._tiers`.

Review details

Files reviewed: 4/4 changed files
Comments generated: 1
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…reflight Upstream `pypcap` is unusable on a current Python: 1.3.0 compiles a `pcap.c` pre-generated by Cython 0.29.32, which does not build against the 3.12+ C API, and it publishes no wheels. Measured with libpcap present and found: builds on 3.10 and 3.11, fails on 3.12 (`ob_digit`, `curexc_traceback`) and 3.14 (those plus `ma_version_tag` and `_PyLong_AsByteArray` arity). `pcap-ct` is a ctypes reimplementation that provides the same top-level `pcap` module and works across the whole supported range. It is added as its own engine rather than a second backend behind `pypcap`, because the two are **mutually exclusive** -- both distributions own the import name `pcap`, and with both installed pcap-ct wins while upstream's extension module is shadowed and unreachable. One engine silently meaning two different implementations is exactly the ambiguity worth avoiding. New shared, stdlib-only `_pcap_backend.py` is the single place that answers "which backend did I get": `hasattr(pcap, '_pcap')` distinguishes them (pcap-ct is a package, upstream a lone extension module), and `importlib.metadata` reveals the distribution the import did *not* resolve to. Shared deliberately -- two copies would be two chances for the engines to disagree. Each engine exposes `.backend` and warns when both are installed. `unsupported_reason()` now works as a preflight across every engine, which is the case the import test cannot cover: a dependency that installs cleanly and only fails when used. * `pcap`, `pcapng` -- no override; pcapkit's own parsers have no requirement. * `dpkt`, `scapy` -- no override, measured genuinely unconstrained on 3.14. * `pyshark` -- ceiling 3.14 plus a tshark check. It *imports* on 3.14 and fails at use: the call it makes, `asyncio.get_event_loop_policy().get_event_loop()`, returns a loop on 3.10/3.11, warns on 3.12 and raises on 3.14. 3.13 was not available and is inferred, flagged as such in the code. * `pypcap`, `pcap_ct` -- wrong-backend detection; `pcap_ct` also reports a missing system libpcap. * `pypcapfile` -- unchanged from #396. The tshark check delegates to pyshark's own `get_process_path()` rather than `shutil.which`, which is not equivalent: pyshark consults `tshark_path` in its config first, then PATH on POSIX, both Program Files directories on Windows, and `/Applications/Wireshark.app` on macOS, so `which` would refuse working setups. Not cached -- the failing path measures ~287 us against 39 PATH entries and runs once per Extractor, and caching would freeze an answer about the *environment*, so installing Wireshark would not take effect until restart. Three of my own earlier claims that measurement disproved, corrected in the docs rather than left standing: the `libpcap` wheel does **not** remove the need for a system libpcap (its published config sets `LIBPCAP = None`, sending the loader to `find_library("pcap")`); with no libpcap, `import pcap` raises `OSError`, not `ImportError`, so it escapes `import_test` and aborts the extraction rather than degrading; and the PCAP-NG verdict is host-dependent -- libpcap 1.11 reads `dhcp.pcapng` correctly while 1.5.3 silently returns nonsense timestamps, so both engines keep rejecting PCAP-NG, now with the real reason. Fixed while probing: a second probe after a failure reported `NameError: '__about__' is not defined` instead of the real cause, because both `pcap-ct`'s and `libpcap`'s `__init__` open with a non-rerunnable `from .__about__ import * ; del __about__`. `probe()` now purges both module trees on failure. Also moved `test_new_engine_parity.py` to `*_runtime.py`. It read four generated captures from the unit tier -- a tier violation that #393's guard could not catch, because it calls `sample_path(capture)` with a *variable*, which only the runtime layer sees, and that layer is never reached on a host where the engine packages are absent. It was the sole cause of the 5 failures in the pristine baseline below. `examples/legacy_smoke/_engine_support.py` listed only four engines, so the timing harness never exercised the new ones; its skip messages now consult `unsupported_reason()` and name the real cause. No benchmark numbers are published. They were measured, but this host is a shared cloud desktop under load whose figures disagree with the README's existing row in *both* directions, so mixing the two tables row-for-row would be misleading. An engine x Python support matrix is documented instead, which is host-independent: 3.11 is the last version where every engine runs, and even there pypcap and pcap_ct are mutually exclusive, so no single environment has all seven. Verified after rebasing onto 89201df: engine identity asserted rather than inferred (`__engine_name__ == 'PCAP_CT'`, 6 frames, no EngineWarning); `unsupported_reason()` exercised for all eight engines; `files=True` naming checked against #403's bare-`_fext` change, which this engine's per-frame naming depends on. Unit tier 614 passed / 28 skipped / 0 failed, against a pristine-main baseline of 5 failed / 546 passed / 50 skipped.

…reflight (#405) * engines: add the pcap_ct engine, and make unsupported_reason a real preflight Upstream `pypcap` is unusable on a current Python: 1.3.0 compiles a `pcap.c` pre-generated by Cython 0.29.32, which does not build against the 3.12+ C API, and it publishes no wheels. Measured with libpcap present and found: builds on 3.10 and 3.11, fails on 3.12 (`ob_digit`, `curexc_traceback`) and 3.14 (those plus `ma_version_tag` and `_PyLong_AsByteArray` arity). `pcap-ct` is a ctypes reimplementation that provides the same top-level `pcap` module and works across the whole supported range. It is added as its own engine rather than a second backend behind `pypcap`, because the two are **mutually exclusive** -- both distributions own the import name `pcap`, and with both installed pcap-ct wins while upstream's extension module is shadowed and unreachable. One engine silently meaning two different implementations is exactly the ambiguity worth avoiding. New shared, stdlib-only `_pcap_backend.py` is the single place that answers "which backend did I get": `hasattr(pcap, '_pcap')` distinguishes them (pcap-ct is a package, upstream a lone extension module), and `importlib.metadata` reveals the distribution the import did *not* resolve to. Shared deliberately -- two copies would be two chances for the engines to disagree. Each engine exposes `.backend` and warns when both are installed. `unsupported_reason()` now works as a preflight across every engine, which is the case the import test cannot cover: a dependency that installs cleanly and only fails when used. * `pcap`, `pcapng` -- no override; pcapkit's own parsers have no requirement. * `dpkt`, `scapy` -- no override, measured genuinely unconstrained on 3.14. * `pyshark` -- ceiling 3.14 plus a tshark check. It *imports* on 3.14 and fails at use: the call it makes, `asyncio.get_event_loop_policy().get_event_loop()`, returns a loop on 3.10/3.11, warns on 3.12 and raises on 3.14. 3.13 was not available and is inferred, flagged as such in the code. * `pypcap`, `pcap_ct` -- wrong-backend detection; `pcap_ct` also reports a missing system libpcap. * `pypcapfile` -- unchanged from #396. The tshark check delegates to pyshark's own `get_process_path()` rather than `shutil.which`, which is not equivalent: pyshark consults `tshark_path` in its config first, then PATH on POSIX, both Program Files directories on Windows, and `/Applications/Wireshark.app` on macOS, so `which` would refuse working setups. Not cached -- the failing path measures ~287 us against 39 PATH entries and runs once per Extractor, and caching would freeze an answer about the *environment*, so installing Wireshark would not take effect until restart. Three of my own earlier claims that measurement disproved, corrected in the docs rather than left standing: the `libpcap` wheel does **not** remove the need for a system libpcap (its published config sets `LIBPCAP = None`, sending the loader to `find_library("pcap")`); with no libpcap, `import pcap` raises `OSError`, not `ImportError`, so it escapes `import_test` and aborts the extraction rather than degrading; and the PCAP-NG verdict is host-dependent -- libpcap 1.11 reads `dhcp.pcapng` correctly while 1.5.3 silently returns nonsense timestamps, so both engines keep rejecting PCAP-NG, now with the real reason. Fixed while probing: a second probe after a failure reported `NameError: '__about__' is not defined` instead of the real cause, because both `pcap-ct`'s and `libpcap`'s `__init__` open with a non-rerunnable `from .__about__ import * ; del __about__`. `probe()` now purges both module trees on failure. Also moved `test_new_engine_parity.py` to `*_runtime.py`. It read four generated captures from the unit tier -- a tier violation that #393's guard could not catch, because it calls `sample_path(capture)` with a *variable*, which only the runtime layer sees, and that layer is never reached on a host where the engine packages are absent. It was the sole cause of the 5 failures in the pristine baseline below. `examples/legacy_smoke/_engine_support.py` listed only four engines, so the timing harness never exercised the new ones; its skip messages now consult `unsupported_reason()` and name the real cause. No benchmark numbers are published. They were measured, but this host is a shared cloud desktop under load whose figures disagree with the README's existing row in *both* directions, so mixing the two tables row-for-row would be misleading. An engine x Python support matrix is documented instead, which is host-independent: 3.11 is the last version where every engine runs, and even there pypcap and pcap_ct are mutually exclusive, so no single environment has all seven. Verified after rebasing onto 89201df: engine identity asserted rather than inferred (`__engine_name__ == 'PCAP_CT'`, 6 frames, no EngineWarning); `unsupported_reason()` exercised for all eight engines; `files=True` naming checked against #403's bare-`_fext` change, which this engine's per-frame naming depends on. Unit tier 614 passed / 28 skipped / 0 failed, against a pristine-main baseline of 5 failed / 546 passed / 50 skipped. * docs: use auto-numbered footnotes, and keep the README plain-docutils clean The benchmark table's footnotes were hand-numbered `[1]_`..`[4]_`, and referenced out of order -- the table cited 3, 1, 4, 2 -- so any insertion meant renumbering every marker and definition by hand. They are now RST auto-numbered footnotes with labels (`[#pyshark-historical]_` and friends), which is the form already used across `docs/source/pcapkit/const/*.rst` as auto-symbol `[*]_`. Labelled rather than bare `[#]_` on purpose: bare auto-numbers are assigned in reference order, so the definitions would have to stay in the table's order to read correctly, and a reordered row would silently repoint a marker at the wrong note. Labels make the pairing explicit and order-independent. Labelled auto-footnotes do take their *numbers* from definition order, though, so the definitions are also reordered to match the table -- otherwise the markers rendered 3, 1, 4, 2, exactly the wart being removed. Verified: markers now render 1, 2, 3, 4 in both the table and the definition list, in `README.rst` and `docs/source/index.rst`. Also removed three Sphinx-only roles this branch had introduced into `README.rst` -- one `:manpage:` and two `:func:` -- which `main` did not have. GitHub renders the README with plain docutils, which knows none of them and emits a visible `Unknown interpreted text role` error for each, the same defect #400 fixed for the admonitions. They are now ``literals``. README again renders with zero diagnostics under plain docutils. * docs: auto-symbol footnotes in the Sphinx matrix, literal symbols in the README The engine-support matrix hand-wrote its `*` and `†` markers in the cells with loose prose beneath, so nothing linked and the symbols had to be maintained by hand. In `docs/source/index.rst` those are now RST auto-symbol footnotes (`[*]_`), which index themselves. `README.rst` deliberately keeps the literal `*`/`†` characters. GitHub renders it with plain docutils, which does not present indexed footnotes usefully, so the markers there stay plain text -- the same reasoning that keeps admonitions out of that file. One constraint worth recording, because it shaped the layout: **auto-symbol footnotes are strictly one reference per definition.** Two references against one definition is an error (`Too many symbol footnote references: only 1 corresponding footnote available`), and three references mint three *separate* notes rendering `*`, `†`, `‡`. Since `*` appeared in eight cells and `†` in four, a direct conversion would have needed twelve definitions. The markers therefore moved off the individual cells and onto the things they actually qualify -- the `3.13` and `3.15` column headers, and the `pyshark` row label -- which is three references, three definitions, and no loss of precision about which verdicts are inferred. The benchmark table's `[1]_`..`[4]_` footnotes are left numbered as they were. Verified: README renders with zero diagnostics under plain docutils; the Sphinx copy's three auto-symbol footnotes resolve without warnings.

JarryShaw requested a lite review from Copilot September 15, 2026 08:26

Copilot started reviewing on behalf of JarryShaw September 15, 2026 08:27 View session

Copilot AI reviewed Sep 15, 2026

View reviewed changes

Comment thread tests/_tiers.py

JarryShaw merged commit e0f0160 into main Sep 15, 2026
50 checks passed

This was referenced Sep 15, 2026

ci: stop dropping a commit's validation when the next one lands #394

Merged

engines: add the pcap_ct engine, and make unsupported_reason a real preflight #405

Merged

JarryShaw deleted the test/unit-tier-fixture-guard branch September 17, 2026 01:08

JarryShaw added the test Pull requests that add or correct tests (test: subject prefix) label Sep 22, 2026

Navigation Menu

Uh oh!

tests: refuse a generated sample capture from the unit tier - #393

tests: refuse a generated sample capture from the unit tier#393
JarryShaw merged 1 commit into
mainfrom
test/unit-tier-fixture-guard

JarryShaw commented Sep 15, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

JarryShaw commented Sep 15, 2026

The bug it prevents

Mechanism

The opt-out is an idiom the suite already uses

The message teaches rather than just refusing

Verification

Known gap, documented in the module

One design decision worth your call

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants