test: correct docstrings on the #572 truncated-option tests - #580
Merged
Merged
Conversation
An independent cross-review of #578 (which merged before the review finished) found the two new tests correct in every constructed byte, assertion, and pass/fail outcome, but flagged several prose errors in their docstrings: - 0x4f is TCP's Option.Reserved_79, not an "unassigned" kind (the UnassignedOption *schema class* handles it, but the wire code itself is reserved) -- fixed the TCP docstring's wording. - The IPv4 docstring attributed the option_padding rewind-and-reread- as-padding mechanism to "#431 machinery" inside OptionField.unpack. It is actually in Schema.unpack (schema.py:890), added by #371, and predates #431; #431's own contribution to OptionField.unpack is only the post-break progress check, which performs no rewind here. - The IPv4 docstring claimed declaring an 8-octet option area would trip IPv4's stricter length-sum check "before the accommodation under test is ever reached." The accommodation does run -- the short data field is read and left-padded -- the outer check just discards that result afterwards. Fixed to say so. - The TCP docstring attributed the "sizes by what it consumed, not by the declared length" measurement to OptionField.unpack; it is TCP._read_tcp_options itself (tcp.py:698, `len(schema)`). - The TCP docstring's opening line ("cut short mid-option") was wrong for the TCP fixture specifically: nothing is truncated there (hdr_len == len(raw), and the test asserts a full round-trip); the over-declaration is internal to the option, not the capture. Reworded. Left the IPv4 opening line as-is, since that fixture genuinely is short. - The TCP docstring's justification for checking length=32 alongside 12 ("the fix would reject both identically") argued for one case being enough; replaced with the actual distinction (pad width scales with the declared length: 24 zero octets vs 4). - Switched both tests' fixed 6-octet trailing literal to bytes.fromhex(), matching the surrounding files' idiom. No assertion, constructed byte, or test outcome changes. Both tests still pass; both still fail against a reject-on-any-shortfall guard with the FieldValueError text quoted in #572. Build/test: unit tier (pytest -q --ignore=tests/integration --ignore-glob='*_runtime.py' --ignore-glob='*_regression.py') green on this branch, same as before the docstring changes.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #578, which merged before its cross-review finished. The
independent review (on a different model, per this repo's practice for
agent-raised changes) found the two new tests —
TCPUDPUnitTestsandIPv4UnitTests'test_a_truncated_option_still_parses_its_declared_length— correct in every constructed byte, assertion, and pass/fail outcome, but
flagged several factual errors in their docstring prose:
0x4fis TCP'sOption.Reserved_79, not an "unassigned" kind (theUnassignedOptionschema class handles it, but the wire code itself isreserved, not unassigned).
option_paddingrewind-and-reread-as-padding mechanism to "OptionField.unpack never returns for a well-formed HOPOPT header with an SMF_DPD option #431 machinery" inside
OptionField.unpack. It isactually in
Schema.unpack(pcapkit/protocols/schema/schema.py:890),added by Fix seven PCAP-NG parser defects (#341-#347) #371, and predates OptionField.unpack never returns for a well-formed HOPOPT header with an SMF_DPD option #431 — OptionField.unpack never returns for a well-formed HOPOPT header with an SMF_DPD option #431's own contribution to
OptionField.unpackis only the post-break progress check, which performsno rewind in this case.
IPv4's stricter length-sum check "before the accommodation under test is
ever reached." That's mechanically false: the accommodation does run (the
short
datafield is read and left-padded), the outer_read_ipv4_optionscheck just discards that result afterwards, once itsloop over the parsed options finishes.
declared length" measurement to
OptionField.unpack; it is actuallyTCP._read_tcp_optionsitself (pcapkit/protocols/transport/tcp.py:698,counter += len(schema)).the TCP fixture specifically — nothing in it is truncated (
hdr_len == len(raw), and the test asserts a full round-trip); the over-declarationis internal to the option, not the capture. Reworded. Left the IPv4
opening line as-is, since that fixture genuinely is short (28 physical vs
36 declared octets).
length=32alongside 12("the fix would reject both identically") argued for one case being
sufficient rather than explaining why both are checked; replaced with the
actual distinction — the pad width scales with the declared length (24
zero octets vs 4).
bytes([0xaa, ...])tobytes.fromhex('aabbccddeeff'), matching the surrounding files'idiom.
No assertion, constructed byte, or test outcome changes here — this is a
docstring-only correction. Both tests still pass, and both still fail
against a reject-on-any-shortfall guard in
FieldBase.unpackwith theFieldValueErrortext quoted in #572.Test plan
pytest tests/protocols/transport/test_tcp_udp_unit.py tests/protocols/internet/test_ipv4_unit.py -k test_a_truncated_option_still_parses_its_declared_length -q— 2 passed, 2 subtests passed, unchanged from before this correctionpytest -q --ignore=tests/integration --ignore-glob='*_runtime.py' --ignore-glob='*_regression.py'