Add Hotline codec fixtures and tests for wireframe_testing#487
Conversation
Introduce a new module providing fixture functions that generate valid, invalid, truncated, and correlated Hotline-framed wire bytes for use in tests. These fixtures allow test authors to easily produce raw wire data for codec exercise, including error conditions that are hard to manually craft. Includes: - Valid frames with specified payloads and transaction IDs - Oversized payload frames that trigger decoder errors - Mismatched total_size frames to test error handling - Truncated headers and payloads producing decode errors - Multi-frame sequences sharing or incrementing transaction IDs Supports comprehensive unit and BDD integration tests verifying correct codec behaviours when decoding these fixtures. Also adds documentation updates and marks roadmap item 9.7.2 as done. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
WalkthroughSummarise addition of a public codec fixture suite to wireframe_testing that emits valid, invalid and edge-case Hotline-framed wire bytes and typed frames; adds integration tests, BDD scaffolding, and documentation updates (including a duplicate ADR subsection insertion). Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideAdds a Hotline-specific codec fixtures module to wireframe_testing, exposes its helpers through the crate’s public surface, and introduces both integration and BDD-style tests plus documentation/ADR/roadmap updates describing and validating these fixtures. Sequence diagram for using codec fixtures with HotlineFrameCodecsequenceDiagram
actor TestAuthor
participant WireframeTesting as wireframe_testing
participant CodecFixtures as codec_fixtures
participant CodecExt as codec_ext
participant HotlineCodec as HotlineFrameCodec
participant Decoder as HotlineDecoder
TestAuthor->>WireframeTesting: valid_hotline_wire(payload, transaction_id)
WireframeTesting->>CodecFixtures: valid_hotline_wire(payload, transaction_id)
CodecFixtures-->>WireframeTesting: wire_bytes
WireframeTesting-->>TestAuthor: wire_bytes
TestAuthor->>HotlineCodec: new(max_frame_length)
HotlineCodec-->>TestAuthor: codec
TestAuthor->>WireframeTesting: decode_frames_with_codec(codec, wire_bytes)
WireframeTesting->>CodecExt: decode_frames_with_codec(codec, wire_bytes)
CodecExt->>HotlineCodec: decoder()
HotlineCodec-->>CodecExt: Decoder
loop decode_loop
CodecExt->>Decoder: decode(buffer)
alt complete_valid_frame
Decoder-->>CodecExt: Ok(frame)
CodecExt->>CodecExt: push frame into frames
else needs_more_bytes
Decoder-->>CodecExt: Ok(None)
CodecExt->>CodecExt: break
else invalid_or_oversized
Decoder-->>CodecExt: Err(error)
CodecExt-->>WireframeTesting: Err(error)
WireframeTesting-->>TestAuthor: Err(error)
WireframeTesting->>WireframeTesting: return
end
end
CodecExt->>Decoder: decode_eof(buffer)
alt bytes_remaining
Decoder-->>CodecExt: Err(bytes_remaining_error)
CodecExt-->>WireframeTesting: Err(bytes_remaining_error)
WireframeTesting-->>TestAuthor: Err(bytes_remaining_error)
else clean_eof
Decoder-->>CodecExt: Ok(None)
CodecExt-->>WireframeTesting: Ok(frames)
WireframeTesting-->>TestAuthor: Ok(frames)
end
Class diagram for Hotline codec fixtures moduleclassDiagram
direction LR
class HotlineFrameCodec {
+new(max_frame_length usize) HotlineFrameCodec
+decoder() Decoder
+encoder() Encoder
+frame_payload(frame HotlineFrame) Bytes
+wrap_payload(payload Bytes) HotlineFrame
+correlation_id(frame HotlineFrame) u32
+max_frame_length() usize
}
class HotlineFrame {
+data_size u32
+total_size u32
+transaction_id u32
+reserved [u8]
+payload Bytes
}
class CodecFixtures {
<<utility>>
+valid_hotline_wire(payload u8[], transaction_id u32) Vec_u8
+valid_hotline_frame(payload u8[], transaction_id u32) HotlineFrame
+oversized_hotline_wire(max_frame_length usize) Vec_u8
+mismatched_total_size_wire(payload u8[]) Vec_u8
+truncated_hotline_header() Vec_u8
+truncated_hotline_payload(payload_len usize) Vec_u8
+correlated_hotline_wire(transaction_id u32, payloads u8[][]) Vec_u8
+sequential_hotline_wire(base_transaction_id u32, payloads u8[][]) Vec_u8
-build_hotline_wire(payload u8[], transaction_id u32, data_size usize, correct_total_size bool) Vec_u8
-u32_from_usize(value usize) u32
}
class CodecExtHelpers {
+decode_frames_with_codec(codec HotlineFrameCodec, wire Vec_u8) Result~Vec_HotlineFrame,Error~
+encode_payloads_with_codec(codec HotlineFrameCodec, payloads Bytes[]) Result~Vec_u8,Error~
+extract_payloads(frames HotlineFrame[]) Bytes[]
}
CodecFixtures ..> HotlineFrameCodec : uses
CodecFixtures ..> HotlineFrame : constructs
CodecExtHelpers ..> HotlineFrameCodec : uses
CodecExtHelpers ..> HotlineFrame : returns
class WireframeTestingCrate {
+correlated_hotline_wire
+mismatched_total_size_wire
+oversized_hotline_wire
+sequential_hotline_wire
+truncated_hotline_header
+truncated_hotline_payload
+valid_hotline_frame
+valid_hotline_wire
+decode_frames_with_codec
}
WireframeTestingCrate ..> CodecFixtures : reexports
WireframeTestingCrate ..> CodecExtHelpers : reexports
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file fn sequential_frames_have_incrementing_ids() -> io::Result<()> {
let wire = sequential_hotline_wire(10, &[b"x", b"y", b"z"]);
let codec = hotline_codec();
let frames = decode_frames_with_codec(&codec, wire)?;
if frames.len() != 3 {
return Err(io::Error::other(format!(
"expected 3 frames, got {}",
frames.len()
)));
}
let expected_ids: &[u32] = &[10, 11, 12];
for (i, (frame, expected_id)) in frames.iter().zip(expected_ids.iter()).enumerate() {
if frame.transaction_id != *expected_id {
return Err(io::Error::other(format!(
"frame {i}: expected transaction_id {expected_id}, got {}",
frame.transaction_id
)));
}
}
Ok(())
}❌ New issue: Code Duplication |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests/fixtures/codec_fixtures.rs Comment on file pub fn verify_invalid_data_error(&self) -> TestResult {
let err = self
.decode_error
.as_ref()
.ok_or("expected a decode error but decoding succeeded")?;
if !err.to_string().contains("payload too large") {
return Err(format!("expected 'payload too large' error, got: {err}").into());
}
Ok(())
}❌ New issue: Code Duplication |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: wireframe_testing/src/helpers/codec_fixtures.rs Comment on file //! Codec fixture functions for generating valid and invalid Hotline-framed
❌ New issue: Primitive Obsession |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
- Added `TransactionId`, `MaxFrameLength`, and `PayloadLength` newtypes for hotline frame parameters for improved type safety and clarity. - Updated fixture functions to accept these newtypes instead of raw primitives. - Refactored decode test helpers to use assertion helper functions simplifying test code and error validation. - Cleaned up decoding tests by consolidating repeated error string checks into reusable assertions. - Updated exports to include new helper types. - Improved documentation and consistency in wireframe testing helpers. These changes consolidate frame codec fixture logic, improve readability, and ease future maintenance and extension of tests and helpers. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file fn sequential_frames_have_incrementing_ids() -> io::Result<()> {
let wire = sequential_hotline_wire(10, &[b"x", b"y", b"z"]);
let codec = hotline_codec();
let frames = decode_frames_with_codec(&codec, wire)?;
assert_frame_count(&frames, 3)?;
let expected_ids: &[u32] = &[10, 11, 12];
for (i, (frame, expected_id)) in frames.iter().zip(expected_ids.iter()).enumerate() {
if frame.transaction_id != *expected_id {
return Err(io::Error::other(format!(
"frame {i}: expected transaction_id {expected_id}, got {}",
frame.transaction_id
)));
}
}
Ok(())
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
…r function Introduced `assert_transaction_ids` helper to verify transaction IDs in frames. Replaced repetitive individual frame ID assertions with this helper in existing tests for improved code clarity and reuse. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b33d12e0f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/9-7-2-codec-fixtures-in-wireframe-testing.md`:
- Line 431: Replace the British spelling "parameterised" with the Oxford/en-GB
"-ize" form "parameterized" in the sentence containing "raw-byte fixtures rather
than generic `FrameCodec`-parameterised generators" so the documentation follows
the project's en-GB-oxendict guideline; update that token in the
docs/execplans/9-7-2-codec-fixtures-in-wireframe-testing.md content to read
"`FrameCodec`-parameterized generators".
In `@tests/codec_fixtures.rs`:
- Around line 117-137: Replace the manual length check in assert_transaction_ids
with a call to the existing helper assert_frame_count to avoid duplication: call
assert_frame_count(frames, expected_ids.len()) (using the expected_ids.len()
converted as needed) and propagate its Result; then keep the iteration over
frames.zip(expected_ids) to validate transaction_id mismatches and return errors
from the loop as before. Ensure you reference the functions
assert_transaction_ids and assert_frame_count when editing.
In `@tests/fixtures/codec_fixtures.rs`:
- Around line 53-104: Extract the repeated match-and-store logic into a private
helper fn decode_fixture(&mut self, wire: Vec<u8>) -> TestResult that obtains
the codec via self.codec.as_ref().ok_or("codec not configured")? and calls
wireframe_testing::decode_frames_with_codec(codec, wire) storing Ok(frames) into
self.decoded_frames and Err(e) into self.decode_error, then return Ok(()).
Replace the bodies of decode_valid_fixture, decode_oversized_fixture,
decode_truncated_fixture, and decode_correlated_fixture to only build the
appropriate wire (e.g. valid_hotline_wire,
oversized_hotline_wire(codec.max_frame_length()), truncated_hotline_header(),
correlated_hotline_wire(...)) and call self.decode_fixture(wire). Ensure helper
is private and referenced by those methods.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
docs/adr-004-pluggable-protocol-codecs.mddocs/execplans/9-7-2-codec-fixtures-in-wireframe-testing.mddocs/roadmap.mddocs/users-guide.mdtests/codec_fixtures.rstests/features/codec_fixtures.featuretests/fixtures/codec_fixtures.rstests/fixtures/mod.rstests/scenarios/codec_fixtures_scenarios.rstests/scenarios/mod.rstests/steps/codec_fixtures_steps.rstests/steps/mod.rswireframe_testing/src/helpers.rswireframe_testing/src/helpers/codec_fixtures.rswireframe_testing/src/lib.rs
Refactored the codec fixture decoding methods in tests/fixtures/codec_fixtures.rs to delegate to a single helper method decode_fixture, reducing code duplication and improving clarity. Also fixed frame count assertion in tests/codec_fixtures.rs by using assert_frame_count helper. Minor fixes include using wrapping_add for transaction ID calculation in wireframe_testing helpers and a typo correction in docs. This change improves maintainability without altering functionality. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/9-7-2-codec-fixtures-in-wireframe-testing.md`:
- Around line 352-358: Update the docs to match the actual decode_eof error
wording: in the examples/tests described by
truncated_header_produces_trailing_bytes_error and
truncated_payload_produces_trailing_bytes_error (which call
truncated_hotline_header and truncated_hotline_payload(100) and then attempt
decode), change the asserted error text from "trailing" to "bytes remaining" to
reflect the decode_eof implementation; make the same replacement in the other
referenced block (the section around the second occurrence, currently lines
391-395) so all documentation matches the current behavior of decode_eof.
- Around line 490-526: Update the documented function signatures to match the
newtype-based public API: change parameters typed as primitives to accept impl
Into<TransactionId> for functions taking transaction_id (valid_hotline_wire,
valid_hotline_frame, correlated_hotline_wire, sequential_hotline_wire), impl
Into<MaxFrameLength> for oversized_hotline_wire, and impl Into<PayloadLength>
for truncated_hotline_payload; leave return types (e.g., HotlineFrame) and
payload slice shapes intact and keep truncated_hotline_header() as-is. Ensure
the doc examples and signature listings use these Into<> types instead of raw
u32/usize so the docs reflect the shipped API.
In `@tests/fixtures/codec_fixtures.rs`:
- Around line 87-95: In decode_fixture, reset the fixture state at the start by
clearing self.decoded_frames and setting self.decode_error = None before calling
wireframe_testing::decode_frames_with_codec; keep the existing codec check (let
codec = self.codec.as_ref().ok_or(...)?), then run the match and assign into
those fields as now so each decode starts with deterministic empty state and no
stale results from prior runs.
In `@wireframe_testing/src/helpers/codec_fixtures.rs`:
- Around line 198-204: The function truncated_hotline_payload currently allows a
claimed payload_len of 0 which yields a header-only (non-truncated) frame;
adjust truncated_hotline_payload to enforce a minimum claimed payload length of
1 by clamping payload_len (or replacing 0 with 1) before computing data_size,
total_size, half_payload and allocating buf (preserve use of variables
payload_len, data_size, total_size, half_payload, HEADER_LEN and the existing
return type) so the generated buffer always represents a truncated payload.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/execplans/9-7-2-codec-fixtures-in-wireframe-testing.mdtests/codec_fixtures.rstests/fixtures/codec_fixtures.rswireframe_testing/src/helpers/codec_fixtures.rs
…rs and improve test state reset - Changed multiple fixture helper function parameters to accept `impl Into<T>` for flexibility. - Updated decoding fixture method to clear previous frames and errors before decoding. - Adjusted documentation and comments for clarity, including error messages wording in scenarios. - Ensured truncated payload fixture always claims at least 1 byte to guarantee truncation. These improvements enhance API ergonomics and test reliability. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/execplans/9-7-2-codec-fixtures-in-wireframe-testing.md (2)
277-319:⚠️ Potential issue | 🟠 MajorSynchronize documented interfaces with the shipped API.
Replace primitive signatures with
impl Into<TransactionId>,impl Into<MaxFrameLength>, andimpl Into<PayloadLength>in Stage A. Renameverify_trailing_bytes_error()toverify_bytes_remaining_error()in the Stage C world-method list.As per coding guidelines: "Use markdown files within the
docs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions."Also applies to: 402-408
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/9-7-2-codec-fixtures-in-wireframe-testing.md` around lines 277 - 319, Update the documented function signatures in Stage A to match the shipped API by replacing primitive parameter types with Into wrappers (e.g., change signatures to use impl Into<TransactionId>, impl Into<MaxFrameLength>, and impl Into<PayloadLength> for functions like valid_hotline_wire, valid_hotline_frame, oversized_hotline_wire, mismatched_total_size_wire, truncated_hotline_header, truncated_hotline_payload, correlated_hotline_wire, and sequential_hotline_wire), and in Stage C rename the listed world-method verify_trailing_bytes_error() to verify_bytes_remaining_error() so the docs and tests match the actual codebase.
87-95:⚠️ Potential issue | 🟡 MinorCorrect the truncated-frame error-path description.
State that truncated inputs surface via
decode_eofwith"bytes remaining on stream". Remove attribution to the trailing-bytes check in this section.As per coding guidelines: "Ensure that any API or behavioural changes are reflected in the documentation in
docs/."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/9-7-2-codec-fixtures-in-wireframe-testing.md` around lines 87 - 95, Update the docs to state that truncated-frame inputs from the Hotline decoder surface via decode_eof with an error message "bytes remaining on stream" (not via the trailing-bytes check), remove the sentence attributing truncated inputs to the trailing-bytes path, and explicitly note that decode_frames_with_codec will report these truncations through decode_eof rather than the decoder returning Err; reference the Hotline decoder behavior (Ok(None) on truncated data), decode_frames_with_codec, and the fixture API so BDD tests and fixtures assert on the decode_eof "bytes remaining on stream" error path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wireframe_testing/src/helpers/codec_fixtures.rs`:
- Around line 120-123: Replace unchecked usize additions with saturating
arithmetic to avoid overflow when constructing fixtures: in
oversized_hotline_wire (using MaxFrameLength) change oversized_len =
max_frame_length + 1 to use saturating_add(1); in truncated_hotline_payload
(using PayloadLength and HEADER_LEN) use payload_len.saturating_add(HEADER_LEN)
before calling u32_from_usize; and in build_hotline_wire compute total_size with
data_size.saturating_add(HEADER_LEN) (and for the off-by-one branch apply
another saturating_add(1)) before converting with u32_from_usize. Update all
occurrences in those functions to use saturating_add on usize operands (e.g.,
MAX/sizes) so fixture construction remains panic-free.
---
Duplicate comments:
In `@docs/execplans/9-7-2-codec-fixtures-in-wireframe-testing.md`:
- Around line 277-319: Update the documented function signatures in Stage A to
match the shipped API by replacing primitive parameter types with Into wrappers
(e.g., change signatures to use impl Into<TransactionId>, impl
Into<MaxFrameLength>, and impl Into<PayloadLength> for functions like
valid_hotline_wire, valid_hotline_frame, oversized_hotline_wire,
mismatched_total_size_wire, truncated_hotline_header, truncated_hotline_payload,
correlated_hotline_wire, and sequential_hotline_wire), and in Stage C rename the
listed world-method verify_trailing_bytes_error() to
verify_bytes_remaining_error() so the docs and tests match the actual codebase.
- Around line 87-95: Update the docs to state that truncated-frame inputs from
the Hotline decoder surface via decode_eof with an error message "bytes
remaining on stream" (not via the trailing-bytes check), remove the sentence
attributing truncated inputs to the trailing-bytes path, and explicitly note
that decode_frames_with_codec will report these truncations through decode_eof
rather than the decoder returning Err; reference the Hotline decoder behavior
(Ok(None) on truncated data), decode_frames_with_codec, and the fixture API so
BDD tests and fixtures assert on the decode_eof "bytes remaining on stream"
error path.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
docs/execplans/9-7-2-codec-fixtures-in-wireframe-testing.mdtests/fixtures/codec_fixtures.rswireframe_testing/src/helpers/codec_fixtures.rs
…ndling Revise documentation in codec_fixtures to reflect changes in error handling, parameter types (using Into traits), and fixture API descriptions. Clarify behavior on truncated frame decoding and error reporting via decode_eof. Improve explanations of fixture functions and update terminology for error paths to enhance clarity and accuracy in the docs. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Summary
Changes
Why this approach
Testing plan
Notes
📎 Task: https://www.devboxer.com/task/8913b8f3-a3be-4300-9a1a-7464825f2d72