Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Summary by CodeRabbit
WalkthroughIntroduce a new async helper Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Test as Test
participant SUT as System Under Test
participant Helper as collect_multi_packet
participant Rx as mpsc::Receiver<Frame>
Test->>SUT: trigger multi-packet response
SUT-->>Test: Response::MultiPacket(Rx)
Test->>Helper: collect_multi_packet(Response::MultiPacket)
rect rgb(240,250,240)
note over Helper,Rx: Async drain loop\n(recv until None)
loop until None
Helper->>Rx: recv()
Rx-->>Helper: Frame
end
end
Helper-->>Test: Vec<Frame>
note over Test: Proceed with assertions on collected frames
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
Comment |
Reviewer's GuideThis PR centralizes repeated async loops that drain Class diagram for the new collect_multi_packet helperclassDiagram
class Response {
<<generic>>
MultiPacket(rx)
// ... other variants
}
class collect_multi_packet {
+async fn collect_multi_packet(resp: Response<F, E>) Vec<F>
}
Response <|-- collect_multi_packet: uses
Flow diagram for collect_multi_packet draining MultiPacket responsesflowchart TD
A["Response::MultiPacket(rx)"] --> B["collect_multi_packet()"]
B --> C["Loop: rx.recv().await"]
C --> D["Push frame to Vec"]
D --> C
C -->|No more frames| E["Return Vec<F>"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
MultiPacketWorld, you switched from appending intoself.messagesto assigning the collected frames directly—double-check that this replacement (instead of extending) preserves the intended behavior when multiple drains occur. - The helper panics on non-MultiPacket variants; consider returning a
Resultor enhancing the panic message to include the actual variant for clearer debugging when mis-used.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `MultiPacketWorld`, you switched from appending into `self.messages` to assigning the collected frames directly—double-check that this replacement (instead of extending) preserves the intended behavior when multiple drains occur.
- The helper panics on non-MultiPacket variants; consider returning a `Result` or enhancing the panic message to include the actual variant for clearer debugging when mis-used.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/world.rs (1)
224-232: Remove redundant clear
Remove theself.messages.clear()call sinceself.messagesis immediately overwritten bycollect_multi_packet(resp).await.- self.messages.clear();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
tests/multi_packet.rs(5 hunks)tests/world.rs(2 hunks)wireframe_testing/src/lib.rs(2 hunks)wireframe_testing/src/multi_packet.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use precise names; boolean names should start with is/has/should
Use en-GB-oxendict spelling and grammar in comments
Function documentation must include clear examples; test documentation should omit redundant examples
Keep code files ≤ 400 lines; split long switch/dispatch logic by feature; move large test data to external files
Disallow Clippy warnings
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions for long functions; adhere to separation of concerns and CQRS
Group related parameters into meaningful structs when functions have many parameters
Consider using Arc for large error returns to reduce data size
Each Rust module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditional criteria with more than two branches
Do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect(..)] over #[allow(..)] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Files:
wireframe_testing/src/lib.rswireframe_testing/src/multi_packet.rstests/world.rstests/multi_packet.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
wireframe_testing/src/lib.rswireframe_testing/src/multi_packet.rstests/world.rstests/multi_packet.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
tests/world.rstests/multi_packet.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/world.rstests/multi_packet.rs
🧬 Code graph analysis (3)
wireframe_testing/src/lib.rs (1)
wireframe_testing/src/multi_packet.rs (1)
collect_multi_packet(26-37)
tests/world.rs (1)
wireframe_testing/src/multi_packet.rs (1)
collect_multi_packet(26-37)
tests/multi_packet.rs (1)
wireframe_testing/src/multi_packet.rs (1)
collect_multi_packet(26-37)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (3)
wireframe_testing/src/lib.rs (1)
23-23: Expose the new module — LGTM.The module declaration is correct and scoped appropriately.
tests/multi_packet.rs (1)
19-20: Adopt the shared helper — LGTM.The helper use removes duplicated drain loops and improves clarity.
tests/world.rs (1)
22-22: Adopt the shared helper — LGTM.The import is correct and aligns worlds with unit tests.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
wireframe_testing/src/lib.rs (1)
43-44: Inline docs on the re-export — LGTM.
#[doc(inline)]is correctly applied so rustdoc surfaces the function docs at the crate root.wireframe_testing/src/multi_packet.rs (1)
26-28: Panic contract documented — LGTM.The Panics section is explicit and aligns with test-helper ergonomics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
tests/multi_packet.rs(1 hunks)tests/world.rs(2 hunks)wireframe_testing/src/lib.rs(2 hunks)wireframe_testing/src/multi_packet.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use precise names; boolean names should start with is/has/should
Use en-GB-oxendict spelling and grammar in comments
Function documentation must include clear examples; test documentation should omit redundant examples
Keep code files ≤ 400 lines; split long switch/dispatch logic by feature; move large test data to external files
Disallow Clippy warnings
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions for long functions; adhere to separation of concerns and CQRS
Group related parameters into meaningful structs when functions have many parameters
Consider using Arc for large error returns to reduce data size
Each Rust module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditional criteria with more than two branches
Do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect(..)] over #[allow(..)] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Files:
tests/world.rstests/multi_packet.rswireframe_testing/src/multi_packet.rswireframe_testing/src/lib.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
tests/world.rstests/multi_packet.rswireframe_testing/src/multi_packet.rswireframe_testing/src/lib.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
tests/world.rstests/multi_packet.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/world.rstests/multi_packet.rs
🧬 Code graph analysis (3)
tests/world.rs (1)
wireframe_testing/src/multi_packet.rs (1)
collect_multi_packet(30-44)
tests/multi_packet.rs (1)
wireframe_testing/src/multi_packet.rs (1)
collect_multi_packet(30-44)
wireframe_testing/src/lib.rs (1)
wireframe_testing/src/multi_packet.rs (1)
collect_multi_packet(30-44)
🔍 Remote MCP
Here are a few additional points surfaced by automated reviews that may impact your assessment:
-
In
MultiPacketWorld::process_messages, the patch replaces the previousself.messages.extend(drain(...))(or manual pushes) with a plain assignment of the vector returned bycollect_multi_packet—i.e.// before self.messages.clear(); self.messages.extend(self.drain(resp).await); // after self.messages = collect_multi_packet(resp).await;
Reviewers caution that this change in “extend vs. assign” semantics could alter behavior if
process_messagesis called multiple times on the same world instance (github.com). -
The new helper currently panics if given a
Responsethat isn’tMultiPacket. It simply does:match resp { Response::MultiPacket(rx) => { … } _ => panic!("expected Response::MultiPacket"), }
It’s been suggested to either return a
Result<Vec<F>, Error>(so mis-uses can be handled gracefully) or at least include the unexpected variant in the panic message for easier debugging (github.com). -
In the Cucumber world tests, the now-redundant
self.messages.clear()call was removed sincecollect_multi_packetalways returns a freshVec(github.com). -
Overall, centralizing the drain logic into
collect_multi_packetdid remove duplicated loops and unified the collection path, improving clarity and reducing boilerplate (github.com).
🔇 Additional comments (6)
wireframe_testing/src/lib.rs (1)
23-23: Expose the new module at the crate root — LGTM.Module wiring is correct and keeps the crate surface tidy.
tests/world.rs (1)
22-22: Use the shared helper in Cucumber world — LGTM.Imports are minimal and avoid local duplication.
wireframe_testing/src/multi_packet.rs (2)
1-5: Module docs for test-only helpers — LGTM.Docs are clear and scoped to test usage.
31-43: Drain loop and enriched panic messages — LGTM.Loop is correct and terminates on channel close; variant-specific panic messages aid debugging.
tests/multi_packet.rs (2)
11-17: Consolidate cases with rstest — LGTM.Parameterisation reduces duplication while covering empty/partial/over-capacity paths.
19-37: Concurrent send + drain verifies backpressure — LGTM.Test structure avoids races and asserts exact message ordering.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai have the following now been resolved?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos: Let me check the current state of these issues: Issue 1 - MultiPacketWorld assignment vs extending behavior: self.messages = collect_multi_packet(resp).await;This has not been resolved. The behavior change from extending to assignment means that if Issue 2 - Helper panic behavior: _ => panic!("expected Response::MultiPacket"),This has not been resolved. The panic message doesn't include the actual variant received, making debugging difficult when the helper is misused. Recommendation:
🏁 Script executed: Length of output: 3671 🏁 Script executed: Length of output: 1688 @leynos: Based on my examination of the current code, here's the status of both issues: Issue 1 - MultiPacketWorld assignment vs extending behavior: ❌ NOT RESOLVED The code in self.messages = collect_multi_packet(resp).await;This means if Issue 2 - Helper panic behavior: ✅ PARTIALLY RESOLVED The
Summary:
The panic improvement in issue 2 is actually better than the original suggestion since it provides specific messages for each variant type, making debugging much easier. |
…helper - Keep branch’s collect_multi_packet helper in wireframe_testing - Update tests to use helper; add drain_all for non-channel variants - Remove obsolete local drains and conflict markers - Fix formatting and clippy by constraining error Debug bound Validated with: make check-fmt, make test, make lint
leynos
force-pushed
the
codex/extract-shared-helper-for-testing
branch
from
4daa63f to
6b51272
Compare
|
@coderabbitai Please create a new GitHub issue for the following: Issue 1 - MultiPacketWorld assignment vs extending behavior: ❌ NOT RESOLVED The code in tests/world.rs line 230 still uses direct assignment: self.messages = collect_multi_packet(resp).await; This means if process_messages is called multiple times on the same MultiPacketWorld instance, previous messages will be overwritten rather than accumulated. This could break test behavior if any tests rely on accumulating messages across multiple calls. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos I'll create a GitHub issue for the MultiPacketWorld assignment behavior problem. 🏁 Script executed: Length of output: 46 @leynos ✅ I've successfully created GitHub issue #355 for the MultiPacketWorld assignment behavior problem. Issue created: #355 The issue includes:
The issue is now ready for tracking and future resolution. |
1 participant
Summary
collect_multi_packethelper for drainingResponse::MultiPacketTesting
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_68bb3035ab248322b84ad28d8e6f70e2
Summary by Sourcery
Extract a shared helper for draining Response::MultiPacket values to consolidate duplicate loops in tests and Cucumber steps.
Enhancements:
Documentation:
Tests: