ci: give the pyshark engine real-capture coverage, and stop a fallback passing as a pass - #846
Conversation
JarryShaw
added
ci
|
GOOD TO GO — cross-review (opus, a different model from the sonnet author) on The load-bearing premise held under independent attack. It built the per-job table itself and It did better than I could on the one thing I flagged as unverified. I said the new assertion Better still, CI has now proved it for real. Also confirmed: the The nit worth acting on: Left alone: the local named One residual the reviewer named honestly: |
…k passing as a pass #845's first finding: no CI job anywhere drove `engine='pyshark'` through a real tshark-parsed capture, so the engine's own code path was never exercised on any Python version. - .github/workflows/unit-tests.yml: the `pypcap-parity` job now installs tshark alongside libpcap-dev. It is the only job whose selection can reach tests/integration/test_engine_runtime.py's real `engine='pyshark'` extraction -- `engine-tests` installs both pyshark and tshark but ignores tests/integration and *_runtime.py wholesale. The debconf pre-seed and DEBIAN_FRONTEND mirror `engine-tests`'s own tshark step, or the postinst blocks on the capture-permissions prompt. - tests/integration/test_engine_runtime.py: the pyshark case now asserts no EngineWarning fired, `extractor._exnam == 'pyshark'`, and the engine is a PyShark instance -- following test_new_engine_parity_runtime.py's pattern. Without tshark the engine declines, the extractor silently falls back to the built-in parser, and the existing length assertion passed on the fallback's output: a vacuous pass, invisible in skip counts. Deliberately narrow. #845's other two findings are not addressed here: the PyPCAPFile marker gap is superseded by the maintainer's two-dimension matrix ruling on that issue, and promoting `Engines Python X` to a required check is a ruleset change, not a workflow one. Tests: 106 passed (tests/integration/test_engine_runtime, tests/test_tier_guard). The new assertion sits in the Python < 3.14 branch, which this host's 3.14.7 venv does not take, so it is verified by construction against the existing parity test's identical idiom rather than by local execution. Refs #845.
d953676 to
8d15a90
Compare
|
Label not updated after review? Code LGTM. I'll merge once CI passed. |
JarryShaw
added
review: good-to-go
|
Recorded, verbatim: "Label not updated after review? Code LGTM. I'll merge once CI passed." You are right, and it was mine to flip — corrected to One thing worth stating rather than glossing, since the head moved after the verdict. The Nothing but prose, in a test module's module docstring, fixing the sentence that claimed tshark is If you would rather the label only ever sit on a sha a reviewer actually saw, say so and I will CI on |
JarryShaw
removed
the
review: good-to-go
engine='pyshark' had no CI job driving it through a real tshark-parsed capture: pypcap-parity installed pyshark but not tshark, so PyShark.unsupported_reason() declined the engine, the extractor fell back to the built-in parser, and the one real-extraction assertion passed against the fallback's output -- a vacuous pass invisible in every skip count. pypcap-parity now installs tshark with the same debconf pre-seed engine-tests already carries, and the test proves which engine actually ran rather than trusting the assertion alone.
Adds util/pyshark_encap_map.py, a generator that regenerates ENCAP_TYPE_TO_LINKTYPE (152 entries) and FILTER_NAME_TO_LINKTYPE (58) in place inside pcapkit/toolkit/pyshark.py, so the two tables #850 hand-built stop being hand-maintained. Guards against its own worst failure mode: a naive LinkType(dlt) lookup cannot detect an unmapped DLT, because _missing_ mints a placeholder rather than raising, so the generator snapshots every known value before any lookup. The real-tshark sweep is gated on a new HAS_WIRESHARK flag and is version-pinned -- editcap -T accepts 226 encapsulations on Wireshark 4.6.9, 224 on 4.2.2 (CI's Ubuntu noble) -- so the count assertions run only under the measured version. One line of pyshark.py itself changes as a result: the IPMB_LINUX table entry becomes I2C_LINUX, the canonical name #848 gave value 209. util/changelog_md.py regenerated CHANGELOG.md for all seven entries added across this and the six preceding commits (#838, #846, #847, #848, #849, #850, #853); `--check` exit 0.
#846 and #850 - #838's "the 21 whose vendor crawler leaves Vendor.process() unmodified" measures 22, not 21 -- pcapkit.const.hip.transport.Transport also has an unmodified process(), but its FLAG-bound range has no unassigned gap, so its _missing_ carries no bounded-range extend_enum branch to fix. The real discriminator is that branch, not the process() override; reworded, and the "other 84" split into the 83 that do override process() and the one that doesn't but has nothing to fix either. Re-derived independently against 5e25db2^..5e25db2 (not main, which would fold in #847's own edit to ipx/socket.py): 22 registries have no process() override, 21 of them have the bounded-range branch, matching tests/const/test_const_enum_no_mint.py's own REGISTRIES_WITH_UNASSIGNED_RANGES (21 entries) vs ALL_REGISTRIES (22, +hip.transport, with a NOTE explaining the exclusion). - #846's "Only tests/integration/test_engine_runtime.py and test_engine_parity.py change" is false -- 30bca99's own numstat also touches .github/workflows/unit-tests.yml (17+/2-). Qualified to "only these two test files change". - #850 gets the breaking marker: its own prose already says the fallback is gone and an unrecognised encapsulation or filter name now raises MissingKeyError instead of substituting a plausible DLT -- the same shape as the two existing markers at AppType.get (1.5.0.rst:2456) and the four .get()-backed enum fields (1.5.0.rst:2720). Restructured to lead with the marker and the subject, matching their wording and placement; the later restatement of the same fact is dropped. None of these are code or test changes -- prose only, matching the cross-review's own framing. #848 stays unmarked (its own "Not breaking" paragraph holds); #838, #849 and #853 stay unmarked per explicit instruction not to add markers beyond what was asked.
…g claim The previous fix qualified "Only ... change" but dropped the two file names it was qualifying, leaving "these two" with no antecedent -- a grep for test_engine_runtime and test_engine_parity over the whole file returned nothing. Restored the names: "Only tests/integration/test_engine_runtime.py and test_engine_parity.py change among the test files". Also softened "(0 hits for tshark anywhere in that job's log)" -- nobody had actually read that CI log, on this pass or the last. Checked what is verifiable instead: 30bca99's diff shows the pypcap-parity job's apt-get install step named packages "build-essential libpcap-dev" before this change and "build-essential libpcap-dev tshark" after, so the entry now says its apt-get install step named no tshark package -- true by the diff alone, no log read required.
…ence fix util/changelog_md.py regenerated from 1.5.0.rst; --check exit 0.
engine='pyshark' had no CI job driving it through a real tshark-parsed capture: pypcap-parity installed pyshark but not tshark, so PyShark.unsupported_reason() declined the engine, the extractor fell back to the built-in parser, and the one real-extraction assertion passed against the fallback's output -- a vacuous pass invisible in every skip count. pypcap-parity now installs tshark with the same debconf pre-seed engine-tests already carries, and the test proves which engine actually ran rather than trusting the assertion alone.
Adds util/pyshark_encap_map.py, a generator that regenerates ENCAP_TYPE_TO_LINKTYPE (152 entries) and FILTER_NAME_TO_LINKTYPE (58) in place inside pcapkit/toolkit/pyshark.py, so the two tables #850 hand-built stop being hand-maintained. Guards against its own worst failure mode: a naive LinkType(dlt) lookup cannot detect an unmapped DLT, because _missing_ mints a placeholder rather than raising, so the generator snapshots every known value before any lookup. The real-tshark sweep is gated on a new HAS_WIRESHARK flag and is version-pinned -- editcap -T accepts 226 encapsulations on Wireshark 4.6.9, 224 on 4.2.2 (CI's Ubuntu noble) -- so the count assertions run only under the measured version. One line of pyshark.py itself changes as a result: the IPMB_LINUX table entry becomes I2C_LINUX, the canonical name #848 gave value 209. util/changelog_md.py regenerated CHANGELOG.md for all seven entries added across this and the six preceding commits (#838, #846, #847, #848, #849, #850, #853); `--check` exit 0.
#846 and #850 - #838's "the 21 whose vendor crawler leaves Vendor.process() unmodified" measures 22, not 21 -- pcapkit.const.hip.transport.Transport also has an unmodified process(), but its FLAG-bound range has no unassigned gap, so its _missing_ carries no bounded-range extend_enum branch to fix. The real discriminator is that branch, not the process() override; reworded, and the "other 84" split into the 83 that do override process() and the one that doesn't but has nothing to fix either. Re-derived independently against 5e25db2^..5e25db2 (not main, which would fold in #847's own edit to ipx/socket.py): 22 registries have no process() override, 21 of them have the bounded-range branch, matching tests/const/test_const_enum_no_mint.py's own REGISTRIES_WITH_UNASSIGNED_RANGES (21 entries) vs ALL_REGISTRIES (22, +hip.transport, with a NOTE explaining the exclusion). - #846's "Only tests/integration/test_engine_runtime.py and test_engine_parity.py change" is false -- 30bca99's own numstat also touches .github/workflows/unit-tests.yml (17+/2-). Qualified to "only these two test files change". - #850 gets the breaking marker: its own prose already says the fallback is gone and an unrecognised encapsulation or filter name now raises MissingKeyError instead of substituting a plausible DLT -- the same shape as the two existing markers at AppType.get (1.5.0.rst:2456) and the four .get()-backed enum fields (1.5.0.rst:2720). Restructured to lead with the marker and the subject, matching their wording and placement; the later restatement of the same fact is dropped. None of these are code or test changes -- prose only, matching the cross-review's own framing. #848 stays unmarked (its own "Not breaking" paragraph holds); #838, #849 and #853 stay unmarked per explicit instruction not to add markers beyond what was asked.
…g claim The previous fix qualified "Only ... change" but dropped the two file names it was qualifying, leaving "these two" with no antecedent -- a grep for test_engine_runtime and test_engine_parity over the whole file returned nothing. Restored the names: "Only tests/integration/test_engine_runtime.py and test_engine_parity.py change among the test files". Also softened "(0 hits for tshark anywhere in that job's log)" -- nobody had actually read that CI log, on this pass or the last. Checked what is verifiable instead: 30bca99's diff shows the pypcap-parity job's apt-get install step named packages "build-essential libpcap-dev" before this change and "build-essential libpcap-dev tshark" after, so the entry now says its apt-get install step named no tshark package -- true by the diff alone, no log read required.
…ence fix util/changelog_md.py regenerated from 1.5.0.rst; --check exit 0.
1 participant
Please follow the guide below
make pylint,make mypy,make isort)make testpasses, and a test case covers the changedocs/source/changelog/and regeneratedCHANGELOG.md, if the change is user-visible — N/A — changelog centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657What is the purpose of your pull request?
fix— corrects a defectfeat— adds a featureperf— changes performance, not behaviourrefactor— changes neither behaviour nor performancetest— tests onlydocs— documentation onlyci— workflows or build toolingchore— anything elseDescription of your pull request and other information
#845's first finding: no CI job anywhere drove
engine='pyshark'through a realtshark-parsed capture, on any Python version, so the engine's own code path was never
exercised.
The one real
engine='pyshark'extraction is intests/integration/test_engine_runtime.py.engine-testsinstalls bothpysharkandtsharkbut its selection ignorestests/integrationand*_runtime.pywholesale, so itcannot reach it.
pypcap-paritycan reach it and installspyshark— but installed notshark(grep -ci tsharkover its whole job log returned 0). SoPyShark.unsupported_reason()declined the engine, the extractor fell back to the built-inparser, and
assertGreater(extractor.length, 0)passed on the fallback's output. Avacuous pass, invisible in every skip count.
Two changes:
pypcap-parityinstalls tshark, with the debconf pre-seed andDEBIAN_FRONTENDcopied from
engine-tests's own tshark step — without the pre-seed, tshark's postinstblocks on the "allow non-superusers to capture packets" prompt.
EngineWarningfired,extractor._exnam == 'pyshark', and the engine is aPySharkinstance. Same idiom astest_new_engine_parity_runtime.py'sextracthelper already uses for PyPCAP. Theexisting length assertion is kept, not weakened.
Deliberately narrow. #845's other two findings are not here: the PyPCAPFile
marker-shadowing fix is superseded by the maintainer's two-dimension matrix ruling on that
issue, and promoting
Engines Python Xto a required check is a ruleset change rather thana workflow one.
One honest limit. The new assertion sits in the
sys.version_info < (3, 14)branch, andthis host's venv is 3.14.7, so it takes the other branch locally — it is verified by
construction against the identical idiom in the already-passing parity test, not by local
execution. CI's 3.10/3.11 legs are what will actually exercise it.
Tests: 106 passed (
tests/integration/test_engine_runtime,tests/test_tier_guard).Refs #845.