Conversation
Reviewer's GuideThis PR enriches and reorders documentation across the logging module and push queue tests, corrects a doctest warn! example, adjusts a crate-level usage example, and adds the Entity relationship diagram for logging module dependencieserDiagram
LOGGER ||--o{ LoggerHandle : "acquires"
LoggerHandle }o--|| Logger : "guards"
LoggerHandle }o--|| MutexGuard : "holds"
Logger ||--o{ logtest : "used by"
LoggerHandle ||--o{ log : "uses"
Class diagram for updated LoggerHandle struct and methodsclassDiagram
class LoggerHandle {
- guard: MutexGuard<'static, Logger>
+ new() LoggerHandle
}
LoggerHandle --|> Deref
LoggerHandle --|> DerefMut
class Logger
class MutexGuard
LoggerHandle o-- MutexGuard
MutexGuard o-- Logger
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughAdd descriptive documentation comments to test functions, fixtures, and helper methods in Changes
Sequence Diagram(s)No sequence diagram is necessary as all changes are documentation or dependency updates without control flow modifications. Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (1)**/*.rsInstructions used from: Sources:
⚙️ CodeRabbit Configuration File ⏰ Context from checks skipped due to timeout of 240000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
✨ 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}
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/logging.rs:23` </location>
<code_context>
/// Handle to the global logger with exclusive access.
///
/// This guard ensures tests do not interfere with each other's log capture by
-/// serialising access to a [`logtest::Logger`].
+/// serialising access to a [`logtest::Logger`]. Acquire it using [`logger`] or
+/// [`LoggerHandle::new`].
+///
+/// ```
+/// use wireframe_testing::logger;
+/// # use log::warn;
+///
+/// let mut log = logger();
+/// warn!("warned");
+/// assert!(log.pop().is_some());
+/// ```
pub struct LoggerHandle {
guard: MutexGuard<'static, Logger>,
}
impl LoggerHandle {
/// Acquire the global [`Logger`] instance.
+ ///
+ /// ```
+ /// use wireframe_testing::LoggerHandle;
+ ///
+ /// let _log = LoggerHandle::new();
+ /// ```
pub fn new() -> Self {
</code_context>
<issue_to_address>
The code example in the docstring for `LoggerHandle::new` does not demonstrate any log assertion or usage.
Expanding the example to include a log assertion or logger interaction would make the intended usage clearer for users.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
/// ```
/// use wireframe_testing::LoggerHandle;
///
/// let _log = LoggerHandle::new();
/// ```
=======
/// ```
/// use wireframe_testing::LoggerHandle;
/// # use log::warn;
///
/// let mut log = LoggerHandle::new();
/// warn!("warned");
/// assert!(log.guard.pop().is_some());
/// ```
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| /// ``` | ||
| /// use wireframe_testing::LoggerHandle; | ||
| /// | ||
| /// let _log = LoggerHandle::new(); | ||
| /// ``` |
There was a problem hiding this comment.
suggestion: The code example in the docstring for LoggerHandle::new does not demonstrate any log assertion or usage.
Expanding the example to include a log assertion or logger interaction would make the intended usage clearer for users.
| /// ``` | |
| /// use wireframe_testing::LoggerHandle; | |
| /// | |
| /// let _log = LoggerHandle::new(); | |
| /// ``` | |
| /// ``` | |
| /// use wireframe_testing::LoggerHandle; | |
| /// # use log::warn; | |
| /// | |
| /// let mut log = LoggerHandle::new(); | |
| /// warn!("warned"); | |
| /// assert!(log.guard.pop().is_some()); | |
| /// ``` |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (5)
wireframe_testing/src/logging.rs (1)
68-72: Replace forbidden#[allow]with scoped#[expect]Project conventions forbid blanket
allow. Useexpectto document intent and keep lints active elsewhere.-#[allow( - unused_braces, - reason = "rustc false positive for single line rstest fixtures" -)] +#[expect(unused_braces, reason = "rustc false positive for single-line rstest fixtures")]tests/push.rs (3)
56-63: Mention virtual time in commentReaders unfamiliar with
tokio::time::pausemay not realise time is frozen. Add a clause explaining virtual-time context.
106-125: Spell out inter-queue interferenceAdd a brief note that limiter token bucket is shared between priorities to reinforce lesson.
141-164: Highlight burst size equals capacityDocument that the permitted burst equals
capacityparameter to aid future maintainers tweaking limits.tests/push_policies.rs (1)
14-26: Remove forbidden#[allow]The same lint-suppression policy applies here. Switch to
#[expect(unused_braces, reason = ...)].
📜 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 (5)
tests/push.rs(8 hunks)tests/push_policies.rs(5 hunks)wireframe_testing/Cargo.toml(1 hunks)wireframe_testing/src/lib.rs(1 hunks)wireframe_testing/src/logging.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
⚙️ CodeRabbit Configuration File
⏰ Context from checks skipped due to timeout of 240000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (11)
wireframe_testing/Cargo.toml (1)
13-13: Add feature flags or pin the patch version for reproducibilityThe blanket
"0.4"version range may pull in semver-compatible but behaviour-changing patch versions during CI vs production. Pin the patch level or document why minor drift is acceptable.wireframe_testing/src/logging.rs (1)
6-16: Good example illustrates fixture usageThe doctest clearly shows the happy-path and compiles as a standalone test function, so documentation value is high.
tests/push.rs (4)
9-12: Doc comment adds intent – 👍
42-54: Doc correctly documents closed-queue path
92-104: Doc emphasises rate-limit window – good
127-139: Comment captures unlimited-queue guarantee – 👍tests/push_policies.rs (5)
27-67: Comment thoroughly describes parameterised matrix – 👍
69-86: Doc clear on DLQ forwarding
97-103: Great asynchronous assertion helper
105-107: Helper correctly does nothing – fine
108-152: Comprehensive log-error validation – 👍
| //! # async fn example(app: WireframeApp) { | ||
| //! let bytes = drive_with_bincode(app, 42u8).await.unwrap(); | ||
| //! # } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Update surrounding prose to match simplified generic
The removed generics narrow reader focus, but the preceding paragraph still talks about “generic parameters”. Remove or adjust that wording for consistency.
🤖 Prompt for AI Agents
In wireframe_testing/src/lib.rs around lines 11 to 13, the documentation still
refers to "generic parameters" even though the code example has been simplified
to remove generics. Update the surrounding prose to remove or rephrase mentions
of generic parameters so it accurately reflects the simplified example and
maintains consistency.
1 participant
Summary
wireframe_testinglogging docswarn!examplelogcrate for doctestsTesting
make lintmake testcargo test -p wireframe_testing --dochttps://chatgpt.com/codex/tasks/task_e_68704f8b047c83228f77d571ea0d49ad
Summary by Sourcery
Improve documentation and examples for logging and push queue tests, fix a doctest example, and add the necessary log dependency for wireframe_testing.
Bug Fixes:
Enhancements:
Build:
Documentation: