Merged
Conversation
Contributor
Reviewer's GuideExtract common I/O plumbing into a new drive_internal helper and refactor existing test driver functions and documentation to use it. Class diagram for refactored test driver helpersclassDiagram
class WireframeApp {
+handle_connection(server)
}
class drive_internal {
+drive_internal(server_fn, frames, capacity)
}
class drive_with_frame {
+drive_with_frame(app, frame, capacity)
}
class drive_with_frame_mut {
+drive_with_frame_mut(app, frames, capacity)
}
drive_with_frame --|> drive_internal : delegates
drive_with_frame_mut --|> drive_internal : delegates
drive_with_frame : WireframeApp app
drive_with_frame_mut : WireframeApp app
drive_internal : server_fn
drive_internal : frames
drive_internal : capacity
WireframeApp : handle_connection(server)
Flow diagram for centralized frame driving logicflowchart TD
A[Start test helper] --> B[Call drive_with_frame or drive_with_frame_mut]
B --> C[Delegate to drive_internal]
C --> D[Create duplex stream]
D --> E[Run app.handle_connection on server half]
D --> F[Write frames to client half]
F --> G[Shutdown client]
G --> H[Read response bytes]
H --> I[Return collected bytes]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughRevise the documentation for wireframe testing helpers to clarify their behaviour and internal delegation. Refactor the Rust helper implementations by introducing a private Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Helper
participant Internal as drive_internal
participant App as WireframeApp (server)
participant Client as Client Task
Test->>Internal: Invoke with frames, handler, capacity
Internal->>App: Run server handler on server half of duplex stream
Internal->>Client: Write frames, shutdown write half, read output bytes
App-->>Internal: Process frames, write response
Client-->>Internal: Collect output bytes
Internal-->>Test: Return output bytes or error
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `wireframe_testing/src/helpers.rs:36` </location>
<code_context>
const DEFAULT_CAPACITY: usize = 4096;
+async fn drive_internal<F, Fut>(
+ server_fn: F,
+ frames: Vec<Vec<u8>>,
+ capacity: usize,
+) -> io::Result<Vec<u8>>
+where
+ F: FnOnce(DuplexStream) -> Fut,
+ Fut: std::future::Future<Output = ()> + Send,
+{
+ let (mut client, server) = duplex(capacity);
+ let server_fut = server_fn(server);
+ let client_fut = async {
</code_context>
<issue_to_address>
Error handling for server task failures is no longer present.
With drive_internal, errors or panics in the server future are no longer surfaced, which could hide server-side failures. Please restore error propagation for the server future to prevent silent errors.
</issue_to_address>
### Comment 2
<location> `wireframe_testing/src/helpers.rs:109` </location>
<code_context>
- format!("server task failed: {e}"),
- )),
- }
+ drive_internal(
+ |server| async move { app.handle_connection(server).await },
+ frames,
+ capacity,
+ )
+ .await
}
</code_context>
<issue_to_address>
Return type of drive_with_frames has changed to io::Result<Vec<u8>> without server error context.
Previously, server task failures were wrapped with a descriptive io::Error, aiding in debugging. The new approach omits this, making server errors less visible to callers.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
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
Testing
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_688bf673f1f08322ab4e2b7919e6038c
Summary by Sourcery
Centralize frame driving logic for test helpers by extracting a common internal function and refactoring existing drive functions to use it
Enhancements:
Documentation: