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.
@JarryShaw JarryShaw added this to the 1.5 milestone Oct 6, 2026
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

Status: Done

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