Raise risk-driven coverage to 85% - #10
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughCoverage reporting now enforces line and branch thresholds and emits formatted XML. Tests expand validation for CLI parsing, output, interruptions, faults, file replacement, discovery options, and unit/state string conversions. ChangesCoverage and test coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/test_cli.cpp`:
- Around line 421-433: Strengthen the stderr assertions in both CliMonitor
tests: test/test_cli.cpp lines 421-433 should check for a stable substring
identifying the stored client failure, and test/test_cli.cpp lines 467-479
should check for a stable substring identifying the fail-stop client fault. Keep
the existing non-empty checks and return-code expectations, but ensure each test
rejects generic first-sample timeout diagnostics.
- Around line 257-278: Strengthen CliInfo.SerializesEveryJsonControlBranch by
asserting the output JSON contains the product field with the complete escaped
value, including escaped quotes, backslashes, backspace, form feed, newline,
carriage return, tab, and the control-byte escape. Keep the existing success and
empty-error assertions unchanged.
In `@test/test_types.cpp`:
- Around line 300-317: Update ConvertsStateAndFaultCodeStrings to assert each
individual ClientState and FaultCode maps to its exact expected public string,
rather than relying only on set-size uniqueness. Retain the existing coverage of
all enum values, but add direct expectations for each netft::to_string result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d4ccf229-ea69-46d0-8040-5ec019d5653f
📒 Files selected for processing (5)
codecov.ymlpixi.tomltest/test_cli.cpptest/test_discovery.cpptest/test_types.cpp
Comment on lines
+421
to
+433
| TEST(CliMonitor, IncludesAStoredClientFailureWhenNoSampleArrives) { | ||
| netft::test::FakeSensor sensor; | ||
| auto options = options_for(netft::cli::Command::Monitor, sensor); | ||
| options.config.rdt_port = 1; | ||
| options.config.recovery_policy = netft::RecoveryPolicy::FailStop; | ||
| options.duration = 50ms; | ||
| std::ostringstream output; | ||
| std::ostringstream errors; | ||
|
|
||
| EXPECT_EQ(netft::cli::run(options, output, errors), 2); | ||
| EXPECT_TRUE(output.str().empty()); | ||
| EXPECT_FALSE(errors.str().empty()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify the specific client-failure diagnostics. Both tests accept any stderr text, including a generic first-sample timeout, rather than proving the intended client failure was propagated.
test/test_cli.cpp#L421-L433: assert a stable substring identifying the stored client failure.test/test_cli.cpp#L467-L479: assert a stable substring identifying the fail-stop client fault.
📍 Affects 1 file
test/test_cli.cpp#L421-L433(this comment)test/test_cli.cpp#L467-L479
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/test_cli.cpp` around lines 421 - 433, Strengthen the stderr assertions
in both CliMonitor tests: test/test_cli.cpp lines 421-433 should check for a
stable substring identifying the stored client failure, and test/test_cli.cpp
lines 467-479 should check for a stable substring identifying the fail-stop
client fault. Keep the existing non-empty checks and return-code expectations,
but ensure each test rejects generic first-sample timeout diagnostics.
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Merged
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
No production source or public API is changed.
Verification
pixi run format-checkpixi run testpixi run coveragepixi run -e cmake-316 cmake-316-testLocal coverage before:
Local coverage after:
Summary by CodeRabbit
Bug Fixes
Tests