From e7a37a3c10f1a83b095f6a53e1aa08dc492b61b8 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Mon, 21 Sep 2026 23:44:29 -0400 Subject: [PATCH] test(tcp): expect the short-read padding on the tail, repairing `main` * `TCPUDPUnitTests.test_a_truncated_option_still_parses_its_declared_length` still expected `b'\x00' * zeroes + trailing`, the head-padded short read that #604 removed. Both subtests failed on that assertion once #621 landed, so `main` was red. The expectation is now `trailing + b'\x00' * zeroes`. * The docstring above it said `FieldBase.unpack` "left-pads" the short read and described the value as four zero octets followed by the six real ones. Both are inverted, so the prose no longer contradicts its own assertion. Test-only; `pcapkit/` is untouched. The file goes 2 failed / 17 passed / exit 1 to 17 passed / 2 subtests passed / exit 0, and the two files #621 updated stay at 61 passed / 164 subtests / exit 0. --- CHANGELOG.md | 1 + docs/source/changelog/1.5.0.rst | 16 +++++++++++++ .../protocols/transport/test_tcp_udp_unit.py | 23 ++++++++++++++----- 3 files changed, 34 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd1fc8786..432072217 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -74,6 +74,7 @@ This is the resolution of #548, which reported `TransType.L2TP` (115) as registe - **Fixed** -- documentation. `mptcp_dss_ack_selector`'s note said a corrected field-width lambda "would not have worked" and that fixing it belonged to `pcapkit.corekit.fields.numbers`, which is exactly where #598 then fixed it; the same paragraph sat in `test_tcp_mptcp_length_arithmetic_unit.py`'s module docstring, whose other stale claim was that MP_JOIN "cannot be built through the public `TCP()` constructor at all", true only until #587. A callable-length `NumberField` packs and unpacks both DSS widths now, and wire *absence* was never the obstacle either: `MPTCPDSS.ssn`, `dl_len` and `checksum` have always been `ConditionalField` on the sibling `M` flag, so the class already relied on that wrapper to keep a field off the wire. The `SwitchField` form is kept for the narrower reason the note now gives -- `ConditionalField`'s `length` forwards to the wrapped field without consulting the condition, so it is safe here only because `Schema.pack` and `Schema.unpack` special-case that wrapper by name, whereas a `SwitchField` always resolves to a concrete field. Replacing it would be a behaviour change and is not made (#603). - **Changed** -- the README is a landing page now, and Markdown rather than reStructuredText. `README.rst` (424 lines) became `README.md` (103), keeping what a reader arriving from PyPI or a search result actually needs -- what the library is, why it exists rather than Scapy or DPKT, how to install it, a worked example, and where the documentation lives -- and dropping the technical detail the documentation already carried. **Module Structure**, **Engine Comparison**, **Engine support by Python version**, **Test Environment**, **Test Results** and **Installation Notes** were each already duplicated in `docs/source/index.rst`, in a fuller form, so they are linked rather than restated. Two blocks existed nowhere else and moved rather than going: **Testing** is now `docs/source/testing.rst`, registered in the index toctree, and the `pipenv` and `make setup` local development block joined the Installation section of `docs/source/index.rst`. Requested by the project owner, and a deliberate exception to the convention that documentation here is reStructuredText -- for the README only, since it is the one documentation file whose renderers are GitHub and PyPI rather than Sphinx. Accordingly `setup.py` reads `README.md` and declares its content type as `text/markdown`, and the `include README.md` line in `MANIFEST.in` is now the only thing that puts the README in a source distribution, because `global-include *.rst` no longer matches it -- which matters, since `setup.py` reads the file unguarded and an sdist without it cannot be installed. Verified with `twine check --strict` against a built sdist and wheel, both of which pass. The rename's own references moved with it, since a change that renames a file owns the references to it: `examples/benchmark/Dockerfile` copies `README.md` -- a literal `COPY` of the old name would have failed the layer outright and taken `make bench`, `make bench-quick` and `run.sh` with it -- and the benchmark harness prose that named the root README as the destination of its generated tables now names `docs/source/index.rst`, which is where those tables went. That covers the `Makefile` comment, `report.py`, `test_harness.py`, `run.sh` and the suite's own README, including the one place whose stated reason had inverted: the emitted markup is kept parseable by plain docutils, which is now a conservative choice rather than a hard requirement, because the page it lands on is rendered by Sphinx. `examples/benchmark/benchmark.py` still says `README.rst` and is left alone, because it means the benchmark suite's own README in the same directory, not the project's. - **Changed** -- `CODE_OF_CONDUCT.md` moves from Contributor Covenant 1.4 to Contributor Covenant 3.0, at the maintainer's request. The text is the canonical 3.0 Markdown fetched from https://www.contributor-covenant.org/version/3/0/code_of_conduct/code_of_conduct.md rather than a transcription, so the pledge, the encouraged and restricted behaviours and the scope are unaltered. Three things needed deciding rather than copying. 3.0 ships two `[NOTE` placeholders an adopter must fill: the reporting channel, which now names `jarryshaw@icloud.com` -- the same contact 1.4 carried and the one `SECURITY.md` already points at as its email fallback -- plus GitHub's report-abuse form for the case a single-maintainer project cannot otherwise cover, a report about the maintainer; and the enforcement section, whose placeholder is an instruction to the adopter and is removed. 3.0 then assigns enforcement throughout to plural "Community Moderators" (and once, inconsistently, to "Community Managers"), which this repository does not have, so all eight occurrences become the singular maintainer. The four-rung ladder -- Warning, Temporarily Limited Activities, Temporary Suspension, Permanent Ban -- is offered as a suggestion and is **kept**, because each rung maps onto a lever one person actually holds on GitHub: a private message, a locked thread, an interaction limit or block, a permanent block. Finally, 3.0 is licensed CC BY-SA 4.0 where 1.4's attribution paragraph carried no licence notice at all, so the attribution now names version 3.0, links the permanent `version/3/0/` URL, carries the CC BY-SA 4.0 notice and link, indicates that changes were made as BY requires, and says explicitly that the share-alike term covers this document only -- the code remains BSD-3-Clause and `LICENSE` is untouched. Rendering was checked against GitHub's own Markdown API rather than assumed: the ladder comes back as four list items each nesting three, which is what #613 had to repair in the 1.4 file when a stray list marker collapsed the whole document into one nested item (#624). +- **Fixed** -- `main` went red the moment #604's `ljust()` landed, because `TCPUDPUnitTests.test_a_truncated_option_still_parses_its_declared_length` still pinned the head-padded short read that fix removed. The `Reserved_79` option declaring `length=12` over 6 real octets now reports `aabbccddeeff00000000` where the test expected `00000000aabbccddeeff`, so both subtests -- `declared_length=12` and `=32` -- failed on that one assertion while the 17 other cases in the file stayed green: the parse itself never changed, only which end the synthesised zeros sit at. The expectation is inverted, and the docstring above it -- which said the short read was *left*-padded and described the value as four zero octets followed by the six real ones -- is corrected to match, since a docstring that contradicts its own assertion is how the stale expectation survived in the first place. The inputs do discriminate: `trailing` is non-zero and the pad width is 4 and 24, so neither subtest would hold under the other order. #621 left this file alone deliberately, because #612 owned it at the time, and merged two minutes ahead of the cross-review verdict that named it (#604, #621). Preceded by `1.5.0a1` (2026-09-15), `1.5.0b1` and `1.5.0b2` (both 2026-09-18) and `1.5.0b3` (2026-09-19), all published as prereleases and so resolved only by `pip install --pre`. `1.5.0b1` half-shipped: the tag, the GitHub release and the Conda deployments landed, but PyPI rejected the wheel because `twine check` found a Sphinx-only `:mod:` role in `README.rst`, which `pyproject.toml` declares as the dynamic long description. `1.5.0b2` is what reshipped it -- the release workflow is version-driven, so an existing version cannot republish -- and `1.5.0b3` followed the CI change that stops a TestPyPI outage from costing a release its wheels (#497, #498). diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index d827348f0..5ae643ef5 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -1004,6 +1004,22 @@ pull requests between #326 and #509. own Markdown API rather than assumed: the ladder comes back as four list items each nesting three, which is what #613 had to repair in the 1.4 file when a stray list marker collapsed the whole document into one nested item (#624). +* **Fixed** -- ``main`` went red the moment #604's ``ljust()`` landed, because + ``TCPUDPUnitTests.test_a_truncated_option_still_parses_its_declared_length`` + still pinned the head-padded short read that fix removed. The + ``Reserved_79`` option declaring ``length=12`` over 6 real octets now reports + ``aabbccddeeff00000000`` where the test expected ``00000000aabbccddeeff``, so + both subtests -- ``declared_length=12`` and ``=32`` -- failed on that one + assertion while the 17 other cases in the file stayed green: the parse itself + never changed, only which end the synthesised zeros sit at. The expectation + is inverted, and the docstring above it -- which said the short read was + *left*-padded and described the value as four zero octets followed by the six + real ones -- is corrected to match, since a docstring that contradicts its + own assertion is how the stale expectation survived in the first place. The + inputs do discriminate: ``trailing`` is non-zero and the pad width is 4 and + 24, so neither subtest would hold under the other order. #621 left this file + alone deliberately, because #612 owned it at the time, and merged two minutes + ahead of the cross-review verdict that named it (#604, #621). Preceded by ``1.5.0a1`` (2026-09-15), ``1.5.0b1`` and ``1.5.0b2`` (both 2026-09-18) and ``1.5.0b3`` (2026-09-19), all published as prereleases and so diff --git a/tests/protocols/transport/test_tcp_udp_unit.py b/tests/protocols/transport/test_tcp_udp_unit.py index ad5229d64..0549731ce 100644 --- a/tests/protocols/transport/test_tcp_udp_unit.py +++ b/tests/protocols/transport/test_tcp_udp_unit.py @@ -1371,10 +1371,13 @@ def test_a_truncated_option_still_parses_its_declared_length(self) -> None: ``data`` field (``BytesField(length=lambda pkt: pkt['length'] - 2)``, 10 octets here) for more than the 6 octets actually behind it. :meth:`FieldBase.unpack ` - left-pads the short read with zero octets rather than raising, so the + pads the short read with zero octets rather than raising, so the option parses with its declared ``length`` intact and a ``data`` value - of four zero octets followed by the six real ones. That is reachable - here because :meth:`~pcapkit.protocols.transport.tcp.TCP._read_tcp_options` + of the six real octets followed by four zero ones. The zeros go on the + *tail*, where the octets that were never read would have been; before + #604 they were placed at the front, which for a numeric field corrupted + the value outright. That is reachable here because + :meth:`~pcapkit.protocols.transport.tcp.TCP._read_tcp_options` sizes each parsed option by ``len(schema)`` -- what it actually consumed (8 octets) -- rather than by its self-reported ``length``, so its own ``TCP: invalid format`` threshold never sees the shortfall -- @@ -1383,8 +1386,8 @@ def test_a_truncated_option_still_parses_its_declared_length(self) -> None: ``IPv4UnitTests.test_a_truncated_option_still_parses_its_declared_length``). ``length=32`` (30 octets of data wanted, still only 6 available) is checked alongside 12 because the pad width tracks ``length - 2``: 32 - yields 24 zero octets where 12 yields 4, pinning that the padding - scales with the declared length rather than being a fixed 4. + yields 24 trailing zero octets where 12 yields 4, pinning that the + padding scales with the declared length rather than being a fixed 4. """ import struct @@ -1412,7 +1415,15 @@ def segment(data_offset: 'int', options: 'bytes') -> 'bytes': ) unassigned = next(opt for code, opt in proto.info.options.items(multi=True) if code == custom) - self.assertEqual(unassigned.data, b'\x00' * zeroes + trailing) + # The six octets actually behind the option come first, where + # they were read, and the zeros synthesised for the ones that + # were not follow them. Before #604 they arrived the other way + # round; since ``trailing`` is non-zero and ``zeroes`` is + # non-zero for both declared lengths, this assertion tells the + # two orders apart rather than holding for either. See #604 and + # ``FieldBaseShortReadPaddingSideTests`` in + # ``tests/corekit/test_fields_field.py``. + self.assertEqual(unassigned.data, trailing + b'\x00' * zeroes) self.assertEqual(bytes(proto.__header__), raw) def test_unregistered_option_kind_does_not_mutate_the_class_registry(self) -> None: