Merged
Conversation
Contributor
Reviewer's GuideIntroduces a reusable helper to dynamically compute expected output for fairness-disabled scheduler tests and adds boundary test cases for high-only and low-only priority scenarios, simplifying the existing test by replacing hardcoded sequences with a call to the new helper. File-Level Changes
Possibly linked issues
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 7 minutes and 34 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
WalkthroughThe test Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Function
participant FrameGen as Frame Number Generator
participant Assert as Assertion
Test->>FrameGen: Determine number of high/low priorities in input order
FrameGen-->>Test: Generate dynamic frame numbers for highs and lows
Test->>Test: Push frames using generated numbers
Test->>Assert: Construct expected output vector dynamically
Assert-->>Test: Compare actual and expected outputs
Possibly related issues
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 (
|
![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 - here's some feedback:
- The expected output builder uses low-level iterator arithmetic which makes the test harder to follow; consider extracting a small helper or using a simple map with two counters to generate the expected sequence more declaratively.
- The test function name
fairness_disabled_processes_all_high_firstno longer accurately reflects that it covers both high-only and low-only scenarios; renaming it to something more general (e.g.,processes_all_priorities_in_order) would improve clarity. - The new
#[case]attributes for homogeneous sequences can be simplified withvec![Priority::High; 3]andvec![Priority::Low; 3]instead of manually repeating each element.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The expected output builder uses low-level iterator arithmetic which makes the test harder to follow; consider extracting a small helper or using a simple map with two counters to generate the expected sequence more declaratively.
- The test function name `fairness_disabled_processes_all_high_first` no longer accurately reflects that it covers both high-only and low-only scenarios; renaming it to something more general (e.g., `processes_all_priorities_in_order`) would improve clarity.
- The new `#[case]` attributes for homogeneous sequences can be simplified with `vec![Priority::High; 3]` and `vec![Priority::Low; 3]` instead of manually repeating each element.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/connection_actor.rs:91` </location>
<code_context>
+async fn queue_frames(
</code_context>
<issue_to_address>
Consider adding tests for empty input and mixed single-element cases.
Adding tests for empty input and single-element cases will help verify the scheduler's behavior in these edge scenarios.
Suggested implementation:
```rust
async fn queue_frames(
order: &[Priority],
handle: &wireframe::push::PushHandle<u8>,
high_count: usize,
) -> Vec<u8> {
let mut next_high = 1u8;
let mut next_low = u8::try_from(high_count).expect("too many high frames") + 1;
let mut highs = Vec::new();
let mut lows = Vec::new();
}
// Test for empty input
#[tokio::test]
async fn test_queue_frames_empty_input() {
let (handle, mut output) = wireframe::push::PushHandle::new();
let result = queue_frames(&[], &handle, 0).await;
assert!(result.is_empty(), "Expected empty output for empty input");
}
// Test for single high-priority frame
#[tokio::test]
async fn test_queue_frames_single_high() {
let (handle, mut output) = wireframe::push::PushHandle::new();
let result = queue_frames(&[Priority::High], &handle, 1).await;
assert_eq!(result, vec![1u8], "Expected output to contain only the high-priority frame");
}
// Test for single low-priority frame
#[tokio::test]
async fn test_queue_frames_single_low() {
let (handle, mut output) = wireframe::push::PushHandle::new();
let result = queue_frames(&[Priority::Low], &handle, 0).await;
assert_eq!(result, vec![1u8], "Expected output to contain only the low-priority frame");
}
// Test for mixed single-element case (one high, one low)
#[tokio::test]
async fn test_queue_frames_mixed_single_elements() {
let (handle, mut output) = wireframe::push::PushHandle::new();
let result = queue_frames(&[Priority::High, Priority::Low], &handle, 1).await;
assert_eq!(result, vec![1u8, 2u8], "Expected output to contain high then low frame");
}
```
- Ensure that `wireframe::push::PushHandle::new()` is available and returns a handle suitable for testing.
- If the `queue_frames` function or the test module uses a different async runtime, adjust the test attribute accordingly.
- If the `Priority` enum is not in scope, import it at the top of the file.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+91
to
+100
| async fn queue_frames( | ||
| order: &[Priority], | ||
| handle: &wireframe::push::PushHandle<u8>, | ||
| high_count: usize, | ||
| ) -> Vec<u8> { | ||
| let mut next_high = 1u8; | ||
| let mut next_low = u8::try_from(high_count).expect("too many high frames") + 1; | ||
|
|
||
| let mut highs = Vec::new(); | ||
| let mut lows = Vec::new(); |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider adding tests for empty input and mixed single-element cases.
Adding tests for empty input and single-element cases will help verify the scheduler's behavior in these edge scenarios.
Suggested implementation:
async fn queue_frames(
order: &[Priority],
handle: &wireframe::push::PushHandle<u8>,
high_count: usize,
) -> Vec<u8> {
let mut next_high = 1u8;
let mut next_low = u8::try_from(high_count).expect("too many high frames") + 1;
let mut highs = Vec::new();
let mut lows = Vec::new();
}
// Test for empty input
#[tokio::test]
async fn test_queue_frames_empty_input() {
let (handle, mut output) = wireframe::push::PushHandle::new();
let result = queue_frames(&[], &handle, 0).await;
assert!(result.is_empty(), "Expected empty output for empty input");
}
// Test for single high-priority frame
#[tokio::test]
async fn test_queue_frames_single_high() {
let (handle, mut output) = wireframe::push::PushHandle::new();
let result = queue_frames(&[Priority::High], &handle, 1).await;
assert_eq!(result, vec![1u8], "Expected output to contain only the high-priority frame");
}
// Test for single low-priority frame
#[tokio::test]
async fn test_queue_frames_single_low() {
let (handle, mut output) = wireframe::push::PushHandle::new();
let result = queue_frames(&[Priority::Low], &handle, 0).await;
assert_eq!(result, vec![1u8], "Expected output to contain only the low-priority frame");
}
// Test for mixed single-element case (one high, one low)
#[tokio::test]
async fn test_queue_frames_mixed_single_elements() {
let (handle, mut output) = wireframe::push::PushHandle::new();
let result = queue_frames(&[Priority::High, Priority::Low], &handle, 1).await;
assert_eq!(result, vec![1u8, 2u8], "Expected output to contain high then low frame");
}- Ensure that
wireframe::push::PushHandle::new()is available and returns a handle suitable for testing. - If the
queue_framesfunction or the test module uses a different async runtime, adjust the test attribute accordingly. - If the
Priorityenum is not in scope, import it at the top of the file.
6 tasks
This was referenced Jul 7, 2025
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 fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_686c206e925c8322b95f363cc1e8574f
Summary by Sourcery
Add dynamic expected output generation and boundary tests for the scheduler’s fairness-disabled priority queues
Enhancements:
queue_frameshelper instead of hardcoded sequencesTests:
queue_frameshelper to generate expected output dynamically for priority push queues