Move server tests out of module #237
Conversation
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 29 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 (3)
Summary by CodeRabbit
WalkthroughRelocate the worker task and connection processing logic from Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TcpListener
participant WorkerTask
participant WireframeApp
Client->>TcpListener: Initiate TCP connection
TcpListener->>WorkerTask: Accept connection
WorkerTask->>WorkerTask: Decode preamble
alt Preamble success
WorkerTask->>WireframeApp: Create app instance with connection
WorkerTask->>WorkerTask: Invoke success callback (if any)
else Preamble failure
WorkerTask->>WorkerTask: Invoke failure callback (if any)
WorkerTask->>Client: Drop connection
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 (
|
Reviewer's GuideThis PR refactors the server implementation by extracting connection-handling logic into a dedicated worker module, cleaning up imports and exposing testing hooks, and migrating the bulk of tests into standalone integration test files. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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.
- In worker_task and process_stream you still use eprintln! for errors; switch to tracing::error! (or another tracing macro) so logging is consistent with the rest of the server.
- The two test files under tests/ largely duplicate types and fixtures (e.g. TestPreamble, EmptyPreamble, factory, free_port); consider extracting those into a shared test utility to DRY up the suite.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In worker_task and process_stream you still use eprintln! for errors; switch to tracing::error! (or another tracing macro) so logging is consistent with the rest of the server.
- The two test files under tests/ largely duplicate types and fixtures (e.g. TestPreamble, EmptyPreamble, factory, free_port); consider extracting those into a shared test utility to DRY up the suite.
## Individual Comments
### Comment 1
<location> `src/server/worker.rs:103` </location>
<code_context>
+/// // process_stream::<_, ()>(stream, factory, None, None).await;
+/// # }
+/// ```
+pub(crate) async fn process_stream<F, T>(
+ mut stream: tokio::net::TcpStream,
+ factory: F,
</code_context>
<issue_to_address>
Consider making process_stream private if not used outside this module.
If only used in this module, remove the visibility modifier. Use `pub(crate)` only if accessed from other modules or tests.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
+pub(crate) async fn process_stream<F, T>(
=======
+async fn process_stream<F, T>(
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@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: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/server.rs(3 hunks)src/server/worker.rs(1 hunks)tests/server_runtime.rs(1 hunks)tests/server_runtime_more.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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_runtime_more.rssrc/server/worker.rssrc/server.rstests/server_runtime.rs
🧬 Code Graph Analysis (3)
src/server/worker.rs (4)
src/extractor.rs (2)
std(270-270)peer_addr(362-362)tests/server_runtime_more.rs (1)
factory(32-34)tests/server_runtime.rs (1)
factory(27-29)src/server.rs (2)
new(91-107)bind(268-274)
src/server.rs (2)
tests/preamble.rs (3)
tokio(131-131)tokio(132-132)oneshot(73-73)src/server/worker.rs (2)
worker_task(24-76)worker_task(161-161)
tests/server_runtime.rs (3)
tests/server_runtime_more.rs (3)
server(131-131)factory(32-34)free_port(37-41)src/server/worker.rs (1)
factory(148-150)src/server.rs (6)
new(91-107)bind(268-274)worker_count(227-227)local_addr(231-233)has_preamble_success(235-235)has_preamble_failure(237-237)
🔇 Additional comments (8)
src/server/worker.rs (2)
1-18: Module structure looks good!The module documentation clearly describes its purpose, and imports are well-organised.
19-76: Excellent implementation of worker task with robust error handling!The panic catching mechanism with peer address logging and exponential backoff for accept errors demonstrates good defensive programming.
src/server.rs (3)
29-36: Clean module refactoring!The worker module extraction and test-only export are well-structured.
234-237: Appropriate test helper methods.The hidden accessor methods provide clean test interfaces without polluting the public API.
358-360: Seamless integration with extracted worker module.The refactoring maintains the same functionality while improving code organisation.
tests/server_runtime.rs (2)
1-52: Well-structured test helpers and fixtures!Good use of
#[expect]with reason, and fixtures properly reduce test duplication.
140-256: Excellent callback and lifecycle tests!Good use of atomic counters for callback verification and proper panic testing with expected messages.
tests/server_runtime_more.rs (1)
1-41: Consistent test setup across test modules.Good reuse of patterns from server_runtime.rs.
| #[cfg(test)] | ||
| mod tests { | ||
| use rstest::{fixture, rstest}; | ||
| use tokio::{ | ||
| net::TcpListener, | ||
| time::{Duration, timeout}, | ||
| }; | ||
| use tokio_util::{sync::CancellationToken, task::TaskTracker}; | ||
|
|
||
| use super::*; | ||
|
|
||
| #[fixture] | ||
| fn factory() -> impl Fn() -> WireframeApp + Send + Sync + Clone + 'static { | ||
| || WireframeApp::default() | ||
| } | ||
|
|
||
| #[rstest] | ||
| #[tokio::test] | ||
| async fn test_worker_task_shutdown_signal( | ||
| factory: impl Fn() -> WireframeApp + Send + Sync + Clone + 'static, | ||
| ) { | ||
| let token = CancellationToken::new(); | ||
| let tracker = TaskTracker::new(); | ||
| let listener = Arc::new(TcpListener::bind("127.0.0.1:0").await.unwrap()); | ||
|
|
||
| tracker.spawn(worker_task::<_, ()>( | ||
| listener, | ||
| factory, | ||
| None, | ||
| None, | ||
| token.clone(), | ||
| tracker.clone(), | ||
| )); | ||
|
|
||
| token.cancel(); | ||
| tracker.close(); | ||
|
|
||
| let result = timeout(Duration::from_millis(100), tracker.wait()).await; | ||
| assert!(result.is_ok()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Test implementation follows best practices.
Good use of rstest fixtures and focused test scope. Consider adding tests for accept error retry logic and panic handling scenarios.
Would you like me to generate additional test cases for error scenarios?
🤖 Prompt for AI Agents
In src/server/worker.rs around lines 136 to 176, the current test covers normal
shutdown signaling but lacks coverage for error retry logic and panic handling
in the worker task. Add new async tests that simulate accept errors on the
TcpListener to verify retry behavior, and tests that induce panics within the
worker task to ensure proper handling and recovery. Use similar patterns with
rstest and tokio::test to maintain consistency.
| #[rstest] | ||
| #[tokio::test] | ||
| async fn test_multiple_worker_creation( | ||
| factory: impl Fn() -> WireframeApp + Send + Sync + Clone + 'static, | ||
| free_port: SocketAddr, | ||
| ) { | ||
| let _ = &factory; | ||
| let call_count = Arc::new(AtomicUsize::new(0)); | ||
| let call_count_clone = call_count.clone(); | ||
|
|
||
| let factory = move || { | ||
| call_count_clone.fetch_add(1, Ordering::SeqCst); | ||
| WireframeApp::default() | ||
| }; | ||
|
|
||
| let server = WireframeServer::new(factory) | ||
| .workers(3) | ||
| .bind(free_port) | ||
| .expect("Failed to bind"); | ||
|
|
||
| let shutdown_future = async { | ||
| tokio::time::sleep(Duration::from_millis(10)).await; | ||
| }; | ||
|
|
||
| let result = timeout( | ||
| Duration::from_millis(1000), | ||
| server.run_with_shutdown(shutdown_future), | ||
| ) | ||
| .await; | ||
|
|
||
| assert!(result.is_ok()); | ||
| assert!(result.unwrap().is_ok()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertion for factory invocation count.
The test sets up call counting but doesn't verify the expected number of factory invocations.
assert!(result.is_ok());
assert!(result.unwrap().is_ok());
+ // Verify factory was called 3 times (once per worker)
+ assert_eq!(call_count.load(Ordering::SeqCst), 3);📝 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.
| #[rstest] | |
| #[tokio::test] | |
| async fn test_multiple_worker_creation( | |
| factory: impl Fn() -> WireframeApp + Send + Sync + Clone + 'static, | |
| free_port: SocketAddr, | |
| ) { | |
| let _ = &factory; | |
| let call_count = Arc::new(AtomicUsize::new(0)); | |
| let call_count_clone = call_count.clone(); | |
| let factory = move || { | |
| call_count_clone.fetch_add(1, Ordering::SeqCst); | |
| WireframeApp::default() | |
| }; | |
| let server = WireframeServer::new(factory) | |
| .workers(3) | |
| .bind(free_port) | |
| .expect("Failed to bind"); | |
| let shutdown_future = async { | |
| tokio::time::sleep(Duration::from_millis(10)).await; | |
| }; | |
| let result = timeout( | |
| Duration::from_millis(1000), | |
| server.run_with_shutdown(shutdown_future), | |
| ) | |
| .await; | |
| assert!(result.is_ok()); | |
| assert!(result.unwrap().is_ok()); | |
| } | |
| #[rstest] | |
| #[tokio::test] | |
| async fn test_multiple_worker_creation( | |
| factory: impl Fn() -> WireframeApp + Send + Sync + Clone + 'static, | |
| free_port: SocketAddr, | |
| ) { | |
| let _ = &factory; | |
| let call_count = Arc::new(AtomicUsize::new(0)); | |
| let call_count_clone = call_count.clone(); | |
| let factory = move || { | |
| call_count_clone.fetch_add(1, Ordering::SeqCst); | |
| WireframeApp::default() | |
| }; | |
| let server = WireframeServer::new(factory) | |
| .workers(3) | |
| .bind(free_port) | |
| .expect("Failed to bind"); | |
| let shutdown_future = async { | |
| tokio::time::sleep(Duration::from_millis(10)).await; | |
| }; | |
| let result = timeout( | |
| Duration::from_millis(1000), | |
| server.run_with_shutdown(shutdown_future), | |
| ) | |
| .await; | |
| assert!(result.is_ok()); | |
| assert!(result.unwrap().is_ok()); | |
| // Verify factory was called 3 times (once per worker) | |
| assert_eq!(call_count.load(Ordering::SeqCst), 3); | |
| } |
🤖 Prompt for AI Agents
In tests/server_runtime_more.rs around lines 71 to 103, the test sets up a
call_count to track factory invocations but does not assert the expected number
of times the factory is called. Add an assertion after the server run completes
to verify that call_count matches the expected number of worker creations (3 in
this case) to ensure the factory was invoked correctly.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/server/worker.rs (1)
136-176: Add tests for error scenarios and panic handling.As previously suggested, add tests for accept error retry logic and panic handling scenarios to improve test coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/server/worker.rs(1 hunks)tests/server_helpers.rs(1 hunks)tests/server_runtime.rs(1 hunks)tests/server_runtime_more.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 usingArcto reduce the amount of data returned.
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 unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not usereturnin 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.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor 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 toeyreonly in the mainmain()entrypoint or top-level async task.
Files:
tests/server_helpers.rssrc/server/worker.rstests/server_runtime_more.rstests/server_runtime.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_helpers.rssrc/server/worker.rstests/server_runtime_more.rstests/server_runtime.rs
🧬 Code Graph Analysis (2)
tests/server_helpers.rs (4)
tests/server_runtime.rs (2)
server(56-56)WireframeServer(156-158)tests/server_runtime_more.rs (1)
server(115-115)src/server/worker.rs (1)
factory(148-150)src/server.rs (1)
bind(268-274)
tests/server_runtime.rs (6)
src/extractor.rs (1)
std(270-270)tests/server_runtime_more.rs (1)
server(115-115)tests/server_helpers.rs (5)
WireframeServer(49-49)bind_server(35-42)factory(18-20)free_port(23-27)server_with_preamble(45-50)src/server/worker.rs (1)
factory(148-150)src/server.rs (6)
new(91-107)worker_count(227-227)local_addr(231-233)has_preamble_success(235-235)has_preamble_failure(237-237)tests(494-991)src/session.rs (1)
get(43-51)
⏰ 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 (6)
src/server/worker.rs (3)
69-69: Error logging correctly uses tracing.The error logging has been properly updated to use
tracing::error!as previously suggested.
103-103: Function visibility is appropriately private.The
process_streamfunction is correctly private (default visibility) as it's only used within this module.
118-119: Error logging correctly uses tracing.The preamble callback error logging has been properly updated to use
tracing::error!as previously suggested.tests/server_runtime_more.rs (2)
53-87: Test correctly verifies factory invocation behaviour.The test properly asserts that the factory is not invoked until connections are made, with the count correctly remaining at 0 when no connections occur.
160-228: Test comprehensively verifies panic logging.The test properly captures and verifies that panics in connection tasks are logged with the expected message, task identifier, and peer address information.
tests/server_runtime.rs (1)
76-88: Bind test now properly validates privileged port binding failure.The test has been correctly updated to check for expected binding failure on privileged ports when not running with elevated privileges.
| #[allow(dead_code)] | ||
| pub fn bind_server<F>(factory: F, addr: SocketAddr) -> WireframeServer<F> |
There was a problem hiding this comment.
Replace #[allow(dead_code)] with #[expect(...)].
The coding guidelines forbid #[allow]. Use narrowly scoped #[expect(lint, reason = "...")] instead.
-#[allow(dead_code)]
+#[expect(dead_code, reason = "Used in integration tests")]
pub fn bind_server<F>(factory: F, addr: SocketAddr) -> WireframeServer<F>📝 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.
| #[allow(dead_code)] | |
| pub fn bind_server<F>(factory: F, addr: SocketAddr) -> WireframeServer<F> | |
| #[expect(dead_code, reason = "Used in integration tests")] | |
| pub fn bind_server<F>(factory: F, addr: SocketAddr) -> WireframeServer<F> |
🤖 Prompt for AI Agents
In tests/server_helpers.rs around lines 34 to 35, replace the attribute
#[allow(dead_code)] with #[expect(dead_code, reason = "explanation here")] to
comply with coding guidelines that forbid #[allow]. Use #[expect] with a
specific lint and a clear reason for the exception instead of a broad allow.
| #[allow(dead_code)] | ||
| pub fn server_with_preamble<F>(factory: F) -> WireframeServer<F, TestPreamble> |
There was a problem hiding this comment.
Replace #[allow(dead_code)] with #[expect(...)].
The coding guidelines forbid #[allow]. Use narrowly scoped #[expect(lint, reason = "...")] instead.
-#[allow(dead_code)]
+#[expect(dead_code, reason = "Used in integration tests")]
pub fn server_with_preamble<F>(factory: F) -> WireframeServer<F, TestPreamble>📝 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.
| #[allow(dead_code)] | |
| pub fn server_with_preamble<F>(factory: F) -> WireframeServer<F, TestPreamble> | |
| #[expect(dead_code, reason = "Used in integration tests")] | |
| pub fn server_with_preamble<F>(factory: F) -> WireframeServer<F, TestPreamble> |
🤖 Prompt for AI Agents
In tests/server_helpers.rs around lines 44 to 45, replace the attribute
#[allow(dead_code)] with #[expect(dead_code, reason = "...")], providing a clear
reason for expecting this lint. This change aligns with coding guidelines that
forbid #[allow] and prefer narrowly scoped #[expect] attributes.
1 participant
Summary
src/server/worker.rsmod testsfromsrc/server.rsintotests/server_runtime*.rsTesting
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_688bf634507c8322a842ee9c9bccda9a
Summary by Sourcery
Refactor the WireframeServer module by extracting worker logic into its own module and reorganize tests into integration suites
Enhancements:
Tests: