test(tcp): reach the MP_JOIN dispatchers through TCP(), not a hand-written _flags (#603) - #612
Conversation
|
✅ GOOD TO MERGE — head |
Cross-review appendix — PR #612Reviewer: Sonnet; PR authored on Opus 5. Reviewed at head
|
…itten _flags (#603) `tests/protocols/transport/test_tcp_udp_unit.py` assigned a plain Python `set` to `TCP._flags` on a bare `object.__new__(TCP)`. A `set` answers the membership tests `_make_mptcp_join` and `_read_mptcp_join` use, so every flag branch ran and both TCP modules read 100% statement and branch coverage -- while the attribute had neither the `aenum.IntFlag` type production assigns nor the ordering that governs when it exists. That is how #587 stayed invisible behind that number. * the MP_JOIN cases now build through `TCP()` itself, in both the keyword and the data-model construction forms, via a new `mptcp_option` helper that constructs a fresh instance per call; * the reader cases resolve `_flags` with `proto.make(...)` instead of writing the attribute, so the dispatcher sees the real enum member; * stale prose corrected: `mptcp_dss_ack_selector`'s note said fixing the callable-length `NumberField` belonged to `pcapkit.corekit.fields.numbers`, where #598 has since fixed it, and claimed wire absence was what a `ConditionalField` could not express -- `MPTCPDSS.ssn`, `dl_len` and `checksum` have always been `ConditionalField` on the sibling `M` flag. The `SwitchField` form is kept for the narrower `length`-safety reason the note now states; * `test_tcp_mptcp_length_arithmetic_unit.py`'s claim that MP_JOIN cannot be built through `TCP()` at all is likewise marked as true only until #587. Behaviour-identical, so coverage of both modules is unchanged at 100% statement and 100% branch. Proven instead by injection: reverting #587's hoist fails 2 of this file's 17 tests with `AttributeError: 'TCP' object has no attribute '_flags'`, and restoring `cast('Enum_Flags', 0)` fails 2 with `TypeError: argument of type 'int' is not a container or iterable` -- both of which the `set`-based version passed. 148 tests pass across `tests/protocols/transport/` and the option round-trip suite; `changelog_md.py --check` exits 0.
64db391 to
615e2c3
Compare
Cross-review re-point — PR #612 (post-merge record)Reviewer: Sonnet; PR authored on Opus 5. The ✅ above was earned at head Both rebase hops confirmed changelog/docs-only
Net effect identical to what the original review examined
Worth naming the trap for the record: the unrestricted Tests re-run green at the merged head
Disagreement logNone. The verdict carries over cleanly to |
Cross-review re-point — PR #612 (posted after merge)Reviewer: Sonnet; PR authored on Opus 5. This re-points the Stated plainly: this landed before the re-point was finished. The merged tree is the tree that was reviewed
The two hops were not code changes
The PR's own three files are unchanged — checked by blob hash, not by diff
Blob-hash equality rather than an empty Re-reproduced at the new head
Not independently checked
Disagreement logOne precision note, not a dispute: "changelog-only rebase" understates hop 1, whose raw diff spans 137 files. The maintainer's own contribution in that hop is changelog-only; the other 134 files are pre-existing upstream commits arriving with the rebase, confirmed by exact file-set match. No disagreement with the substance. |
…604) (#628) * `TCPUDPUnitTests.test_a_truncated_option_still_parses_its_declared_length` expected a truncated TCP option's `data` as the synthesised zero octets followed by the real ones, which is what `rjust()` produced. #621 made the padding `ljust()` everywhere but could not retarget this file, because #612 owned it at the time; it has been red on `main` since #621 merged. * The real octets now come first for both parametrised widths, and the docstring above the assertion says tail-padding rather than left-padding. * Test-only: no library code changes. The sibling case in `tests/protocols/internet/test_ipv4_unit.py` was already retargeted in #621. Measured against `main` at 2221c2d: two subtest failures before, none after. `tests/protocols/transport/` and `tests/corekit/test_fields_field.py` together give 177 passed, 232 subtests passed.
JarryShaw
added
the
test
1 participant
Fixes #603
tests/protocols/transport/test_tcp_udp_unit.pyassigned a plain PythonsettoTCP._flagson a bareobject.__new__(TCP). Asetanswers theintests_make_mptcp_joinand_read_mptcp_joinuse, so all six flag branches ran and both TCP modules read 100% statement and branch coverage — while the attribute had neither theaenum.IntFlagtype production assigns nor the ordering that governs when it exists at all. That is how #587, an ordering defect that broke MP_JOIN construction for every caller, sat behind that coverage number untouched, and how thecast('Enum_Flags', 0)no-op behind it went unnoticed too.What changed
mptcp_optionhelper builds one MPTCP option throughTCP()itself, in both construction forms the library offers — the keyword form (options=[(Option.Multipath_TCP, {...})]) and the data-model form (anOrderedMultiDictof apcapkit.protocols.data.transport.tcpobject). Each call constructs a fresh instance, so_flagsis resolved from those very arguments rather than left over from an earlier call.proto._flags = {...}MP_JOIN sites are gone. The three maker sites now go throughmptcp_option; the three reader sites resolve the attribute withproto.make(syn=..., ack=...), which is the public entry point that assigns it._flags(MP_CAPABLE, DSS, ADD_ADDR, REMOVE_ADDR, MP_PRIO, MP_FAIL, MP_FASTCLOSE) keep callingproto._make_mode_mp/proto._read_mode_mpdirectly — routing them through the constructor would say nothing extra, and the issue asks for the diff to stay inside its argument.The change has teeth — measured, not asserted
The rewrite is behaviour-identical, so the load-bearing evidence is defect injection. Each defect was re-introduced on a scratch copy of
pcapkit/protocols/transport/tcp.pyand this file run against it, once with theset-based version frommainand once with the rewrite. 17 tests in the file.main'sset-based testsself._flags = _flagback below_make_tcp_options)_flag = cast('Enum_Flags', 0)restoredVerbatim,
--tb=line:Against the same two injections the
set-based file printed17 passedboth times, with no failures at all.Two details worth naming.
tcp.py:2812is_make_mptcp_join's first membership test andtcp.py:1558is_read_mptcp_join's — so restoring thecastis caught on both dispatchers, the reader included, because the reader cases now take their flags frommakerather than from a hand-writtenset. And the singleassertRaises(ProtocolError)on a flagless MP_JOIN catches both defects on its own: revert the hoist and it raisesAttributeError, restore thecastand it raisesTypeError, where asetgave the right answer for the wrong reason.Coverage does not move, and that is the point
Over
tests/protocols/transport/, withcoverage run --branch:pcapkit/protocols/transport/tcp.pypcapkit/protocols/schema/transport/tcp.pyUnchanged, as #603 predicted — a coverage number cannot tell "exercised under production conditions" from "exercised with a hand-placed attribute of the wrong type". The injection table above is the measurement that can.
(One correction against my own first run, which read the schema module at 95% before: that was an artefact of editing the schema file while the run was in flight, shifting line numbers under it. Re-run cleanly, both readings are 100%, and an isolated single-file run reads 96%/99% identically before and after.)
Prose corrected — and one claim of my own falsified
mptcp_dss_ack_selector's note said a corrected field-width lambda "would not have worked" becauseNumberFieldcannot pack a callable length, and that fixing it "belongs topcapkit.corekit.fields.numbers". #598 fixed it there, so the note now marks that half as history. The same paragraph intest_tcp_mptcp_length_arithmetic_unit.py's module docstring gets the same treatment, as does its separate claim that MP_JOIN "cannot be built through the publicTCP()constructor at all" — true only until #587.#603's text also carried forward a justification that turns out to be wrong, and I have not restated it. It said the
SwitchFieldworkaround stays because itsNoValueField()branch handles wire-absence "which a callable-lengthNumberFieldcannot express". Absence was never the obstacle:MPTCPDSS.ssn,dl_lenandchecksumare each aConditionalFieldon the siblingMflag, so this very class has always relied on that wrapper to keep a field off the wire. Measured on this tree,ConditionalField(NumberField(length=lambda pkt: 8 if pkt['flags']['a'] else 4), lambda pkt: pkt['flags']['A'])packs 8 octets, 4 octets andb''correctly and round-trips all three.What actually remains is narrower and about composition, not absence:
ConditionalField.lengthforwards to the wrapped field without consulting the condition, so reading it while the condition is false — the wrapped field still at its-1placeholder — raisesstruct.error: bad char in struct format. Nothing here meets that only becauseSchema.packandSchema.unpackspecial-caseConditionalFieldby name andcontinuebefore anylengthis read. ASwitchField's selector always hands back an already-concrete field,NoValueField()included, so itslengthis safe wherever it is read. That is what the note now says.Reported, deliberately not done: the
SwitchFieldform onMPTCPDSS.ack/.dsncould now be simplified to aConditionalFieldwith a callable-lengthNumberField. Replacing it is a behaviour change outside #603's scope, so this PR only records the finding.Also found, deliberately not fixed
TCP.readstill seeds its accumulator withcast('Enum_Flags', 0), so a flagless parsed segment leaves_flagsa plainint—TCP(raw, len(raw))._flagsis0, andEnum_Flags.SYN init raisesTypeError. That is why the reader cases take their flags frommakerather than from a parsed segment. It is deliberate and unreachable from a caller (mptcp_data_selectorrejects a flagless MP_JOIN in the schema layer first, andtest_tcp_mptcp_join_flag_ordering_unitpins both halves against real segments), it lives in a production file outside this PR's scope, and the reason is recorded in a comment at the site.Verification
148 passed, 457 subtests passedacrosstests/protocols/transport/andtests/protocols/test_option_roundtrip_unit.py, on Python 3.14.7 — before and after the rebase onto4529fdb1f.python util/changelog_md.py --checkexits 0.origin/main. The1.5.0.rstrebase conflict against pre_process sizes a value with floor division disguised as math.ceil, so every octet boundary raises OverflowError #599's and test_stacklevel asserts an exact warning count, so a leaked in.pcap handle fails it on unrelated PRs #606's bullets was resolved by keeping every bullet with mine last and regeneratingCHANGELOG.md, not by hand-merging the generated file.