Conversation
Reviewer's GuideThis PR centralizes test fixtures by introducing a shared File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughIntroduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Module
participant Common as common/mod.rs
participant Server as WireframeServer
Test->>Common: Request unused_listener()
Common-->>Test: Return StdTcpListener
Test->>Server: Call bind_listener(listener)
Server-->>Test: Return server bound to listener
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
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:
- The unused_port helper drops the listener before the server bind, which can introduce a race—consider binding the server directly on the listener or holding it until the server is bound.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider renaming tests/common to something like tests/fixtures or tests/helpers to make its purpose clearer and avoid ambiguity with other common code.
- The unused_port helper drops the listener before the server bind, which can introduce a race—consider binding the server directly on the listener or holding it until the server is bound.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}
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
(1 files improve in Code Health)
Gates Passed
See analysis details in CodeScene
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| push.rs | 9.69 → 10.00 | Complex Conditional |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
leynos
force-pushed
the
codex/add-shared-helpers-module-for-tests
branch
from
c8611a5 to
802519a
Compare
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/server.rs(1 hunks)tests/common/mod.rs(1 hunks)tests/preamble.rs(2 hunks)tests/server.rs(2 hunks)tests/world.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Function documentation must include clear examples demonstrating the usage and outcome of the function. Test documentation should omit examples where the example serves only to reiterate the test logic.
No single code file may be longer than 400 lines. Long switch statements or dispatch tables should be broken up by feature and constituents colocated with targets. Large blocks of test data should be moved to external data files.
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.
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....
Files:
tests/server.rssrc/server.rstests/world.rstests/preamble.rstests/common/mod.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.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.
Files:
tests/server.rssrc/server.rstests/world.rstests/preamble.rstests/common/mod.rs
🧬 Code Graph Analysis (3)
tests/server.rs (1)
tests/common/mod.rs (2)
factory(22-24)unused_listener(9-12)
tests/world.rs (1)
tests/common/mod.rs (1)
unused_listener(9-12)
tests/preamble.rs (1)
tests/common/mod.rs (2)
factory(22-24)unused_listener(9-12)
⏰ 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 (9)
tests/world.rs (2)
12-14: Excellent refactoring to use shared test utilities.The import of
unused_listenerfrom the common module properly centralises test setup and eliminates hardcoded addresses.
31-35: Proper implementation of listener-based binding.The switch from
.bind("127.0.0.1:0")to usingunused_listener()with.bind_listener()correctly utilises the new server method whilst maintaining the same functional behaviour.tests/preamble.rs (1)
7-8: Good refactoring to shared test utilities.Using the common module's
factoryandunused_listenerproperly eliminates code duplication across test files.src/server.rs (1)
277-286: Well-implemented listener binding method.The
bind_listenermethod properly mirrors the existingbindmethod's pattern whilst accepting a pre-created listener. The error handling and non-blocking configuration are correctly implemented.tests/common/mod.rs (3)
1-4: Excellent module documentation.The module comment clearly explains the purpose and utility of the shared test fixtures, following the coding guidelines requirement for module-level documentation.
8-12: Clean implementation of unused listener utility.The function correctly binds to localhost with an ephemeral port (0) and handles the binding error appropriately.
17-24: Proper rstest fixture implementation.The factory fixture correctly returns a closure that creates
WireframeAppinstances, and the allow attribute is properly justified for the rustc false positive.tests/server.rs (2)
3-4: Good adoption of shared test utilities.Importing and using the common module's
factoryandunused_listenerproperly eliminates duplication of test setup code.
9-9: Consistent refactoring to shared factory.All test functions now properly use the shared
factory()instead of inlineWireframeApp::new()closures, improving consistency across the test suite.Also applies to: 16-16, 22-22, 28-28, 44-44
| #[fixture] | ||
| #[allow( | ||
| unused_braces, | ||
| reason = "rustc false positive for single line rstest fixtures" | ||
| )] | ||
| pub fn unused_port() -> SocketAddr { | ||
| unused_listener() | ||
| .local_addr() | ||
| .expect("failed to obtain local addr") | ||
| } |
There was a problem hiding this comment.
Potential race condition in unused_port fixture.
The unused_port fixture creates a listener, extracts its address, then drops the listener. This introduces a race condition where another process could bind to the same port before the test uses it.
Replace the fixture with direct usage of unused_listener() in tests that need a bound listener, or ensure the listener remains alive until the test binds to it.
-#[fixture]
-#[allow(
- unused_braces,
- reason = "rustc false positive for single line rstest fixtures"
-)]
-pub fn unused_port() -> SocketAddr {
- unused_listener()
- .local_addr()
- .expect("failed to obtain local addr")
-}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/common/mod.rs around lines 26 to 35, the unused_port fixture creates a
listener, extracts its address, and then drops the listener, causing a race
condition where the port may be taken before use. To fix this, modify tests to
use unused_listener() directly so the listener stays alive during the test, or
change the fixture to return the listener itself instead of just the address,
ensuring the port remains bound until the test completes.
| let listener = unused_listener(); | ||
| let _addr = listener.local_addr().expect("addr"); | ||
| let server = server.bind_listener(listener).expect("bind"); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove unused variable assignment.
Line 69 assigns listener.local_addr() to _addr but this value is never used. The actual server address is obtained from server.local_addr() on line 71.
- let listener = unused_listener();
- let _addr = listener.local_addr().expect("addr");
- let server = server.bind_listener(listener).expect("bind");
+ let listener = unused_listener();
+ let server = server.bind_listener(listener).expect("bind");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let listener = unused_listener(); | |
| let _addr = listener.local_addr().expect("addr"); | |
| let server = server.bind_listener(listener).expect("bind"); | |
| let listener = unused_listener(); | |
| let server = server.bind_listener(listener).expect("bind"); |
🤖 Prompt for AI Agents
In tests/preamble.rs around lines 68 to 70, the variable _addr is assigned the
value of listener.local_addr() but never used. Remove the assignment to _addr on
line 69 to eliminate the unused variable, since the server address is correctly
obtained later from server.local_addr().
| let listener = unused_listener(); | ||
| let _addr = listener.local_addr().unwrap(); | ||
| let server = WireframeServer::new(factory()) | ||
| .workers(1) | ||
| .bind( | ||
| "127.0.0.1:0" | ||
| .parse() | ||
| .expect("hard-coded socket address must be valid"), | ||
| ) | ||
| .expect("bind failed"); | ||
| .bind_listener(listener) | ||
| .unwrap(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove unused variable assignment.
Line 43 assigns listener.local_addr() to _addr but this value is never used. The server address is obtained from server.local_addr() on line 49.
- let listener = unused_listener();
- let _addr = listener.local_addr().unwrap();
- let server = WireframeServer::new(factory())
+ let listener = unused_listener();
+ let server = WireframeServer::new(factory())
.workers(1)
.bind_listener(listener)
- .unwrap();
+ .unwrap();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let listener = unused_listener(); | |
| let _addr = listener.local_addr().unwrap(); | |
| let server = WireframeServer::new(factory()) | |
| .workers(1) | |
| .bind( | |
| "127.0.0.1:0" | |
| .parse() | |
| .expect("hard-coded socket address must be valid"), | |
| ) | |
| .expect("bind failed"); | |
| .bind_listener(listener) | |
| .unwrap(); | |
| let listener = unused_listener(); | |
| let server = WireframeServer::new(factory()) | |
| .workers(1) | |
| .bind_listener(listener) | |
| .unwrap(); |
🤖 Prompt for AI Agents
In tests/server.rs around lines 42 to 47, the variable _addr is assigned the
value of listener.local_addr() but is never used later. Remove the assignment to
_addr on line 43 to clean up the code, as the server address is correctly
obtained from server.local_addr() on line 49.
1 participant
Summary
tests/common/mod.rsfactoryandunused_porthelpers across testsTesting
make lintmake testmake markdownlinthttps://chatgpt.com/codex/tasks/task_e_688bf55eae1c83228070b5d4ae1185f4
Summary by Sourcery
Centralise test fixtures by moving the factory and port-binding helpers into a shared common module and update existing tests to import and use these shared fixtures, removing their local definitions.
Tests: