Skip to content

ci(unit-tests): install DPKT so the dpkt-gated tests actually run - #737

Merged
JarryShaw merged 1 commit into
mainfrom
fix/729-dpkt-gated-tests-never-run
Sep 24, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/729-dpkt-gated-tests-never-run

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

  • fix
  • feat
  • perf
  • refactor
  • test
  • ci
  • chore

Description

Fixes #729. dpkt is declared only in the DPKT/all extras; no unit-tests.yml install line named it (.[test] for test, .[test,Scapy] for integration/gate), so all HAS_RUNTIME and HAS_DPKT-gated tests skipped on every job/version, forever, reading as passes.

AST-measured (still accurate after #725/#731 shifted the issue's line numbers): 28 methods, 6 files — 20 test_dpkt_unit.py, 4 test_misc.py, 1 each test_extraction.py/test_core.py/test_engine_parity.py/test_engine_runtime.py.

Added DPKT to all three install lines, on every existing leg (not one representative leg): dpkt 1.9.8 installs and imports cleanly on 3.10–3.14 (measured, throwaway venvs), it adds no new job/leg — unlike Scapy, which already sets the "extra on every leg" precedent on these same lines — and singling out one leg would be an inconsistency, not a wall-clock saving (#715 is about job count, not per-job extras). HAS_RUNTIME's four packages are core project.dependencies, always installed already, so DPKT alone is sufficient — no separate fix needed there.

test_reassembly_engine_parity.py also imports HAS_DPKT but only branches internal assertions on it; it isn't skipUnless-gated and isn't one of the 28.

Test evidence (throwaway /tmp venvs, repo .venv never touched; pcapkit.__file__ confirmed pointing at this worktree): before (.[test] / .[test,Scapy], no dpkt): 26 passed/28 skipped + 6 passed/3 skipped. After (+DPKT): 62 passed, 1 skipped — the 1 remaining skip is an unrelated pyshark not installed. All 28 previously-skipped dpkt methods now pass.

`dpkt` was declared only in the `DPKT` and `all` extras (pyproject.toml),
and no install line in unit-tests.yml named either -- `.[test]` for the
`test` job, `.[test,Scapy]` for `integration` and `gate`. Every
`@unittest.skipUnless(HAS_RUNTIME and HAS_DPKT, ...)` test therefore
skipped on every job, every Python version, forever, and read as a pass in
the summary line. AST-measured: 28 methods across 6 files, confirmed by
running the affected selection before and after this change.

Add `DPKT` to all three `pip install -e` lines (`test`, `integration`,
`gate`), on every existing matrix leg rather than one representative leg:
`dpkt` measured a clean install and import on 3.10-3.14, it adds no new
job or leg (unlike Scapy, which already sets that precedent on the same
lines), and the `test`/`integration` jobs already run their existing
tiers across the full matrix, so singling out one leg for this extra
would be an inconsistency, not a saving. `HAS_RUNTIME`'s four packages are
core `project.dependencies`, already installed unconditionally, so the
extra alone is sufficient.

Verified with throwaway venvs (dpkt is never installed into the repo
venv): before, the six files report 26 passed / 28 skipped +
6 passed / 3 skipped (28 dpkt-gated + 3 unrelated scapy/pyshark skips);
after, 62 passed / 1 skipped (only the unrelated pyshark skip remains).
@JarryShaw JarryShaw added test Pull requests that add or correct tests (test: subject prefix) ci Pull requests that change CI or workflow configuration (ci: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one and removed test Pull requests that add or correct tests (test: subject prefix) labels Sep 24, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

✅ GOOD TO MERGE @ 28582b25c — re-derived 28/6 independently, measured all 28 now execute, dpkt wheel-installs on all five Pythons, 15 required contexts intact.

@JarryShaw

Copy link
Copy Markdown
Owner Author

✅ GOOD TO MERGE @ 28582b25c — re-derived 28/6 independently, measured all 28 now execute, dpkt wheel-installs on all five Pythons, 15 required contexts intact.

Independent cross-review on a different model from the author. Every figure below is one I obtained myself, not one taken from the PR body.

Claim Verdict Evidence I obtained
28 gated methods across 6 files ✅ exact Own AST pass resolving class-level and method-level skipUnless gates: 20 / 4 / 1 / 1 / 1 / 1 — identical split. The two other HAS_DPKT files (tests/integration/_helpers.py, test_reassembly_engine_parity.py) carry no gate, as the body says
All 28 now execute, dpkt is the sole blocker ✅ 28/28 Those exact 28 node-ids, fixtures regenerated: dpkt hidden → 28 skipped; dpkt visible → 28 passed … 39 subtests passed. Only variable changed was find_spec('dpkt')
Every gated file is reached by a leg that now has DPKT ✅ no gap test ignores tests/integration + *_runtime.py, so it carries 24; fixture_tier_paths() hands integration the other 4 — tests/integration plus two test_dpkt_unit.py node-ids. 24 + 4 = 28. Both changed lines are load-bearing
CI corroborates, zero collateral ✅ Python 3.12 1542→1566 passed / 116→92 skipped (+24 / −24); Integration 3.14 143→147 / 31→27 (+4 / −4). Exactly the partition above and nothing else moved
dpkt installs on 3.10–3.14 ✅ measured, all 5 dpkt-1.9.8-py3-none-any.whl (194 kB, pure Python, no sdist build) on all five test legs and all five Integration legs
15 required contexts survive ✅ name: lines byte-identical to main (line numbers only shifted); all 15 green, rollup SUCCESS, mergeStateStatus: CLEAN, rebased on 55513f69e as one commit
HAS_RUNTIME needs no fix ✅ RUNTIME_DEPS = ('tbtrim','aenum','chardet','dictdumper') — all four in project.dependencies
Template ✅ Four-item checklist, exactly one type (ci), Fixes #729.; both standing exceptions worded correctly
Coverage rule n/a Diff is one file, .github/workflows/unit-tests.yml; no test or library file touched, so the rule does not engage. pytest-cov still absent; I installed nothing anywhere

On the matrix legs I agree with all five, not one. #715 was job count (~130 min/push of duplicated legs); this adds no job, and a 194 kB pure-Python wheel costs about a second per leg. Measured wall-clock stays inside pre-existing variance (test legs 411–578 s against a 559 s baseline and a 45-min timeout). One representative leg would leave 28 tests on single-version coverage while 1566 get five — and pcapkit's dpkt adapter is byte/struct-level work, which is precisely what a version matrix is for. Scapy already sets the every-leg precedent on these same lines.

Caveats and disagreements, none blocking:

  • The gate job's changed line is unexercised by this PR: if: inputs.gate-only == true, so it reported skipped with steps: []. Correct by inspection, but verified by nothing here.
  • The body's headline evidence conflates two variables. "before … 26 passed/28 skipped + 6 passed/3 skipped; after 62 passed, 1 skipped" is a delta of 30 resolved skips, but only 28 are dpkt-gated — the other 2 are scapy-gated tests that resolved because the "after" venv also gained Scapy. Isolating dpkt alone across the same six files gives 35 passed, 28 skipped → 63 passed, 0 skipped, a delta of exactly 28. The conclusion is right; the evidence as written does not isolate it.
  • The new integration comment says "the two dpkt-gated methods living under tests/integration/". That leg actually carries four — the other two being test_dpkt_unit.py::DPKTEngineParityTests::test_tcp_reassembly_matches_the_default_engine and ::DPKTTCPHeaderSplitTests::test_every_tcp_frame_of_the_sample_capture_holds_the_invariants, which fixture_tier_paths() routes there because they need test.pcap. Comment-only, and the code is right.
  • The body drops the docs commit-type box (template lists eight, body seven).
  • CI runs pytest -q with no -r, so skip reasons never print: "0 occurrences of dpkt not installed" would read the same whether or not the fix worked. The skip delta is the only usable signal there.
  • Why this survived so long: Pipfile [dev-packages] already had dpkt = "*", so local make test always ran these 28 and only CI was blind. The fix is at the right layer.

✅ GOOD TO MERGE @ 28582b25c — re-derived 28/6 independently, measured all 28 now execute, dpkt wheel-installs on all five Pythons, 15 required contexts intact.

@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 Sep 24, 2026
@JarryShaw
JarryShaw merged commit 9b2d927 into main Sep 24, 2026
24 checks passed
@JarryShaw
JarryShaw deleted the fix/729-dpkt-gated-tests-never-run branch September 24, 2026 14:38
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Sep 24, 2026
JarryShaw added a commit that referenced this pull request Sep 24, 2026
…aches it

#729 and #738 were one defect twice: a HAS_*-gated suite whose dependency no CI
job installs, skipping silently because `pytest -q` prints no skip reasons. #737
and #740 fixed the install lines; nothing held them there.

- tests/_dependency_gates.py derives per flag: its gates (AST over class- AND
  method-level skipUnless), the pytest-running jobs reaching them (_tiers'
  is_unit_tier / fixture_tier_paths, at node-ID granularity), and whether that
  job's install line carries an extra providing it. Extras come from
  pyproject.toml; only import-name -> distribution is hand-written.
- Seven known gaps carry a reason each in DEPENDENCY_GATE_EXCLUSIONS; an entry
  that outlives its gap fails, since declared must equal derived, both ways.
- New: HAS_RUNTIME in test_runtime_engines.py also wants dpkt/scapy/pyshark, so
  5 methods skip on `test` and `gate`. Absent from #738. Tracked in #751.
- Corrects #745 and #738: HAS_CRAWLER_DEPS is not dark (`test` has carried
  requests and bs4 since #507); HAS_VENDOR_DEPS lacks only html5lib;
  HAS_PYPCAPFILE gates 15 methods, not 10 (#747 grew the class 4 -> 9).

37 -> 84 tests in tests/test_tier_guard.py under plain unittest; 98% branch
coverage of the new module. Deleting `crypto` from the `test` job's install line
in a scratch copy makes the guard fail, naming all 14 ESP gates.

Fixes #745.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Pull requests that change CI or workflow configuration (ci: subject prefix)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tests: dpkt-gated tests have never run on any CI path (28 methods, 6 files)

1 participant