tests: round-trip 258 option codes from the registries, recording the 86 that cannot close the cycle - #440
Conversation
…d option code The sample corpus covered the parse path for common protocols and almost none of the option, chunk, parameter and block space -- and none of the *construction* side of it, which is why defects there keep being found by hand. - Add `examples/generators/options.py`. It enumerates 258 codes from the dispatch registries themselves (TCP, MPTCP, IPv4, HOPOPT, IPv6-Opts, IPv6-Route, SCTP chunks/parameters/causes, MH messages/options/extensions, HIP parameters, HTTP/2 frames, PCAP-NG blocks/options/records/secrets), then for each one constructs it, parses it back, constructs it again from what was parsed, and compares the octets. Registries are iterated, never subscripted, so no lookup grows a shared `defaultdict`; `handler()` goes through `_lookup_registry`. - Write the 166 cases whose parse terminates into five `options-*.pcap` fixtures, wired into `make_samples.py`. They regenerate byte-identically. - Add `tests/protocols/test_option_roundtrip_unit.py`: 172 of 258 cases close the cycle; the other 86 are recorded case by case against the `file:line` that stops each, and are asserted to still fail in the recorded way so a fix turns the tier red rather than leaving a stale entry. - Add `tests/protocols/test_option_coverage_runtime.py`, reading the fixtures through the extraction interface and both dumpers. - Vendor `time_limit` into `tests/_support.py`, byte-identical to #431's copy; two HOPOPT/IPv6-Opts cases hang rather than fail, so the sweep is deadlined. No library file is touched: the 86 failures are reported, not fixed. 886 passed, 17 skipped (877 before, adjusting for 18 tier-guard tests that only skip in a non-git tree). Every pre-existing capture, and its `tree` and `json` dump under `ip=True, tcp=True, reassembly=True`, is byte-identical to origin/main.
…samples # Conflicts: # tests/_support.py
…erpreter, not the code Three things, all consequences of #432, #434 and #439 landing under the branch. - `INTERPRETER_GAPS`: seven PCAP-NG name-resolution cases round-trip from Python 3.11 on and fail to construct on 3.10, so one recorded outcome per case no longer suffices. The root cause is #439 in the *schema* hierarchy: `NameResolutionBlock.post_process` asks `isinstance(record, (IPv4Record, IPv6Record))` at `pcapkit/protocols/schema/misc/pcapng.py:1248`, every `Schema` subclass shares one `_abc_impl` on <= 3.10 (measured: `EndRecord._abc_impl is IPv4Record._abc_impl` is True on 3.10.20, False on 3.14.7), and the block's terminating `EndRecord` therefore tests True and has `.names` read off it. Measured in the real path, not inferred. The `Info` data models are unaffected on both interpreters, and that boundary is asserted too. The table overrides rather than sits beside `EXPECTED_FAILURES`, because the three `ns_dns*` options fail on every interpreter but for different reasons. - `test_schema_isinstance_is_interpreter_dependent` pins that mechanism, so the seven are excused by evidence about a named library bug rather than by a version comparison. On >= 3.11 they are still held to `OK`; fixing #439 turns 3.10 red. - The two SMF_DPD entries: `hopopt-option/SMF_DPD` now round-trips and its entry is deleted, since #429's over-read fix landed with #432's progress guard. `ipv6-opts-option/SMF_DPD` raises instead of hanging and is re-recorded as `PARSE`. The two schema modules are line-for-line duplicates, so that is one fix applied once where it was needed twice. The sweep keeps its deadline: it guards the next non-progress defect, not this one. Also: #434 gave IPv4 and HIP real registries, so the generator now reads `HIP.__parameter__` instead of falling back to enum-crossed-with-handler. No library file is touched. 3.14.7: 258 cases, 173 round-trip. 3.10.20: 258 cases, 169 round-trip -- the difference is exactly the four cases in `INTERPRETER_GAPS` that pass on 3.14.
The entry claimed #432's fix "was applied once where it was needed twice". That was wrong, and it was an inference from the behaviour rather than something measured: #432 touched both schema modules symmetrically, 26 lines each, changing `'len': (1, 8)` to `(8, 8)` and adding the `+ 2` to the selector's `SchemaField` in each. Neither file still carries `(1, 8)`. The real cause is one line, older than #432. Normalising the two modules for their protocol names leaves exactly one structural difference: `ipv6_opts.SMFIdentificationBasedDPDOption` declares a second, redundant `test` `ForwardMatchField` at :434 that `hopopt`'s equivalent does not. The enclosing `_SMFDPDOption` already has one in both modules, and it is that outer field the selector reads -- nothing reads the nested copy, and it is the only field in the class with no `#:` comment. A `ForwardMatchField` consumes nothing but still occupies a slot in `__buffer__`, so the nested schema over-reports its size by an octet and `OptionField` mis-counts the option area. Measured on the same octets, `1100080100010100`, identically on 3.10.20 and 3.14.7 -- so this one is not interpreter-dependent: hopopt __fields__ = [type, len, info, tid, id] len(schema) = 3 ipv6_opts __fields__ = [type, len, test, info, tid, id] len(schema) = 4 HOPOPT(...) -> options=[SMF_DPD, PadN] IPv6_Opts(...) -> ProtocolError: IPv6-Opts: invalid format at pcapkit/protocols/internet/ipv6_opts.py:497 Comment and `Gap.defect` text only; the recorded status and fragment are unchanged, and no library file is touched.
Review of PR #440 at head
|
The recorded fragment is what makes an EXPECTED_FAILURES entry self-maintaining: it has to fail again *in the recorded way*, or the entry gets revisited. Seven entries recorded only `invalid format` or `invalid parameter`, and those are substrings of 205 and 3 messages respectively across 13 modules -- 31 in internet/hip.py, 26 in transport/tcp.py -- so a regression at any of them satisfied the check. Each now carries the alias and whatever bracketed code the message prints, measured from the real detail rather than guessed: - tcp-option/User_Timeout_Option TCP: [OptNo 28] invalid format - ipv6-opts-option/SMF_DPD IPv6-Opts: invalid format - hip-parameter/HIP_TRANSFORM HIPv2: [ParamNo 577] invalid parameter - hip-parameter/HOST_ID HIPv2: invalid format - httpv2-frame/PRIORITY HTTP/2: [Type 2] invalid format Five of those now match exactly one raise site: the bracketed code picks out the one handler that can print it, and the two bare-alias forms are the only sites in their module that omit the bracket at all. `fragment` also accepts a tuple, all of whose members must appear. That is for the two ipv6-route entries, whose message interpolates a bare `type` in a method that has no such parameter and so renders `[TypeNo <class 'type'>]` (#442). Matching the literal rendering would bake that bug into the table and turn it red when #442 is fixed, which says nothing about the round trip; the stable parts either side still narrow it to the three `[TypeNo` lines of ipv6_route.py. The assertion failure now also prints the detail it got. Verified on 3.14.7 and 3.10.20: 7 passed, 299 subtests each. Negative control -- deliberately wrong fragments in both the string and the tuple form -- fails 3 subtests, so the assertion is reached rather than vacuous.
Review of
|
| Interpreter | tree confirmed via pcapkit.__file__ |
tests run | subtests | failures |
|---|---|---|---|---|
3.14.7 (.venv) |
under the checked-out worktree | 7 | 299 | 0 |
3.10.20 (/tmp/mh310-a3a0ca27/venv, tree forced via sys.path[0]) |
under the checked-out worktree | 7 | 299 | 0 |
Matches "7 passed / 299 subtests on both 3.14.7 and 3.10.20" exactly.
The "full suite 909 passed / 17 skipped" figure — could not reproduce; flagging, not blocking
This one doesn't check out. CI's own Unit Tests jobs (the make test selection, i.e. pytest -q --ignore=tests/integration --ignore-glob='*_runtime.py' --ignore-glob='*_regression.py') report 721 passed, 69 skipped, 1040 subtests passed, identically across Python 3.10–3.15. CI's Integration Python * jobs report 855 passed, 88 skipped, 1188 subtests passed, also identically across versions. I reproduced the 3.10 unit-job number exactly locally (721/69/1040, tree confirmed via pcapkit.__file__). My local 3.14.7 run came back as 782 passed/8 skipped/1082 subtests — 61 more passes than CI's own 3.14 job, which is a dev-venv-has-more-optional-deps artifact (RUNTIME_DEPS-gated tests running instead of skipping), not a code issue. None of these four numbers is 909/17, and I did not run make samples && pytest -q (the truly-unfiltered selection, needing generated fixtures) to check whether that configuration hits it — that's the one combination I didn't try. What I can say confidently: every configuration I ran was 100% clean, zero failures, zero errors, matching the green CI. The specific "909/17" figure in the commit message appears to be inaccurate or drawn from an environment/selection I couldn't reproduce; it doesn't affect the correctness of the diff itself.
Verdict
GOOD TO MERGE at sha 651665214. Commit 4535f72f2 does exactly what it claims for the seven EXPECTED_FAILURES entries: every new fragment was measured against the real outcome.detail (not guessed), the counts backing the "too generic" argument check out exactly against origin/main, the tuple form is implemented correctly for the string/tuple/empty-string cases and is genuinely robust to the fix issue #442 proposes, and the negative control is reproducibly non-vacuous. CI is fully green on this sha. The one soft spot is the commit message's "full suite 909 passed / 17 skipped" line, which I could not reproduce under CI's own selection or locally on either interpreter (real numbers: 721/69 unit, 855/88 integration, consistently, with zero failures throughout) — worth a correction in the message, but not a reason to hold the CR.
|
Thanks — the numbers do disagree, but the stale one is the PR body's, not the
So:
On CI's own figures (721 / 69 for Unit Tests, 855 / 88 for Integration) — those are not comparable to a local
That difference is expected rather than a discrepancy: neither job is trying to reproduce a local full-suite number. What matters for the verdict is that every configuration came back with zero failures, which both of us observed. I am updating the body's suite line to the current post-merge measurement so it stops misleading the next reader. Good catch on the mismatch even though the cause turned out to be elsewhere — the body had been carrying a figure from two merges ago and nobody had noticed. |
Picks up #440 (option round-trip harness) so its EXPECTED_FAILURES table can drop the SMF_DPD entry this PR fixes.
…entry #440's round-trip harness recorded this case as a PARSE gap; the fix in this branch closes the round trip, so the entry is removed rather than left stale. Kept the surrounding comment as history of both the #432 and #441 fixes, matching how the hopopt half of the same story was already handled.
…d the CGA extension carrier PR #440's round-trip harness enumerates the registries, so completing `MH.__option__` and `MH.__extension__` added 64 cases to it -- and seven of them were red. Two different reasons, and only one of them is a defect. Four are the harness constructing an option with no arguments at all, which for these four is not a well-formed instance of the option: * Service Selection: `Length` of 0 "is not allowed" and the identifier is 1-255 octets [RFC 5149 section 3]. * Redirect: "Both the 'K' and 'N' flags cannot be set or unset simultaneously" [RFC 6463 section 4.2], so with neither address given the option's own length is undetermined. * Access Network Identifier: "MUST contain at least one ANI sub-option" [RFC 6757 section 3]; it is a pure container. * LMA-Controlled MAG Parameters: likewise at least one sub-option [RFC 8127 section 3]. The constructors refuse all four correctly, so the validations stay and `_mh_option_overrides` gains the arguments that make the codes reachable -- which is what that table is already for, alongside the eleven MH options whose no-argument default is likewise invalid. This raises coverage rather than avoiding anything: all four now round-trip carrying real content, e.g. the ANI option emits `340f 010d 0004 77696669 06 001122334455`. The other three are `mh-extension/Exp_FFF*`, and they are #445 rather than mine. Measured: all four extension codes -- including `Multi_Prefix`, which the table already records -- fail identically with `PARSE / KeyError: 'length'`, raised from `SchemaField.unpack` at `corekit/fields/misc.py:619` by way of `CGAParameter.extensions`, before any extension schema is unpacked at all. A CGA extension has no carrier but the CGA Parameters option, so the whole registry is unreachable until #445 and #446 land. Three `EXPECTED_FAILURES` entries record that, grouped with the existing one and naming the same site; that entry's `file:line` is also refreshed, since this branch moved the lambda from :516 to :873. `pep.rst` claimed all four CGA extensions round-trip byte-for-byte. Their handlers do, when driven directly, but nothing can reach them through the public API, so the claim is narrowed to what is actually true and points at the recorded gap instead. Suite on 3.14: 973 passed, 17 skipped, 1544 subtests. On 3.10: 899 passed, 91 skipped, 1446 subtests. Baseline e2d8ed6 on 3.14: 942 passed, 1256 subtests. The option harness is 7 passed / 363 subtests on both interpreters, and `make_samples.py` regenerates all 24 captures byte-identically.
1 participant
Closes the Test Cases ask on the Help Wanted page, which says what remains wanted is "coverage rather than infrastructure: the protocols and options the suite does not touch".
This adds that coverage for the option/chunk/parameter/block space, in both directions, and the result is the point of the PR: of 258 registry codes exercised, 172 close a construct → parse → construct cycle and 86 do not. Every one of the 86 is recorded in
tests/protocols/test_option_roundtrip_unit.pyagainst thefile:linethat stops it, and asserted to still fail in the recorded way — so fixing one turns the tier red rather than leaving a stale entry behind.pcapkit/is untouched:git diff origin/main -- pcapkit/is empty. This is tests and generators only.How the codes are enumerated
examples/generators/options.pyreads the registries themselves rather than a hand-written list: TCP options 22, MPTCP subtypes 8, IPv4 options 14, HOPOPT 15, IPv6-Opts 15, IPv6-Route 3, SCTP chunks 13 / parameters 8 / causes 13, MH messages 14 / options 20 / extensions 1, HIP parameters 49, HTTP/2 frames 10, PCAP-NG blocks 11 / options 40 / records 3 / secrets 4.Registries are only ever iterated, which mutates nothing, and single-code resolution goes through
ProtocolBase._lookup_registry; a test asserts registry sizes are unchanged after reading. IPv4 and HIP have no registry yet, so those enumerate enum × handler presence, and prefer a real__option__/__parameter__if one appears — so #434 landing is a no-op here.166 cases are written into five
options-*.pcapfixtures viamake_samples.py, regenerating byte-identically across three passes. Fixtures deliberately carry only cases whose parse terminates — a capture containing the SMF_DPD non-progress frame would wedge every reader of it.The seven known construction defects: all reached, and two are worse than recorded
Each was expected and is asserted, not fixed:
ipv4.pydata=where the field ists_data— the call opens at :1480 and the bad keyword is at :1488.Schema.__init__only warns, so the value is dropped andpost_processthen iterates aListField._make_mptcp_*— all 8 subtypes fail, from three distinct causes:KeyError: 'length'(schema/transport/tcp.py:658/790/807/827),no attribute 'kind'(tcp.py:668, three subtypes), and_make_mptcp_joinreadingself._flags, which exists only while parsing (tcp.py:2675).hip.py:3445—cipher=is not a field ofEncryptedParameter, so the cipher id is silently dropped.CGAParameter.extensionsKeyError: 'length'(schema/internet/mh.py:516) — it needspkt['__packet__']['length']on the unpack path. TheMPTCPUnknownhalf of that item is the sameKeyErrorat the fourtcp.pylines above.SchemaField(length=5)is silent corruption, not a failure. A well-formed 8-octet option1908002adeadbee0parses "successfully" withSchemaWarning: packet length < 0: -3, decodingnonceas 55 instead of 933982136, and the three unconsumed octets are then read as a fabricated extra option, making the enclosing packet unparseable. Verified independently: parsingQuickStartRequestOptiondirectly with all 8 octets gives the correctnonce=933982136with no warning, so the schema is right and the selector's hardcoded length is the defect. Identical athopopt.py:224andipv6_opts.py:224. Caveat: the QS cases never reach it, because they fail earlier at CONSTRUCT on a separate defect —funcis set only bypost_process— so fixinglength=5alone will not make them pass._MPTCP.test's'length': (1, 8)— window1e0c01decodeslength=60; with the intended(8, 8)it decodes12. It feedsSchemaField(length=pkt['test']['length']), so it asks for 60 octets of a 12-octet option, and a genuine MP_CAPABLE option cannot be parsed at all._option_padding— mechanism verified (while length > 0, thenlength -= len(data), then_option_padding = length, reachingstruct.calcsize('-1s')→error: bad char in struct format) but not reachable from the existing corpus: an instrumented sweep of all 15 captures saw only{0: 2651, 1: 3, 52: 1}. It is latent. This is the same family as A wire-derived length underflows into a struct format string, raising bare struct.error on untrusted input #438. The related non-progress hang is reached — two SMF_DPD cases TIMEOUT, which is why the sweep is deadlined.Larger finds beyond that list
__post_init__packs and re-parses on one instance sharingself._opt, making 27 of 40 block options unconstructible.extension=True—aliasreads_infobefore it is assigned — and itslen = total // 8 + 4cannot represent a single parameter, so the harness uses two copies and pins the single-parameter defect in its own test._make_*needs lists — reconstruct-only failures, exactly the class the third leg of the cycle exists to catch.KeyError: 'flags', and PRIORITY checkslength != 9for a framemakealways builds as 14.Scope deliberately narrowed
PCAP-NG writes no capture. All 58 codes are exercised and recorded, but the construction API cannot produce a reproducible file:
_make_block_shbtakes byte order fromsys.byteorder, and the TLS/WireGuard key-log writers stampdatetime.now()with no override.examples/generators/pcapng.pyalready covers that space by hand-rolling octets, which is presumably why.Verification
Suite at the current head
0e6a9e15c: 926 passed / 17 skipped / 1268 subtests (567 s) on 3.14.7, zero failures. That is 943 collected, confirmed by counting collection directly.This figure was 886 / 17 when the PR was opened and is corrected here rather than silently: 886 + 17 = 903 collected, which matches no sha now on the branch, because two pass-through merges of
mainhave landed since. Collection is 926 at bothd7240dc0cand4535f72f2and 943 at651665214onwards, the +17 being #436's tests arriving —tests/protocols/test_dispatch_bindings_unit.py0 → 11 methods andtests/protocols/link/test_link_unit.py19 → 25. None of it is this PR's work.Two things about baselines worth keeping, since both have caught agents on this repo. First,
git archive origin/mainmeasures 859 passed / 35 skipped, but 18 of those skips are an artifact of the archive not being a git tree — alltests/test_tier_guard.py, "git could not be run" — and they pass in a real checkout, so usegit clone. Second, CI's own numbers are a different scope again and are not comparable to a local run: Unit Tests runspytest -q --ignore=tests/integrationand never regenerates the sample captures, while Integration runs the full suite after a deliberately partial regeneration on runners lacking the optional engines (pcap,pcapfile).All 19 pre-existing capture files byte-identical to an untouched
origin/maintree, and all 30tree/jsondumps underip=True, tcp=True, reassembly=Truebyte-identical. mypy 9 errors, all pre-existing classes — 5 in_support.pyidentical tomain, 4scapy.allstub errors of the classpcap.pyalready has 15 of. pylint 10.00/10. Tree under test proven on every run viapcapkit.__file__underPYTHONSAFEPATH=1.One coupling to know about before merging
tests/_support.pyonmainhas notime_limit; it lives on the unmerged #432. It is vendored here byte-identically to that branch, so whichever lands first, the second merges cleanly. If #432's version changes before merge, this copy must be re-synced.