Skip to content

test(tcp): retarget the last assertion pinning the old padding side (#604) - #628

Merged
JarryShaw merged 2 commits into
mainfrom
fix/604-tcp-udp-padding-side
Sep 22, 2026
Merged

JarryShaw merged 2 commits into
mainfrom
fix/604-tcp-udp-padding-side

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Follow-up to #621, which fixed #604. main is red without this.

What is broken on main

TCPUDPUnitTests.test_a_truncated_option_still_parses_its_declared_length
(tests/protocols/transport/test_tcp_udp_unit.py:1415 on main) pins the padding
side that #604 corrected:

self.assertEqual(unassigned.data, b'\x00' * zeroes + trailing)

A truncated TCP option's data is the octets that were actually read followed by
the zeros synthesised for the ones that were not — rjust() produced the reverse,
and #621 changed FieldBase.unpack to ljust(). Both parametrised widths
(declared_length=12 and 32) therefore fail.

Measured against main at 2221c2d8f, from a git archive export with
pcapkit.__file__ asserted inside the export before any other import:

# main as it stands
2 failed, 1 passed, 16 deselected, 4 warnings in 1.43s
  SUBFAILED(declared_length=12) ...::test_a_truncated_option_still_parses_its_declared_length
  SUBFAILED(declared_length=32) ...::test_a_truncated_option_still_parses_its_declared_length

# with this change
1 passed, 16 deselected, 3 warnings, 2 subtests passed in 0.64s

Why it was not in #621

#621 deliberately left it. This file was concurrently owned by #612 at the time,
and editing it would have risked discarding work that has since merged — #621's
own body called that out and named the exact one-line change needed. #612 merged,
and then #621 merged before the follow-up could be folded into it, so the
correction lands here instead.

The change

Test-only — no library code changes. The sibling case in
tests/protocols/internet/test_ipv4_unit.py was already retargeted in #621, and
bytes(proto.__header__) == raw on the line below the changed assertion passes
untouched, since the pack path is byte-preserving and is not affected by the
padding side.

Verification

tests/protocols/transport/ + tests/corekit/test_fields_field.py
    177 passed, 232 subtests passed, 0 failed

Run with PYTHONSAFEPATH=1 and PYTHONPATH set to the tree under test, with
pcapkit.__file__ and the padding side both asserted before the run — this
machine's venv editable install otherwise resolves pcapkit to a checkout that is
behind origin/main, which silently invalidates a bare run.

…604)

* `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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test Pull requests that add or correct tests (test: subject prefix)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FieldBase.unpack pads a short read with rjust regardless of byte order, silently corrupting little-endian values

1 participant