Conversation
Reviewer's GuideThis PR integrates the logtest crate to provide an exclusive rstest logger fixture, applies it in new async push policy tests to assert on log behavior, adds logtest as a dev-dependency, and updates the testing guide with a dedicated logtest section. Class diagram for the new logger fixture and push policy testsclassDiagram
class LoggerHandle {
+Logger logger
+fn start() -> LoggerHandle
+fn pop() -> Option<LogRecord>
}
class DropIfFullPolicy {
+fn push(item) -> Result
}
class WarnAndDropIfFullPolicy {
+fn push(item) -> Result
}
class PushPolicyTest {
+fn test_drop_policy_uses_logger()
+fn test_warn_and_drop_policy_uses_logger()
}
LoggerHandle <|-- PushPolicyTest
DropIfFullPolicy <|-- PushPolicyTest
WarnAndDropIfFullPolicy <|-- PushPolicyTest
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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 53 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)
WalkthroughA new development dependency, Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Function
participant Logger as logtest::Logger
participant Queue as PushQueue
Test->>Logger: Start logger
Test->>Queue: Push item (high/low priority)
Test->>Queue: Push another item (DropIfFull/WarnAndDropIfFull)
Queue-->>Logger: Emit warning (if WarnAndDropIfFull)
Test->>Logger: Assert log records
Test->>Queue: Assert queue contents
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}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- In
LoggerHandle::new, you callLogger::start()inside theOnceblock but discard its return value and then dolet mut logger = Logger;—you should capture and return the Logger fromLogger::start()to ensure proper log isolation. - Maintaining a manual
tokio::runtimefixture adds boilerplate—consider marking the testsasyncwith#[tokio::test](or using rstest’s async support) to eliminateblock_onand simplify the test code. - The
logtest = "2"entry in Cargo.toml is too loose; pinning to a more specific version range (e.g.^2.0) will guard against unintended breaking changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `LoggerHandle::new`, you call `Logger::start()` inside the `Once` block but discard its return value and then do `let mut logger = Logger;`—you should capture and return the Logger from `Logger::start()` to ensure proper log isolation.
- Maintaining a manual `tokio::runtime` fixture adds boilerplate—consider marking the tests `async` with `#[tokio::test]` (or using rstest’s async support) to eliminate `block_on` and simplify the test code.
- The `logtest = "2"` entry in Cargo.toml is too loose; pinning to a more specific version range (e.g. `^2.0`) will guard against unintended breaking changes.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}
|
@sourcery-ai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.toml(1 hunks)docs/rust-testing-with-rstest-fixtures.md(1 hunks)tests/push_policies.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test
🔇 Additional comments (4)
Cargo.toml (1)
18-18: LGTM: Appropriate dev dependency addition.The
logtestdependency is correctly placed in[dev-dependencies]with a sensible version constraint that allows compatible updates within major version 2.docs/rust-testing-with-rstest-fixtures.md (1)
1240-1263: Excellent documentation addition for logtest integration.The new section provides clear, practical guidance on using
logtestwithrstestfor verifying log output. The examples are well-structured and the explanation of complementary usage withrstestis helpful.tests/push_policies.rs (2)
40-53: Comprehensive test for DropIfFull policy.This test effectively verifies that the
DropIfFullpolicy discards frames without logging warnings. The test setup, execution, and assertions are well-structured and clearly validate the expected behaviour.
55-71: Thorough verification of WarnAndDropIfFull logging behaviour.This test excellently validates both the functional behaviour (frame discarding) and the logging side effect (warning message) of the
WarnAndDropIfFullpolicy. The log message content assertion usingcontains("push queue full")provides appropriate validation without being overly brittle.
leynos
deleted the
codex/add-tests-for-dropiffull-and-warnanddropiffull-policies
branch
1 participant
Summary
Testing
make fmtmake lintmake testmarkdownlint --fix *.md **/*.md(fails: violations in existing files)nixie docs/rust-testing-with-rstest-fixtures.mdhttps://chatgpt.com/codex/tasks/task_e_6859b5beecc0832292e48a740eb40b9a
Summary by Sourcery
Integrate the logtest crate and a LoggerHandle fixture to enable exclusive, verifiable log capture in push queue policy tests and update documentation accordingly
New Features:
Enhancements:
Build:
Documentation:
Tests:
Summary by CodeRabbit
logtestcrate for verifying log output during tests, with usage examples and setup instructions.logtestas a development dependency.