Refactor budget config parsing and standardize error handling#494
Merged
Refactor budget config parsing and standardize error handling#494
Conversation
…and clarity Refactored the parsing of budget configuration strings in multiple test fixtures to use a shared helper function `parse_standard_budget_config`. Extracted this parser to a new module `budget_config_parser`, consolidating duplicated parsing logic from budget_cleanup.rs, budget_transitions.rs, memory_budget_backpressure.rs, and memory_budget_hard_cap.rs. Also improved error handling and code clarity in BudgetCleanupWorld by simplifying result assignment of send operations. Updated mod.rs to include the new module. This change reduces code duplication, improves maintainability, and centralizes standard budget config parsing logic used across budget-related test fixtures. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Contributor
Reviewer's GuideRefactors test budget fixtures to share a common parser for four-part budget configuration strings and unifies send-error handling in BudgetCleanupWorld, reducing duplicated parsing logic and standardizing error messages across tests. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughCentralise four-segment budget string parsing into a new test-only parser and replace inline parsing in multiple test fixtures with calls to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
![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 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/fixtures/budget_cleanup.rs" line_range="271-277" />
<code_context>
- self.last_send_error = Some(error.to_string());
- Err(error)
- }
+ let result = send_result.and_then(|io_result| io_result.map_err(Into::into));
+ if let Err(ref error) = result {
+ self.last_send_error = Some(error.to_string());
+ } else {
+ self.last_send_error = None;
}
+ result
}
</code_context>
<issue_to_address>
**suggestion (testing):** Add regression tests for `last_send_error` behavior across success and both error paths
Since the three prior branches are now unified, please add focused tests for `BudgetCleanupWorld` (or the relevant abstraction) that verify:
- A successful send after a failure clears `last_send_error`.
- An outer `Err(error)` from the send call sets `last_send_error` to the correct string and returns the expected error type.
- An inner `Ok(Err(io_error))` sets `last_send_error` and returns the converted error.
These tests should assert the exact stored string value, not just `Some/None`, to lock in the observable behavior for future refactors.
Suggested implementation:
```rust
const ROUTE_ID: u32 = 92;
const CORRELATION_ID: Option<u64> = Some(21);
const BUFFER_CAPACITY: usize = 512;
#[cfg(test)]
mod last_send_error_tests {
use super::*;
use std::io;
/// Helper to construct a world instance.
///
/// Adjust this to use the real constructor/new/builder used in the tests.
fn make_world() -> BudgetCleanupWorld {
// If `BudgetCleanupWorld` already has a constructor in this module,
// replace this with that call (e.g. `BudgetCleanupWorld::new(...)`).
BudgetCleanupWorld::default()
}
/// Helper to invoke the method that wraps `send_result.and_then(...)`.
///
/// Adjust the name and parameters to match the real API under test.
fn perform_send(
world: &mut BudgetCleanupWorld,
send_result: Result<Result<(), io::Error>, crate::Error>,
) -> Result<(), crate::Error> {
// This should call the function/method that contains:
// let result = send_result.and_then(|io_result| io_result.map_err(Into::into));
//
// For example, if the method is:
// impl BudgetCleanupWorld {
// fn send_cleanup(&mut self, send_result: Result<Result<(), io::Error>, Error>) -> Result<(), Error> { ... }
// }
// then call that here.
world.send_cleanup(send_result)
}
#[test]
fn last_send_error_is_cleared_on_success_after_failure() {
let mut world = make_world();
//
// 1. Induce a failure to populate `last_send_error`
//
let io_error = io::Error::new(io::ErrorKind::Other, "first failure");
let converted_error: crate::Error = io_error.into();
let send_result = Ok::<Result<(), io::Error>, crate::Error>(Err(io_error));
let result = perform_send(&mut world, send_result);
assert!(result.is_err(), "expected initial send to fail");
let expected_first = converted_error.to_string();
assert_eq!(
world.last_send_error.as_deref(),
Some(expected_first.as_str()),
"expected last_send_error to be populated after failure"
);
//
// 2. Follow with a successful send, which should clear `last_send_error`
//
let success_send_result =
Ok::<Result<(), io::Error>, crate::Error>(Ok::<(), io::Error>(()));
let success_result = perform_send(&mut world, success_send_result);
assert!(success_result.is_ok(), "expected second send to succeed");
assert_eq!(
world.last_send_error, None,
"expected last_send_error to be cleared after a successful send"
);
}
#[test]
fn last_send_error_is_set_for_outer_error_from_send_call() {
let mut world = make_world();
// Create a deterministic outer error.
// Replace `crate::Error::SendFailed("...".into())` with the concrete
// constructor used by your error type.
let outer_error = crate::Error::SendFailed("outer failure".into());
let send_result: Result<Result<(), io::Error>, crate::Error> = Err(outer_error.clone());
let result = perform_send(&mut world, send_result);
// Verify the returned error matches the outer error.
let returned_err = result.expect_err("expected outer error from send call");
assert_eq!(
format!("{returned_err}"),
format!("{outer_error}"),
"expected returned error to equal the outer send error"
);
// Verify the exact stored string.
let expected = outer_error.to_string();
assert_eq!(
world.last_send_error.as_deref(),
Some(expected.as_str()),
"expected last_send_error to store the outer error string"
);
}
#[test]
fn last_send_error_is_set_for_inner_io_error_and_error_is_converted() {
let mut world = make_world();
// Construct a deterministic IO error.
let io_error = io::Error::new(io::ErrorKind::Other, "inner io failure");
let io_error_clone_for_string = io::Error::new(io::ErrorKind::Other, "inner io failure");
let send_result: Result<Result<(), io::Error>, crate::Error> =
Ok::<Result<(), io::Error>, crate::Error>(Err(io_error));
let result = perform_send(&mut world, send_result);
// The inner `io::Error` should have been converted into `crate::Error`
// via `Into::into`.
let converted_error: crate::Error = io_error_clone_for_string.into();
let returned_err = result.expect_err("expected converted inner IO error");
assert_eq!(
format!("{returned_err}"),
format!("{converted_error}"),
"expected returned error to be the converted IO error"
);
// And `last_send_error` should store the converted error's string.
let expected = converted_error.to_string();
assert_eq!(
world.last_send_error.as_deref(),
Some(expected.as_str()),
"expected last_send_error to store the converted IO error string"
);
}
}
type Err = String;
```
These edits assume the following, which you will likely need to align with the actual code:
1. `BudgetCleanupWorld`
- Exists in this module and is `Default`-constructible.
If not, replace `BudgetCleanupWorld::default()` in `make_world()` with the real constructor or builder used in your tests.
2. Method under test
- I used `world.send_cleanup(send_result)` in `perform_send()` as the wrapper for the new logic:
```rust
let result = send_result.and_then(|io_result| io_result.map_err(Into::into));
if let Err(ref error) = result { ... } else { ... }
```
- Replace `send_cleanup` with the actual method name and signature that contains that logic, and adjust the parameter types in `perform_send()` to match.
3. Error types
- I referenced `crate::Error` and a variant `Error::SendFailed(String)` as an example.
- Replace `crate::Error` and the construction of `outer_error`/`converted_error` with your actual error type and constructors.
- Make sure the `send_result` type in tests matches the real `send_result` type in the implementation (outer `Err` type and inner `io::Error` vs your concrete types).
4. Field visibility
- These tests access `world.last_send_error` directly. If `last_send_error` is private or behind an accessor, either:
- Move these tests into the same module as `BudgetCleanupWorld`’s impl so they can see private fields, or
- Replace direct field access with the appropriate getter (e.g. `world.last_send_error()`).
Once you align the constructor, method name/signature, and error types with your real implementation, these tests will lock in the `last_send_error` behavior for all three code paths and assert the exact stored error strings.
</issue_to_address>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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/fixtures/budget_config_parser.rs`:
- Around line 17-50: Add a #[cfg(test)] mod tests in the same file and implement
unit tests that exercise parse_standard_budget_config for the four cases: valid
input "200/2048/8/8" asserting each field, missing-segment input "10/64/100"
asserting the error equals "missing in_flight", trailing-segment input
"10/64/100/100/extra" asserting the error equals "unexpected trailing segments",
and a parse-failure like "abc/64/100/100" asserting the error starts with
"timeout_ms: ". Use the existing parse_standard_budget_config function name and
rely on its current error strings (next_non_empty produces "missing {name}",
trailing check returns "unexpected trailing segments", and parse_segment
produces "{name}: {error}") so tests pin those messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8ebab34b-f903-4b4c-8210-78a538d64ffc
📒 Files selected for processing (6)
tests/fixtures/budget_cleanup.rstests/fixtures/budget_config_parser.rstests/fixtures/budget_transitions.rstests/fixtures/memory_budget_backpressure.rstests/fixtures/memory_budget_hard_cap.rstests/fixtures/mod.rs
Added comprehensive tests for the BudgetCleanupWorld's send result handling, verifying error recording and clearing. Also included multiple tests for StandardBudgetConfig parsing, checking valid inputs, missing segments, trailing segments, and parse failures with field names. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
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: 12978
This satisfies the original suggestion's requirement to lock in observable behaviour for all three code paths via exact string comparisons. |
…config error messages - Removed multiple detailed error message checks for each missing segment and parse failure - Consolidated tests to focus on key error scenarios, improving test clarity - Updated test names and inputs to reflect current expected outputs Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/fixtures/budget_config_parser.rs`:
- Around line 9-12: The public struct StandardBudgetConfig exposes fields
timeout_ms, per_message, per_connection, and in_flight without documentation;
add Rustdoc comments (///) above each public field in the StandardBudgetConfig
definition describing what the field represents, its units/range (e.g.,
timeout_ms in milliseconds), and any behavioral notes so generated cargo doc API
pages have clear field-level descriptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f54861c8-a319-433b-b730-b61666395e45
📒 Files selected for processing (1)
tests/fixtures/budget_config_parser.rs
… struct Added descriptive doc comments for each field in the StandardBudgetConfig struct within the test fixture to clarify their purpose and constraints, improving code readability and maintainability. 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
Changes
Implementation details
Tests plan
Possible impacts
How to run
📎 Task: https://www.devboxer.com/task/7cb7ad67-23ca-481b-9138-dbf1eeda61c6