Fix incorrect Notified await and remove stray lint expectation#101
Fix incorrect Notified await and remove stray lint expectation#101
Conversation
This reverts commit 8dd43a5732e21f18db751915ee7ca78fcdd202a9.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThe PR updates async tests to await Notify.notified() futures directly (removing mutable borrows to satisfy Unpin) and removes a redundant lint expectation on the TestComplexity enum. File-Level Changes
Possibly linked issues
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 9 minutes and 8 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 (2)
Summary by CodeRabbit
WalkthroughRefactor test awaiting of Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
crates/comenqd/src/daemon.rs(1 hunks)crates/comenqd/tests/daemon.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider using Arc to reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not use return in single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Use rstest fixtures for shared setup.
Replace duplicated tests with #[rstest(...)] parameterised cases.
Prefer mockall for mocks/stubs.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary. Use eyre::Report for human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library. Convert to domain enums at API boundaries, and to eyre only in the main main() entrypoint or top-level async task.
Files:
crates/comenqd/src/daemon.rscrates/comenqd/tests/daemon.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
crates/comenqd/src/daemon.rscrates/comenqd/tests/daemon.rs
🧬 Code graph analysis (2)
crates/comenqd/src/daemon.rs (1)
crates/comenqd/src/listener.rs (3)
handle_client(117-136)prepare_listener(34-44)run_listener(57-103)
crates/comenqd/tests/daemon.rs (2)
crates/comenqd/src/worker.rs (1)
run_worker(161-223)crates/comenqd/tests/util.rs (1)
timeout_with_retries(66-92)
🔍 Remote MCP Ref
Summary of relevant facts for reviewing this PR
-
tokio::sync::futures::Notified is not Unpin (Notified<'_> implements !Unpin). Awaiting a Notified by reference (&mut notified).await therefore requires Unpin; awaiting the future by value (notified().await) does not. This explains why the tests were changed to clone the Notify handle and await notified().await instead of awaiting a previously bound &mut Notified.
-
tokio::sync::Notify docs show the common pattern for one‑off waits is notify.notified().await; when reusing the same Notified future across iterations you must pin it (e.g., tokio::pin!) and use as_mut().await. Thus the PR’s approach (clone the Arc and call notified().await inside the closure) is the recommended simple fix when the future is not intended to be reused.
Files/changes this validates in the PR
- Tests switching from awaiting &mut Notified to awaiting notified().await by-value — fixes the Unpin requirement and matches tokio docs.
- Removing the stray #[expect(dead_code,...)] attribute on TestComplexity::Simple is a non-functional lint change (no runtime effect).
Sources/tools used
- docs.rs — tokio::sync::futures::Notified struct docs (Notified !Unpin)
- docs.rs — tokio::sync::Notify struct docs (notified().await pattern, pinning when reusing)
- Documentation search (query for Notified Unpin)
🔇 Additional comments (4)
crates/comenqd/tests/daemon.rs (4)
292-293: PassArc<Notify>into hooks (notNotified)Bind the hook to the
Notifyhandle and await per‑attempt futures elsewhere. This keeps hooks simple and avoids pinning concerns.
297-305: AwaitNotify::notified()by value inside the retry closure (fixes !Unpin and FnMut)Create a fresh
Notifiedon each retry and await it by value. This removes theUnpinrequirement and satisfies theFnMutbound oftimeout_with_retries.Ensure CI runs these tests on a slower runner to smoke out timing flakiness.
359-360: Hook enqueued notification viaArc<Notify>cloneFeed the handle, not a pre‑created waiter. This mirrors the drained path and avoids lifetime/pinning pitfalls.
366-371: Generate a new waiter per attempt for enqueued pathCall
enqueued.notified().awaitinside the closure so retries remain valid and independent.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/comenqd/tests/util.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider using Arc to reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not use return in single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Use rstest fixtures for shared setup.
Replace duplicated tests with #[rstest(...)] parameterised cases.
Prefer mockall for mocks/stubs.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary. Use eyre::Report for human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library. Convert to domain enums at API boundaries, and to eyre only in the main main() entrypoint or top-level async task.
Files:
crates/comenqd/tests/util.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
crates/comenqd/tests/util.rs
🔍 Remote MCP Ref
Summary of additional relevant facts for the review
-
tokio::sync::futures::Notified is not Unpin (Notified implements !Unpin). Awaiting a Notified via a mutable reference (&mut notified).await requires the future to be Unpin; this is why the original tests failed lint/compile when holding a Notified and awaiting it by reference.
-
The documented, idiomatic usage for one‑off waits is to call notify.notified().await (await the future by value). If you must reuse the same Notified future across awaits, you must pin it (e.g., tokio::pin!) and use as_mut().await. The PR’s change—clone the Arc and call notified().await inside the closure—matches the recommended pattern and avoids Unpin issues.
-
Removing the stray #[expect(dead_code, ...)] attribute on TestComplexity::Simple is purely a lint/test-usage cleanup with no behavioral/runtime impact. (No external citation required; change is local to tests in the PR.)
Sources/tools used
- docs.rs — tokio::sync::futures::Notified documentation
- docs.rs — tokio::sync::Notify documentation (notified/pinning guidance)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
1 participant
Summary
TestComplexityNotifiedinstances without borrowing to satisfyUnpinTesting
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_68c6ae2b6a308322a72309ae796d7507
Summary by Sourcery
Remove unused dead_code lint expectation and streamline Notify::notified() usage in tests by awaiting the returned futures directly rather than through mutable references
Bug Fixes:
Enhancements:
&mutTests:
Notify::notified()futures directly