Conversation
Contributor
Reviewer's GuideAdds new wireframe_testing helpers to drive apps with partial frames and fragment-based payloads (including a combined partial-fragment mode), wires them into the public API, and exercises them via integration/BDD tests and updated documentation/roadmap for item 8.5.1. Sequence diagram for drive_with_partial_fragments helpersequenceDiagram
actor TestCode
participant WireframeTesting as wireframe_testing
participant FragmentDriveHelpers as fragment_drive
participant PartialFrameHelpers as partial_frame
participant Fragmenter
participant FragmentPayloadEncoding as fragment_payload
participant Codec
participant DriveInternal as drive_internal
participant WireframeApp
TestCode->>WireframeTesting: use drive_with_partial_fragments
TestCode->>FragmentDriveHelpers: drive_with_partial_fragments(app, codec, fragmenter, payload, chunk_size)
FragmentDriveHelpers->>Fragmenter: fragment_bytes(payload)
Fragmenter-->>FragmentDriveHelpers: FragmentBatch
loop for_each_fragment
FragmentDriveHelpers->>FragmentPayloadEncoding: encode_fragment_payload(header, body)
FragmentPayloadEncoding-->>FragmentDriveHelpers: fragment_payload_bytes
end
FragmentDriveHelpers->>Codec: encode_payloads_with_codec(fragment_payloads)
Codec-->>FragmentDriveHelpers: encoded_frames_bytes
FragmentDriveHelpers->>PartialFrameHelpers: drive_chunked_internal(handler, wire_bytes, chunk_size, DEFAULT_CAPACITY)
activate PartialFrameHelpers
PartialFrameHelpers->>WireframeApp: handle_connection(DuplexStream)
activate WireframeApp
Note over WireframeApp: App reads partial frames via codec
WireframeApp-->>PartialFrameHelpers: writes response frames
deactivate WireframeApp
PartialFrameHelpers-->>FragmentDriveHelpers: raw_response_bytes
deactivate PartialFrameHelpers
FragmentDriveHelpers->>Codec: decode_frames_with_codec(raw_response_bytes)
Codec-->>FragmentDriveHelpers: response_frames
FragmentDriveHelpers->>Codec: extract_payloads(response_frames)
Codec-->>FragmentDriveHelpers: response_payloads
FragmentDriveHelpers-->>TestCode: io::Result<Vec<Vec<u8>>> response_payloads
Class diagram for new partial_frame and fragment_drive helpersclassDiagram
class WireframeApp {
}
class FrameCodec {
}
class Fragmenter {
fragment_bytes(payload: Vec_u8_) Result_FragmentBatch_
}
class ChunkConfig {
+NonZeroUsize chunk_size
+usize capacity
+new(chunk_size: NonZeroUsize) ChunkConfig
+with_capacity(chunk_size: NonZeroUsize, capacity: usize) ChunkConfig
}
class PartialFrameHelpers {
+drive_chunked_internal(server_fn: FnOnce_DuplexStream_Fut_, wire_bytes: Vec_u8_, chunk_size: NonZeroUsize, capacity: usize) io_Result_Vec_u8_
+drive_with_partial_frames(app: WireframeApp, codec: FrameCodec, payloads: Vec_Vec_u8__, chunk_size: NonZeroUsize) io_Result_Vec_Vec_u8__
+drive_with_partial_frames_with_capacity(app: WireframeApp, codec: FrameCodec, payloads: Vec_Vec_u8__, chunk_size: NonZeroUsize, capacity: usize) io_Result_Vec_Vec_u8__
+drive_with_partial_frames_mut(app: WireframeApp, codec: FrameCodec, payloads: Vec_Vec_u8__, chunk_size: NonZeroUsize) io_Result_Vec_Vec_u8__
+drive_with_partial_codec_frames(app: WireframeApp, codec: FrameCodec, payloads: Vec_Vec_u8__, chunk_size: NonZeroUsize) io_Result_Vec_Frame_
-drive_partial_frames_internal(handler: FnOnce_DuplexStream_Fut_, codec: FrameCodec, payloads: Vec_Vec_u8__, config: ChunkConfig) io_Result_Vec_Frame_
}
class FragmentRequest {
+Fragmenter~&~ fragmenter
+Vec_u8_ payload
+usize capacity
+new(fragmenter: Fragmenter~&~, payload: Vec_u8_) FragmentRequest
+with_capacity(capacity: usize) FragmentRequest
}
class FragmentDriveHelpers {
+drive_with_fragments(app: WireframeApp, codec: FrameCodec, fragmenter: Fragmenter~&~, payload: Vec_u8_) io_Result_Vec_Vec_u8__
+drive_with_fragments_with_capacity(app: WireframeApp, codec: FrameCodec, fragmenter: Fragmenter~&~, payload: Vec_u8_, capacity: usize) io_Result_Vec_Vec_u8__
+drive_with_fragments_mut(app: WireframeApp~&~, codec: FrameCodec, fragmenter: Fragmenter~&~, payload: Vec_u8_) io_Result_Vec_Vec_u8__
+drive_with_fragment_frames(app: WireframeApp, codec: FrameCodec, fragmenter: Fragmenter~&~, payload: Vec_u8_) io_Result_Vec_Frame_
+drive_with_partial_fragments(app: WireframeApp, codec: FrameCodec, fragmenter: Fragmenter~&~, payload: Vec_u8_, chunk_size: NonZeroUsize) io_Result_Vec_Vec_u8__
-fragment_and_encode(fragmenter: Fragmenter~&~, payload: Vec_u8_) io_Result_Vec_Vec_u8__
-drive_fragments_internal(handler: FnOnce_DuplexStream_Fut_, codec: FrameCodec, request: FragmentRequest) io_Result_Vec_Frame_
}
class DriveInternal {
+drive_internal(handler: FnOnce_DuplexStream_Fut_, encoded_frames: Vec_Vec_u8__, capacity: usize) io_Result_Vec_u8_
}
class CodecExt {
+encode_payloads_with_codec(codec: FrameCodec, payloads: Vec_Vec_u8__) io_Result_Vec_Vec_u8__
+decode_frames_with_codec(codec: FrameCodec, raw: Vec_u8_) io_Result_Vec_Frame_
+extract_payloads(frames: Vec_Frame_) Vec_Vec_u8__
}
class FragmentPayloadEncoding {
+encode_fragment_payload(header: FragmentHeader, body: Vec_u8_) io_Result_Vec_u8_
}
class FragmentHeader {
}
PartialFrameHelpers ..> ChunkConfig
PartialFrameHelpers ..> CodecExt
PartialFrameHelpers ..> DriveInternal : uses_flattened_wire_bytes
PartialFrameHelpers ..> WireframeApp : drives
PartialFrameHelpers ..> FrameCodec : encodes_and_decodes
FragmentDriveHelpers ..> FragmentRequest
FragmentDriveHelpers ..> Fragmenter : uses_for_fragmentation
FragmentDriveHelpers ..> FragmentPayloadEncoding : encodes_fragments
FragmentDriveHelpers ..> CodecExt
FragmentDriveHelpers ..> DriveInternal : uses_for_transport
FragmentDriveHelpers ..> PartialFrameHelpers : uses_drive_chunked_internal
FragmentDriveHelpers ..> WireframeApp : drives
FragmentDriveHelpers ..> FrameCodec : encodes_and_decodes
FragmentRequest o--> Fragmenter
WireframeApp ..> FrameCodec : composed_with
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
WalkthroughAdds two new test helper modules ( Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Harness
participant Codec
participant Transport
participant App as In-Process App
participant Decoder
Test->>Codec: encode(payload)
Codec->>Test: encoded_bytes
Test->>Transport: write in chunks<br/>(configurable size)
Transport->>App: partial_frame
App->>App: buffer & process
Note over App: May require multiple<br/>partial reads
App->>Transport: response
Transport->>Decoder: read_response
Decoder->>Decoder: decode_frames
Decoder->>Test: Vec<Vec<u8>>
sequenceDiagram
participant Test as Test Harness
participant Fragmenter
participant Codec
participant Transport
participant App as In-Process App
participant Decoder
Test->>Fragmenter: fragment(payload)
Fragmenter->>Test: Vec<fragment>
Test->>Codec: encode(each fragment)
Codec->>Test: Vec<encoded_frame>
Test->>Transport: write frames<br/>(or chunked)
Transport->>App: frame/partial_frame
App->>App: reassemble & process
App->>Transport: response
Transport->>Decoder: read_response
Decoder->>Decoder: decode_frames
Decoder->>Test: Vec<Frame>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
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 |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@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/partial_frame_feeding.rs Comment on file async fn partial_frames_single_byte_chunks() -> io::Result<()> {
let codec = hotline_codec();
let app = build_echo_app(codec.clone())?;
let serialized = serialize_envelope(&[10, 20, 30])?;
let chunk = NonZeroUsize::new(1).ok_or_else(|| io::Error::other("non-zero"))?;
let payloads = drive_with_partial_frames(app, &codec, vec![serialized], chunk).await?;
if payloads.is_empty() {
return Err(io::Error::other("expected non-empty response payloads"));
}
Ok(())
}❌ New issue: Code Duplication |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Owner
Author
|
@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/fragment_drive.rs Comment on file async fn drive_fragments_internal<F, H, Fut>(
handler: H,
codec: &F,
fragmenter: &Fragmenter,
payload: Vec<u8>,
capacity: usize,
) -> io::Result<Vec<F::Frame>>
where
F: FrameCodec,
H: FnOnce(DuplexStream) -> Fut,
Fut: std::future::Future<Output = ()> + Send,
{
let fragment_payloads = fragment_and_encode(fragmenter, payload)?;
let encoded = encode_payloads_with_codec(codec, fragment_payloads)?;
let raw = drive_internal(handler, encoded, capacity).await?;
decode_frames_with_codec(codec, raw)
}❌ New issue: Excess Number of Function Arguments |
Owner
Author
|
@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/partial_frame.rs Comment on file async fn drive_partial_frames_internal<F, H, Fut>(
handler: H,
codec: &F,
payloads: Vec<Vec<u8>>,
chunk_size: NonZeroUsize,
capacity: usize,
) -> io::Result<Vec<F::Frame>>
where
F: FrameCodec,
H: FnOnce(DuplexStream) -> Fut,
Fut: std::future::Future<Output = ()> + Send,
{
let encoded = encode_payloads_with_codec(codec, payloads)?;
let wire_bytes: Vec<u8> = encoded.into_iter().flatten().collect();
let raw = drive_chunked_internal(handler, wire_bytes, chunk_size, capacity).await?;
decode_frames_with_codec(codec, raw)
}❌ New issue: Excess Number of Function Arguments |
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.
… into in-process app - Introduced chunked-write drivers to write codec-encoded payloads in configurable byte-sized chunks, simulating partial-frame network reads. - Added fragment-feeding drivers that fragment raw payloads, encode fragments, and feed them through WireframeApp. - Provided multiple driver variants: owned app, mutable app, with capacity, returning raw frames or payloads. - Established comprehensive BDD test suite and unit tests covering chunked and fragmented feeding scenarios. - Updated docs/users-guide.md with new "Feeding partial frames and fragments" section including usage examples. - Marked roadmap item 8.5.1 as done in docs/roadmap.md. - Registered modules and re-exported public APIs in wireframe_testing crate. These utilities enable testing realistic network conditions where frames arrive across multiple reads or are fragmented, improving codec and application robustness validation. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
devboxerhub
bot
force-pushed
the
add-partial-frame-feeding-utils-7ylyan
branch
from
01268cf to
385f50f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@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/partial_frame_feeding.rs Comment on file async fn partial_frames_single_byte_chunks() -> io::Result<()> {
let codec = hotline_codec();
let app = build_echo_app(codec.clone())?;
let serialized = serialize_envelope(&[10, 20, 30])?;
let chunk = NonZeroUsize::new(1).ok_or_else(|| io::Error::other("non-zero"))?;
let payloads = drive_with_partial_frames(app, &codec, vec![serialized], chunk).await?;
if payloads.is_empty() {
return Err(io::Error::other("expected non-empty response payloads"));
}
Ok(())
}❌ New issue: Code Duplication |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
The suggested refactoring reduces the duplication by parameterising the payload data and chunk size, whilst keeping the test cases clear and maintainable. If you'd like me to revise the approach or provide additional clarification on the fix, please let me know. Otherwise, you can use the AI agent prompt I provided earlier to apply the changes. |
…for config - Added ChunkConfig to combine chunk size and duplex buffer capacity in partial_frame. - Added FragmentRequest to bundle fragmenter, payload, and capacity in fragment_drive. - Updated relevant functions to use these structs for clearer and more flexible configuration. - Refactored partial frame tests for reusability using a helper test function. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 10 issues, and left some high level feedback:
- The doc example for
drive_chunked_internaluseswireframe_testing::helpers::partial_frame::drive_chunked_internal, but the function ispub(super)and the module path is private, so the example will not compile for crate users; either adjust the visibility or change the example to use a public helper instead. - In
PartialFrameFeedingWorld::drive_chunked_multipleyou build payloads withu8::try_from(i).unwrap_or(0), which silently maps indices >255 to 0; consider returning an error whenidoes not fit inu8to avoid surprising duplication in test payloads.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The doc example for `drive_chunked_internal` uses `wireframe_testing::helpers::partial_frame::drive_chunked_internal`, but the function is `pub(super)` and the module path is private, so the example will not compile for crate users; either adjust the visibility or change the example to use a public helper instead.
- In `PartialFrameFeedingWorld::drive_chunked_multiple` you build payloads with `u8::try_from(i).unwrap_or(0)`, which silently maps indices >255 to 0; consider returning an error when `i` does not fit in `u8` to avoid surprising duplication in test payloads.
## Individual Comments
### Comment 1
<location path="tests/partial_frame_feeding.rs" line_range="50-60" />
<code_context>
+// Chunked-write (partial frame) tests
+// ---------------------------------------------------------------------------
+
+async fn test_partial_frames_with_chunk(payload: &[u8], chunk_size: usize) -> io::Result<()> {
+ let codec = hotline_codec();
+ let app = build_echo_app(codec.clone())?;
+ let serialized = serialize_envelope(payload)?;
+ let chunk = NonZeroUsize::new(chunk_size).ok_or_else(|| io::Error::other("non-zero"))?;
+
+ let payloads = drive_with_partial_frames(app, &codec, vec![serialized], chunk).await?;
+
+ if payloads.is_empty() {
+ return Err(io::Error::other("expected non-empty response payloads"));
+ }
</code_context>
<issue_to_address>
**suggestion (testing):** Partial-frame tests only assert non-empty responses; they should decode and assert on the actual application-level payloads
These helpers are intended to verify that behavior under chunked delivery matches full-frame delivery. At the moment `test_partial_frames_with_chunk` and its callers only check that `payloads` is non-empty. To better detect regressions (e.g., framing changes or dropped/duplicated messages), deserialize the returned payloads into `Envelope` via `BincodeSerializer.deserialize` and assert that the header fields and body match the expected values.
```suggestion
async fn test_partial_frames_with_chunk(payload: &[u8], chunk_size: usize) -> io::Result<()> {
let codec = hotline_codec();
let app = build_echo_app(codec.clone())?;
let serialized = serialize_envelope(payload)?;
let chunk = NonZeroUsize::new(chunk_size).ok_or_else(|| io::Error::other("non-zero"))?;
let payloads = drive_with_partial_frames(app, &codec, vec![serialized], chunk).await?;
if payloads.is_empty() {
return Err(io::Error::other("expected non-empty response payloads"));
}
// Verify that each chunked response payload decodes to the same Envelope
// that we originally sent, ensuring chunked framing matches full-frame behavior.
let expected_env = Envelope::new(1, Some(7), payload.to_vec());
for (idx, payload_bytes) in payloads.into_iter().enumerate() {
let env: Envelope<Vec<u8>> = BincodeSerializer
.deserialize(&payload_bytes)
.map_err(|e| io::Error::new(io::ErrorKind::InvalidData, format!("deserialize: {e}")))?;
assert_eq!(
env,
expected_env,
"mismatched envelope at index {idx} for chunk_size={chunk_size}"
);
}
Ok(())
}
```
</issue_to_address>
### Comment 2
<location path="tests/partial_frame_feeding.rs" line_range="64-73" />
<code_context>
+ Ok(())
+}
+
+#[tokio::test]
+async fn partial_frames_single_byte_chunks() -> io::Result<()> {
+ test_partial_frames_with_chunk(&[10, 20, 30], 1).await
+}
+
+#[tokio::test]
+async fn partial_frames_misaligned_chunks() -> io::Result<()> {
+ test_partial_frames_with_chunk(&[1, 2, 3, 4, 5], 7).await
+}
+
+#[tokio::test]
+async fn partial_frames_multiple_payloads() -> io::Result<()> {
+ let codec = hotline_codec();
+ let app = build_echo_app(codec.clone())?;
</code_context>
<issue_to_address>
**suggestion (testing):** Multiple-payload partial-frame test should verify payload ordering and contents, not just count
In `partial_frames_multiple_payloads`, the test only asserts `payloads.len() == 2`. To fully validate that multiple messages survive misaligned chunked delivery, deserialize both payloads and assert their exact contents and order (e.g., first is `[1]`, second is `[2]`), so we also catch any reordering or unintended merging of frames.
Suggested implementation:
```rust
// For the multiple-payload partial-frame test we explicitly verify
// that both messages are preserved in order and with exact contents.
let first_payload: &[u8] = &[1];
let second_payload: &[u8] = &[2];
let first_serialized = serialize_envelope(first_payload)?;
let second_serialized = serialize_envelope(second_payload)?;
let chunk = NonZeroUsize::new(chunk_size).ok_or_else(|| io::Error::other("non-zero"))?;
let payloads = drive_with_partial_frames(
app,
&codec,
vec![first_serialized, second_serialized],
chunk,
)
.await?;
if payloads.len() != 2 {
return Err(io::Error::other(format!(
"expected exactly 2 response payloads, got {}",
payloads.len()
)));
}
// Deserialize both frames and assert ordering and contents.
let first_decoded = deserialize_envelope(&payloads[0], &codec)?;
let second_decoded = deserialize_envelope(&payloads[1], &codec)?;
if first_decoded != first_payload {
return Err(io::Error::other(format!(
"first payload mismatch: expected {:?}, got {:?}",
first_payload, first_decoded
)));
}
if second_decoded != second_payload {
return Err(io::Error::other(format!(
"second payload mismatch: expected {:?}, got {:?}",
second_payload, second_decoded
)));
}
Ok(())
}
```
- Ensure this SEARCH/REPLACE is applied to the body of the `partial_frames_multiple_payloads` test, not the shared helper used by other tests. You may need to adjust the SEARCH context to include the `async fn partial_frames_multiple_payloads` signature to disambiguate.
- The snippet assumes the existence of a `deserialize_envelope` helper with a signature roughly like:
`fn deserialize_envelope<B: AsRef<[u8]>>(bytes: B, codec: &Codec) -> io::Result<Vec<u8>>;`
If your actual deserializer has a different name or return type (e.g., some `Envelope` type that contains a payload field), adapt the two calls and equality checks accordingly (e.g., `first_decoded.payload == first_payload`).
- If `drive_with_partial_frames` already returns decoded payloads (e.g., `Vec<Vec<u8>>`), you can drop the `deserialize_envelope` calls and compare `payloads[0]`/`payloads[1]` directly to `first_payload`/`second_payload` while keeping the ordering and content assertions.
</issue_to_address>
### Comment 3
<location path="docs/execplans/8-5-1-utilities-for-feeding-partial-frames-into-in-process-app.md" line_range="100-101" />
<code_context>
+
+## Surprises & discoveries
+
+- Fragment payloads are `FRAG`-prefixed raw bytes, not valid `Envelope`
+ serialisations. The app receives them via the codec but cannot route
+ them (no matching envelope), so it produces no response. Fragment-feeding
+ tests therefore verify successful I/O completion (no error) rather than
</code_context>
<issue_to_address>
**suggestion (typo):** Spelling of "serialisation/serialization" is inconsistent within this document.
Earlier in the Risks section you use “serialization” (with z), while here it’s “serialisations” (with s). Given the en-GB-oxendict preference for -ize/-ization, please standardise on a single spelling (e.g. “serializations”) across this document.
Suggested implementation:
```
- Fragment payloads are `FRAG`-prefixed raw bytes, not valid `Envelope`
- serializations. The app receives them via the codec but cannot route
```
Search the rest of `docs/execplans/8-5-1-utilities-for-feeding-partial-frames-into-in-process-app.md` for any remaining `serialisation`/`serialisations` spellings and replace them with `serialization`/`serializations` to match the existing usage in the Risks section and maintain consistent `-ize/-ization` style throughout the document.
</issue_to_address>
### Comment 4
<location path="wireframe_testing/src/helpers/fragment_drive.rs" line_range="122" />
<code_context>
+/// # async fn demo() -> std::io::Result<()> {
+/// let codec = HotlineFrameCodec::new(4096);
+/// let app = WireframeApp::new().expect("app").with_codec(codec.clone());
+/// let fragmenter = Fragmenter::new(NonZeroUsize::new(20).unwrap());
+/// let payloads = drive_with_fragments(app, &codec, &fragmenter, vec![0; 50]).await?;
+/// # Ok(())
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Doc example uses `.unwrap()` instead of `.expect()`, which violates the preference guideline.
The style guide asks to prefer `.expect()` over `.unwrap()`. In this doc example, consider changing `NonZeroUsize::new(20).unwrap()` to something like `NonZeroUsize::new(20).expect("non-zero")` to align with that rule.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
Prefer `.expect()` over `.unwrap()`
</details>
</issue_to_address>
### Comment 5
<location path="wireframe_testing/src/helpers/fragment_drive.rs" line_range="122" />
<code_context>
+/// # async fn demo() -> std::io::Result<()> {
+/// let codec = HotlineFrameCodec::new(4096);
+/// let app = WireframeApp::new().expect("app").with_codec(codec.clone());
+/// let fragmenter = Fragmenter::new(NonZeroUsize::new(20).unwrap());
+/// let payloads = drive_with_fragments(app, &codec, &fragmenter, vec![0; 50]).await?;
+/// # Ok(())
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This second doc example also uses `.unwrap()` instead of `.expect()`.
As above, please switch `NonZeroUsize::new(20).unwrap()` to `.expect("non-zero")` (or similar) to follow the `.expect()`-over-`.unwrap()` guidance in examples as well as in main code.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
Prefer `.expect()` over `.unwrap()`
</details>
</issue_to_address>
### Comment 6
<location path="wireframe_testing/src/helpers/fragment_drive.rs" line_range="122" />
<code_context>
+/// # async fn demo() -> std::io::Result<()> {
+/// let codec = HotlineFrameCodec::new(4096);
+/// let app = WireframeApp::new().expect("app").with_codec(codec.clone());
+/// let fragmenter = Fragmenter::new(NonZeroUsize::new(20).unwrap());
+/// let payloads = drive_with_fragments(app, &codec, &fragmenter, vec![0; 50]).await?;
+/// # Ok(())
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Third doc example still uses `.unwrap()` contrary to the `.expect()` preference.
To keep the examples consistent with the project guidelines, please replace this `NonZeroUsize::new(20).unwrap()` with an `.expect(...)` call.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
Prefer `.expect()` over `.unwrap()`
</details>
</issue_to_address>
### Comment 7
<location path="wireframe_testing/src/helpers/fragment_drive.rs" line_range="122" />
<code_context>
+/// # async fn demo() -> std::io::Result<()> {
+/// let codec = HotlineFrameCodec::new(4096);
+/// let app = WireframeApp::new().expect("app").with_codec(codec.clone());
+/// let fragmenter = Fragmenter::new(NonZeroUsize::new(20).unwrap());
+/// let payloads = drive_with_fragments(app, &codec, &fragmenter, vec![0; 50]).await?;
+/// # Ok(())
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Fourth doc example uses `.unwrap()` where `.expect()` is required by the guidelines.
Same comment as the others: please change `NonZeroUsize::new(20).unwrap()` to use `.expect("non-zero")` (or an appropriate message).
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
Prefer `.expect()` over `.unwrap()`
</details>
</issue_to_address>
### Comment 8
<location path="wireframe_testing/src/helpers/partial_frame.rs" line_range="69" />
<code_context>
+/// async fn echo(mut s: DuplexStream) { let _ = s.write_all(&[1, 2]).await; }
+///
+/// # async fn demo() -> std::io::Result<()> {
+/// let out = drive_chunked_internal(echo, vec![0], NonZeroUsize::new(1).unwrap(), 64).await?;
+/// assert_eq!(out, [1, 2]);
+/// # Ok(())
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Doc example for `drive_chunked_internal` uses `.unwrap()` instead of `.expect()` on `NonZeroUsize::new(1)`.
To comply with the guideline of preferring `.expect()` over `.unwrap()`, please change `NonZeroUsize::new(1).unwrap()` here to something like `NonZeroUsize::new(1).expect("non-zero")`.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
Prefer `.expect()` over `.unwrap()`
</details>
</issue_to_address>
### Comment 9
<location path="docs/users-guide.md" line_range="312" />
<code_context>
+
+Real networks rarely deliver a complete codec frame in a single TCP read.
+The `wireframe_testing` crate provides drivers that simulate these
+conditions so you can exercise your codec's buffering logic in tests.
+
+**Chunked-write drivers** encode payloads via a codec, concatenate the wire
</code_context>
<issue_to_address>
**issue (review_instructions):** This sentence uses the 2nd-person pronoun "you", which the style guidelines prohibit.
Please rephrase this line to avoid directly addressing the reader. For example, something like "conditions so codec buffering logic can be exercised in tests" would comply with the no-"you" guideline.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Avoid 2nd person or 1st person pronouns ("I", "you", "we").
</details>
</issue_to_address>
### Comment 10
<location path="docs/execplans/8-5-1-utilities-for-feeding-partial-frames-into-in-process-app.md" line_range="48" />
<code_context>
+- en-GB-oxendict spelling in comments and documentation ("-ize" / "-yse" /
+ "-our").
+- Public functions require `///` doc comments with usage examples.
+- BDD fixtures must use `rstest-bdd` v0.5.0, and step function parameter names
+ must match the fixture name exactly (e.g., `partial_frame_feeding_world`).
+- Existing public APIs must not change signature.
</code_context>
<issue_to_address>
**issue (review_instructions):** The acronym "BDD" is introduced here without being expanded on first use, contrary to the documentation guideline.
Please expand "BDD" on its first occurrence in this document, for example "behaviour-driven development (BDD) fixtures must use `rstest-bdd` v0.5.0, ...". Subsequent uses can then use "BDD" alone.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
docs/execplans/8-5-1-utilities-for-feeding-partial-frames-into-in-process-app.md
Outdated
Show resolved
Hide resolved
docs/execplans/8-5-1-utilities-for-feeding-partial-frames-into-in-process-app.md
Show resolved
Hide resolved
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f226a14c20
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/8-5-1-utilities-for-feeding-partial-frames-into-in-process-app.md`:
- Around line 100-106: Update the documentation so Stage E's description
explicitly states that fragment-feeding tests validate successful I/O completion
(no error) rather than producing non-empty routed responses: change any wording
that implies fragments are valid Envelope serialisations or that the app will
route/respond, and instead clarify that FRAG-prefixed payloads are raw bytes
handled by the codec/transport layer only; also apply the same wording change to
the corresponding paragraph referenced (around the section originally at lines
401-405) so both places consistently indicate the tests verify I/O completion
and not application-level routing responses.
- Around line 442-465: The docs fixture API is out of sync: update the
description of PartialFrameFeedingWorld to reflect the implemented symbols in
tests/fixtures/partial_frame_feeding.rs by removing references to
response_frames and assert_received_fragments() and replacing them with the
actual fragment_feeding_completed field and assert_fragment_feeding_completed()
method; ensure the struct field list mentions fragment_feeding_completed
(instead of response_frames) and the assertion helpers list includes
assert_fragment_feeding_completed(), and verify any prose referring to received
fragments uses the fragment_feeding_completed terminology consistently (e.g., in
the helper methods/assertions section and in configure/drive helper
descriptions).
- Around line 100-101: Update this ExecPlan doc to use the repository's
en-GB-oxendict '-ize' spellings consistently: replace occurrences like
"serialisations" with "serializations" and "parameterised" with "parameterized"
(e.g. in the sentence "FRAG-prefixed raw bytes, not valid `Envelope`
serialisations") and scan the rest of the file (including the other reported
occurrence around the 391 mark) to normalize any remaining '-ise' forms to
'-ize' while preserving punctuation and code identifiers.
In `@docs/users-guide.md`:
- Around line 310-313: The sentence in the docs that mentions the
`wireframe_testing` crate contains second-person phrasing ("so you can exercise
your codec's buffering logic in tests"); change it to neutral voice by
rephrasing to something like "so tests can exercise a codec's buffering logic"
or "enabling exercise of a codec's buffering logic in tests"—update the sentence
referencing the `wireframe_testing` crate and remove "you" while preserving the
meaning about simulating incomplete TCP reads for codec buffering tests.
In `@tests/fixtures/partial_frame_feeding.rs`:
- Around line 113-115: The loop building payloads silently truncates indices >
u8::MAX via u8::try_from(i).unwrap_or(0); modify the loop that constructs
Envelope::new (inside the for i in 0..count) to propagate an error instead of
defaulting to 0 — use u8::try_from(i).map_err(|_| /* return an error describing
that the payload index exceeds u8::MAX */)? and then pass that byte into
Envelope::new, so BincodeSerializer.serialize(&env)? only runs when the index
conversion succeeded.
In `@tests/partial_frame_feeding.rs`:
- Around line 150-151: The test currently only checks that
drive_with_fragments(app, &codec, &fragmenter, vec![0; 100]).await? returns
without error and assigns to _payloads; replace the silent acceptance with
deterministic assertions: capture the returned payloads (e.g., let payloads =
drive_with_fragments(...).await? ) and assert their expected structure/content
and fragment-path outcomes (for example length, specific frame boundaries, or
expected fragment IDs) using assert_eq!/assert!; locate the helper invocation in
tests/partial_frame_feeding.rs and update all similar cases (lines near
drive_with_fragments at 150, and the other occurrences around 161-163, 172-174,
185-187) to verify explicit properties of payloads, codec decoding results, and
fragmenter-produced paths instead of only checking for no error.
In `@wireframe_testing/src/helpers/partial_frame.rs`:
- Around line 60-74: The doctest for drive_chunked_internal references the
internal module partial_frame and a pub(super) function, causing doctest
compilation failures; update the code fence to disable doctest by adding the
rust,ignore attribute to the opening triple-backtick (i.e., change the block
header to "```rust,ignore") so the example is skipped by cargo test while
leaving the example content intact.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
docs/execplans/8-5-1-utilities-for-feeding-partial-frames-into-in-process-app.mddocs/roadmap.mddocs/users-guide.mdtests/features/partial_frame_feeding.featuretests/fixtures/mod.rstests/fixtures/partial_frame_feeding.rstests/partial_frame_feeding.rstests/scenarios/mod.rstests/scenarios/partial_frame_feeding_scenarios.rstests/steps/mod.rstests/steps/partial_frame_feeding_steps.rswireframe_testing/src/helpers.rswireframe_testing/src/helpers/fragment_drive.rswireframe_testing/src/helpers/partial_frame.rswireframe_testing/src/lib.rs
docs/execplans/8-5-1-utilities-for-feeding-partial-frames-into-in-process-app.md
Outdated
Show resolved
Hide resolved
docs/execplans/8-5-1-utilities-for-feeding-partial-frames-into-in-process-app.md
Outdated
Show resolved
Hide resolved
docs/execplans/8-5-1-utilities-for-feeding-partial-frames-into-in-process-app.md
Show resolved
Hide resolved
Fragment payloads are now wrapped inside serialized `Envelope` packets before being fed through the codec and application pipeline. This ensures the application's deserializer accepts these fragment frames instead of accumulating deserialization failures and closing connections. - Updated fragment_and_encode to wrap `FRAG`-prefixed fragment bytes into `Envelope` packets serialized with `BincodeSerializer`. - Adjusted fragment feeding tests to verify envelope deserialization and responses. - Improved documentation and examples in wireframe_testing helpers to reflect the new envelope wrapping. - Added default route ID for wrapped fragments matching typical app route registrations. This enhancement makes fragment feeding tests more accurate by fully simulating the expected codec and application message framing. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
Owner
Author
|
@coderabbitai have the following now been resolved? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 58384 Need to confirm one remaining point before giving a definitive answer — let me check the full body of 🏁 Script executed: Length of output: 1221
Overall comments
Individual comments
|
…or fragment feeding - Corrected spelling of 'parameterised' to 'parameterized' in comments. - Updated test scenario assertions from 'app receives fragment frames' to 'fragment feeding completed'. - Replaced `response_frames` field with `fragment_feeding_completed` boolean in test fixture. - Changed assertion helpers to check for fragment feeding completion instead of received fragments. These changes improve clarity and accuracy of partial frame feeding tests. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.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
1 participant
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
+- Also adds an ExecPlan entry for 8.5.1 and expands docs to cover partial frame feeding utilities (execplans and users-guide).
Changes
Why
This enables tests to exercise realistic partial-frame buffering and fragment handling, aligning with the roadmap and improving test coverage for net/codec behavior without constructing raw wire bytes manually.
How to use (examples)
These utilities compose existing encoding/decoding paths with chunked or fragmented I/O to exercise buffering logic.
Testing plan
Impact
Artifacts
Files added/modified (high level):
Acceptance criteria
Notes
📎 Task: https://www.devboxer.com/task/995a6b16-109f-46b3-9e8f-054bf6b3abc8