test: port the property and fuzz suites from #1 to the current API - #16
Merged
JustinKovacich merged 1 commit intoSep 10, 2026
Merged
Conversation
JustinKovacich
added this pull request to stack #14
September 10, 2026 13:26
JustinKovacich
marked this pull request as ready for review
September 10, 2026 13:27
simple_doip#1 carries 26 property tests written against the pre-`no_std` `write`/`read` API that 0.2.0 removed, on a branch whose merge base predates that migration, the error-taxonomy refactor and the 0.4.0 server break. The properties were sound; only the calls were stale. Rebasing that branch would have resolved textually and then failed to compile, so the cases are ported instead. 24 of the 26 land, all passing on the first run. The two that do not are byte roundtrips through serde, and this crate has no serde dependency to roundtrip through. They live in `tests/property.rs` rather than in `#[cfg(test)]` modules inside `src/` as the original did. proptest needs `std`, the library is `no_std`, and an integration test target gets `std` without any conditional-compilation gymnastics -- at the cost of only exercising the public API, which is all these properties touch anyway. What they check that `golden_vectors.rs` does not: the golden fixtures pin the exact bytes the crate emits, so they catch the wire format changing. These check that `encode` and `decode` agree with each other across the whole input space, which catches a field written in one order and read in another. Neither finds a misreading of the standard that both directions share symmetrically. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JustinKovacich
force-pushed
the
test/port-property-and-fuzz-suites
branch
from
September 10, 2026 13:54
4564ace to
befec53
Compare
gavin-dunlap-luminar
approved these changes
Sep 10, 2026
JustinKovacich
added a commit
that referenced
this pull request
Sep 10, 2026
…sage (#17) **Stacked on #16 — merge order: #11 → #12 → #13 → #16 → this.** ## Issue URL Closes #15. ## What `Message::encode` could emit a frame that `Message::decode` rejects. `decode` takes exactly `header.payload_length` bytes and hands them to `Payload::decode`, which is not required to consume all of them. The decoded `Message` keeps the header verbatim, declared length included, and `encode` wrote that stale field beside a payload of its real size. Note that `encoded_size()` already disagreed with the header being written — it returns `Header::SIZE + payload.encoded_size()`, not the declared length. ```rust // A NACK body is one byte. This header claims five. let framed = [0x02, 0xFD, 0, 0, 0, 0, 0, 0x05, 0x03, 0, 0, 0, 0]; let (msg, _) = Message::decode(&framed).unwrap(); // accepted assert_eq!(msg.header.payload_length, 5); // preserved // re-encode -> 9 bytes, header still claims 5 Message::decode(&encode(&msg)); // Err(Incomplete { needed: 5, available: 1 }) ``` Anything that decodes a frame and re-emits it — a proxy, a replay tool, a logging fake, a test harness echoing what it received — was turning a malformed-but-accepted frame into a corrupt one on the wire. This crate's own `MessageCodec` encoder is on that path. ## The fix `encode` builds its header from `payload.encoded_size()` rather than trusting `self.header.payload_length`, so an encoded frame is always self-consistent regardless of how lenient `decode` is. **No input that is accepted today starts being rejected.** The consequence, documented on the method: for a frame that arrived with a mismatched declared length, `decode(encode(m)).header.payload_length` is the payload's real size rather than the length it arrived with. That is the point — but it is a visible behavior change, so it's in the changelog. A **well-formed frame is byte-identical**, which is why all 11 golden vectors still pass unchanged. That's also pinned as a test. `MessageError::PayloadTooLarge` covers the one fallible step — a payload too large for the `u32` length field, unreachable for a frame off the wire whose length was itself a `u32`. `MessageError` is `#[non_exhaustive]`, so adding it breaks nothing. ## Why not make `decode` strict instead That is the standards-correct complement — ISO 13400-2 has an entity answer an invalid payload length with NACK `0x04`, and `MessageError::PayloadLengthTooShort` sits unused for exactly this. But it's a redesign, not a fix: `Payload::decode` would have to report unconsumed bytes, and the identification requests **deliberately** discard their EID/VIN body (`ARCHITECTURE.md` §7.6), so a `0x0002` request carrying its six EID bytes would start being rejected outright rather than declined — breaking the UDP responder path. Recorded in `ARCHITECTURE.md` §7.6 as deferred rather than dropped. ## Testing | | | |---|---| | `tests/encode_consistency.rs` | 3 regression cases: overlong length on a fixed payload, nonzero length on a unit payload, and a well-formed frame encoding to the bytes it came from | | golden vectors | 11/11 pass **unchanged** — the fix cannot touch a frame whose declared length was already right | | `fuzz_roundtrip`, skip removed, idempotence asserted | **21,033,263 executions clean** | | other three fuzz targets | 6.4M / 8.1M / 1.7M clean | | full test suite, all features and none | pass | | clippy `--all-targets --all-features -Dclippy::pedantic`, and `--no-default-features` | clean | | `cargo doc` with `-D warnings`, `cargo fmt`, `pre-commit`, `cargo publish --dry-run`, MSRV 1.88 | pass | The fuzz target now asserts payload and payload-type equality plus **idempotence** (encode, decode, encode again → identical bytes) rather than whole-`Message` equality. Asserting full equality would assert the bug back into existence, since the declared length legitimately normalizes; idempotence buys back the field-order asymmetry detection that equality was providing. ## Also Corrects `PayloadLengthTooShort`'s message, which read "does match" where it meant "does not match" — a user-visible error string. ## Release bump: 0.6.0 This PR sits at the top of the stack, so it also carries `chore(release): v0.6.0` — the version the whole stack (#11 → #17) publishes as. It follows the same pattern as 0.5.2, whose bump was made inside #10's branch rather than by `cargo release` on main. Nothing in the stack breaks a signature: `MessageError` is `#[non_exhaustive]`, so `PayloadTooLarge` is additive, and `ClientConnectionInfo::logical_address` keeps its type and only starts carrying a real value. The bump is for the encode change in this PR — a caller that set a mismatched `payload_length` deliberately (a negative-test fake, a corpus generator, a proxy replaying what it saw) stops being able to emit that frame, with no compiler diagnostic anywhere. The CHANGELOG entry is marked **Breaking:** so the version and the section header tell the same story. `v0.5.2` is now tagged at the #10 merge on `main` (`304d014`), so that section has a comparison range and the release links run `v0.5.2...v0.6.0`. ## Review status Not reviewed by anyone yet. Draft. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Stacked on #13 — merge order: #11 → #12 → #13 → this.
Issue URL
Closes #1 (supersedes it — see below). Files #15.
What
Brings simple_doip#1's test suites forward instead of rebasing that branch, and
turns on the two CI gates that were off for want of them.
#1's merge base is 2026-04-03, before the
no_stdmigration, theerror-taxonomy refactor and the 0.4.0 server break. Its 26 property tests and 3
of its 4 fuzz targets drive
Message::read,Message::writeandPayload::read— none of which exist onmain, all removed in 0.2.0. Arebase resolves textually (12 conflicts on the first of 7 commits) and then
fails to compile. The properties were sound; only the calls were stale.
tests/property.rsEncode/Decode, all passing on the first runfuzz/.github/templatesmain.ymlrun-property-testsandrun-fuzz-testsboth on24 of 26 properties land. The two that don't are byte round trips through
serde, and this crate has no serde dependency.
They live in
tests/property.rsrather than#[cfg(test)]modules insidesrc/as the original did: proptest needsstd, the library isno_std, andan integration target gets
stdwith no conditional-compilation gymnastics.The cost is only reaching the public API, which is all these properties touch.
What they add over
golden_vectors.rsThe golden fixtures pin the exact bytes the crate emits, so they catch the wire
format changing. These check
encodeanddecodeagree with each otheracross the whole input space, which catches a field written in one order and
read in another. Neither finds a misreading of the standard that both
directions share symmetrically — that's what the fixtures are for.
fuzz_roundtripfound a real bug in under a second#15:
Message::encodecan emit a frame thatMessage::decoderejects.decodetakes exactlyheader.payload_lengthbytes and letsPayload::decodeconsume fewer without complaint; the decoded
Messagekeeps the declaredlength;
encodethen writes that stale length beside a payload of its realsize. A NACK frame declaring 5 body bytes and carrying 1 decodes fine,
re-encodes to 9 bytes with the header still claiming 5, and fails to re-decode
with
Incomplete { needed: 5, available: 1 }.The target skips that specific shape, with #15 referenced at the check, so
it keeps hunting field-order asymmetries without asserting a property the crate
violates today. The skip comes out with the fix, which is stacked on this PR.
Testing
cargo test --all-features/--no-default-featurescargo nextest run -E 'test(~prop_)'exit 4failure modecargo fuzz build(real cargo-fuzz, nightly)clippy --all-targets --all-features -Dclippy::pedanticclippy --no-default-features -Dclippy::pedanticcargo fmt --all --check,pre-commit run --all-filescargo publish --dry-runfuzz/does not enter the packaged crateunit-test-filterstaysall()rather than excludingprop_: the unit jobmeasures coverage, and coverage should describe the whole suite. The property
job re-runs the same 24 under their own name for a readable signal.
Not included
The PR description lint the sibling repos pair with the templates. It
requires
## Issue URLand## Testingsections and would fail everycurrently open PR, including this one's stack-mates. A
No Issuelabel nowexists as its escape hatch; the job belongs in a follow-up once bodies conform.
On #1
Its
main.ymlpredatesrust_workflow@v1by six weeks, so it wasn't amisjudgment — the better option didn't exist. Its scaffolding was
independently written in #13 before I'd read his closely enough, which is on
me. What was uniquely valuable was the tests, and they're here.
Recommend closing #1 with a pointer to this PR rather than leaving it to rot;
@gavin-dunlap-luminar is credited in the commit and in
tests/property.rs.Note on the commit
This is one commit, not the four its message describes — everything was already
staged when I wrote the first one, so it swept the lot. Since the repo
squash-merges with the body taken from the PR description, the rationale that
would have been in those messages is above instead.
Review status
Not reviewed by anyone yet. Draft.