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 9 minutes and 19 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 (6)
Summary by CodeRabbit
WalkthroughReplace all Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Function
participant Macro as push_expect!/recv_expect!
participant Future as Async Operation
Test->>Macro: push_expect!(future)
Macro->>Future: await
Future-->>Macro: Result (Ok or Err)
alt Ok
Macro-->>Test: Continue
else Err
Macro-->>Test: Panic with error message (file:line)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 has finished reviewing your pull request! 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 - here's some feedback:
- There’s a lot of repeated expect("... failed") boilerplate in the tests—consider extracting a small helper or macro (e.g.
push_expect/recv_expect) to reduce noise and improve readability. - Many of the
expect("recv failed")messages are identical; adding more context (like queue type or test name) would make it easier to pinpoint which receive call actually failed. - A couple of
expectcalls made in non-test code (e.g.bounded_no_rate_limit) will panic in library code—consider whether these should return aResultor use adebug_assert!instead to avoid unexpected panics in production.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There’s a lot of repeated expect("... failed") boilerplate in the tests—consider extracting a small helper or macro (e.g. `push_expect` / `recv_expect`) to reduce noise and improve readability.
- Many of the `expect("recv failed")` messages are identical; adding more context (like queue type or test name) would make it easier to pinpoint which receive call actually failed.
- A couple of `expect` calls made in non-test code (e.g. `bounded_no_rate_limit`) will panic in library code—consider whether these should return a `Result` or use a `debug_assert!` instead to avoid unexpected panics in production.
## Individual Comments
### Comment 1
<location> `src/push.rs:312` </location>
<code_context>
low_capacity: usize,
) -> (Self, PushHandle<F>) {
- Self::bounded_with_rate_dlq(high_capacity, low_capacity, None, None).unwrap()
+ Self::bounded_with_rate_dlq(high_capacity, low_capacity, None, None)
+ .expect("bounded_no_rate_limit should not fail")
}
</code_context>
<issue_to_address>
The expect message assumes infallibility; consider documenting why failure is impossible.
Please add a comment explaining why Self::bounded_with_rate_dlq cannot fail with these arguments.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
Self::bounded_with_rate_dlq(high_capacity, low_capacity, None, None)
.expect("bounded_no_rate_limit should not fail")
=======
// bounded_with_rate_dlq cannot fail here because both high_capacity and low_capacity are valid,
// and passing None for the rate limiter and DLQ disables those features, which is always supported.
Self::bounded_with_rate_dlq(high_capacity, low_capacity, None, None)
.expect("bounded_no_rate_limit should not fail")
>>>>>>> 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: 1
♻️ Duplicate comments (4)
tests/connection_actor.rs (4)
79-88: Duplicate of the earlier comment: harmonise wording (“high-priority”), and switch topush_expect!.
213-232: Duplicate of the first comment: adoptpush_expect!and consistent hyphenation.
272-276: Duplicate of the first comment: unify wording and usepush_expect!.
406-410: Duplicate of the first comment: unify wording and usepush_expect!.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (21)
docs/wireframe-testing-crate.md(1 hunks)src/frame/tests.rs(2 hunks)src/push.rs(1 hunks)src/server.rs(10 hunks)tests/app_data.rs(2 hunks)tests/async_stream.rs(1 hunks)tests/connection_actor.rs(14 hunks)tests/extractor.rs(3 hunks)tests/lifecycle.rs(4 hunks)tests/metadata.rs(1 hunks)tests/middleware.rs(1 hunks)tests/middleware_order.rs(1 hunks)tests/preamble.rs(6 hunks)tests/push.rs(8 hunks)tests/push_policies.rs(4 hunks)tests/response.rs(5 hunks)tests/routes.rs(4 hunks)tests/server.rs(2 hunks)tests/session_registry.rs(1 hunks)tests/wireframe_protocol.rs(3 hunks)wireframe_testing/src/helpers.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/routes.rstests/app_data.rstests/server.rstests/middleware.rswireframe_testing/src/helpers.rstests/metadata.rstests/async_stream.rstests/push_policies.rstests/response.rstests/middleware_order.rstests/lifecycle.rssrc/frame/tests.rssrc/push.rstests/extractor.rstests/wireframe_protocol.rstests/preamble.rstests/session_registry.rstests/connection_actor.rstests/push.rssrc/server.rs
**/*.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/wireframe-testing-crate.md
🧬 Code Graph Analysis (6)
tests/routes.rs (2)
src/message.rs (1)
from_bytes(50-55)src/app.rs (6)
new_with_envelope(270-270)default(234-247)new(201-201)new(257-257)id(179-179)id(209-209)
tests/app_data.rs (2)
src/extractor.rs (4)
from_message_request(184-187)from_message_request(265-271)from_message_request(328-335)from_message_request(372-379)tests/extractor.rs (2)
request(18-18)empty_payload(25-25)
tests/push_policies.rs (1)
src/push.rs (2)
bounded_with_rate_dlq(384-418)recv(438-444)
src/frame/tests.rs (1)
src/frame/conversion.rs (2)
bytes_to_u64(49-68)u64_to_bytes(78-114)
tests/wireframe_protocol.rs (4)
src/connection.rs (3)
new(30-34)new(139-152)new(511-520)src/app.rs (3)
new(201-201)new(257-257)protocol(424-428)tests/connection_actor.rs (1)
queues(24-24)src/push.rs (1)
bounded(288-291)
tests/session_registry.rs (1)
tests/connection_actor.rs (1)
queues(24-24)
🔇 Additional comments (54)
src/push.rs (1)
312-318: Excellent improvement addressing past feedback.The implementation now includes comprehensive documentation explaining why
bounded_with_rate_dlqcannot fail when rate limiting is disabled, plus a debug assertion as a regression guard. This addresses the previous review concern about documenting infallibility assumptions.wireframe_testing/src/helpers.rs (2)
282-282: LGTM - Clear error message.The replacement provides a descriptive panic message that clearly indicates the failure source.
304-334: Excellent macro design for standardised test assertions.The
push_expect!andrecv_expect!macros provide consistent error handling with call site information. The dual variants (with/without custom messages) offer flexibility whilst maintaining good diagnostics.tests/middleware.rs (1)
58-58: LGTM - Descriptive error message.The replacement improves test diagnostics by clearly indicating middleware call failures.
tests/middleware_order.rs (3)
57-63: Excellent error message specificity.Each
expect()call provides precise context about which operation failed, greatly improving test debugging capabilities.
69-75: Clear I/O operation error handling.The error messages accurately describe the specific I/O operations that could fail during the test.
80-91: Comprehensive error coverage for async operations.All async operations now have descriptive error messages, making test failures much easier to diagnose.
tests/async_stream.rs (1)
32-32: LGTM - Clear actor operation context.The replacement provides specific context about the actor run failure, improving test diagnostics.
tests/metadata.rs (1)
22-22: Excellent error message improvements!The replacement of
.unwrap()with.expect()provides clear failure context. The messages "failed to create app" and "route registration failed" are descriptive and will aid debugging.Also applies to: 26-26
tests/response.rs (1)
43-43: Systematic improvement to error diagnostics!The comprehensive replacement of
.unwrap()with.expect()throughout the file provides excellent error context. Each message is specific to the operation (e.g., "send_response failed", "decode failed", "deserialize failed"), which will significantly aid debugging test failures.Also applies to: 48-50, 54-58, 70-70, 82-82, 123-123, 125-128, 135-135, 193-193
tests/routes.rs (1)
44-44: Thorough enhancement to test error reporting!The systematic replacement of
.unwrap()with.expect()provides clear, operation-specific error messages throughout the test functions. Messages like "encode failed", "decode failed", and "deserialize failed" will make test failures much easier to diagnose.Also applies to: 56-57, 70-75, 83-83, 89-89, 93-93, 98-100, 104-104, 116-117, 120-121, 124-125, 128-129
tests/lifecycle.rs (1)
53-53: Consistent error handling improvements!The replacement of
.unwrap()with.expect()throughout the lifecycle tests provides clear failure context. Messages like "setup callback", "teardown callback", and "route registration failed" are descriptive and will facilitate debugging.Also applies to: 55-55, 57-57, 78-78, 80-80, 93-93, 95-95, 124-124, 130-132, 136-136, 138-140
docs/wireframe-testing-crate.md (1)
120-130: Clear documentation of new helper macros!The new "Helper macros" section effectively documents the
push_expect!andrecv_expect!macros. The explanation of their purpose and the usage examples are clear and helpful for users wanting to standardise error handling in asynchronous test code.tests/session_registry.rs (1)
34-35: LGTM! Error messages improve test diagnostics.The replacement of
.unwrap()with.expect()calls providing clear context ("push failed", "recv failed") aligns with the coding guidelines and enhances debugging when these async operations fail.tests/app_data.rs (2)
34-35: LGTM! Clear error message for state extraction.The descriptive error message "failed to extract shared state" provides valuable context for debugging test failures during shared state extraction operations.
45-46: LGTM! Appropriate error message for error path testing.The error message "missing state error expected" clearly indicates this is testing an expected error condition, improving test clarity and debugging.
tests/push_policies.rs (9)
43-49: LGTM! Consistent error messaging for push operations.The descriptive error messages "push high priority failed" and "try_push failed" provide clear context for debugging async push operation failures.
51-51: LGTM! Clear error message for recv operation.The error message "recv failed" provides appropriate context for debugging queue receive operation failures.
79-80: LGTM! Descriptive error message for queue creation.The error message "queue creation failed" clearly identifies the failure point during queue setup operations.
82-88: LGTM! Consistent error messaging for async operations.The error messages maintain consistency across push operations, improving test diagnostics without altering functionality.
90-92: LGTM! Clear error messages for recv operations.The descriptive error messages "recv failed" and "dlq recv failed" provide specific context for debugging different receive operation failures.
98-98: LGTM! Appropriate error message for send operation.The error message "send failed" provides clear context for debugging channel send operation failures.
108-108: LGTM! Consistent error messaging for DLQ operations.The error message "dlq recv failed" maintains consistency with other receive operation error messages.
138-147: LGTM! Systematic error handling improvements.The consistent application of descriptive error messages across queue creation and push operations enhances test diagnostics throughout the error scenario tests.
149-149: LGTM! Final recv operation properly handled.The error message "recv failed" completes the systematic improvement of error handling across all async operations in this test file.
tests/extractor.rs (4)
38-38: LGTM! Clear error message for serialisation failure.The error message "failed to serialise message" provides appropriate context for debugging message serialisation operations.
41-42: LGTM! Descriptive error message for extraction failure.The error message "failed to extract TestMsg from payload" clearly identifies both the operation and the specific type being extracted, improving test diagnostics.
55-56: LGTM! Appropriate error message for connection info extraction.The error message "failed to build ConnectionInfo" provides clear context for debugging connection information extraction failures.
70-71: LGTM! Consistent error messaging for shared state extraction.The error message "failed to extract shared state" maintains consistency with similar operations across test files while providing clear debugging context.
src/frame/tests.rs (2)
23-26: LGTM! Clear error message for conversion operation.The error message "failed to convert" provides appropriate context for debugging failures in the
bytes_to_u64conversion function during parameterised testing.
44-44: LGTM! Descriptive error message for encoding operation.The error message "failed to encode u64" clearly identifies the specific encoding operation that failed, improving test diagnostics for the
u64_to_bytesfunction.tests/preamble.rs (1)
71-73: LGTM! Excellent error message improvements.The replacement of
.unwrap()with.expect()calls provides clear, contextual error messages that will significantly improve debugging when failures occur. All messages are descriptive and specific to their failure contexts.Also applies to: 82-82, 87-87, 94-95, 100-100, 110-111, 145-145, 155-155, 163-165, 205-206, 214-214, 216-216, 218-218
tests/push.rs (3)
8-8: Good addition of helper macros.Import of
push_expect!andrecv_expect!macros improves test consistency and error handling standardisation.
15-16: Excellent use of helper macros for async operations.The consistent use of
push_expect!andrecv_expect!macros throughout the tests improves readability and provides better error context for async operations compared to direct.await.unwrap()calls.Also applies to: 18-19, 35-35, 41-42, 73-74, 90-91, 94-95, 105-105, 107-107, 109-110, 122-122, 128-128, 130-131, 143-143, 147-148, 161-161, 171-171, 174-174
69-70: Clear queue creation error messages.The
.expect("queue creation failed")messages provide appropriate context for queue creation failures and follow the established pattern of descriptive error messages.Also applies to: 103-104, 120-121, 157-158
tests/server.rs (1)
45-50: LGTM! Clear and descriptive error messages.The
.expect()calls provide excellent context for potential failures in socket operations and server execution. The messages are specific and will aid in debugging test failures.Also applies to: 52-52, 62-62
tests/wireframe_protocol.rs (2)
54-56: Good error messages for app creation.The
.expect("failed to create app")calls provide clear context forWireframeApp::new()failures and improve debugging capabilities.Also applies to: 78-80
84-87: Clear async operation error handling.The
.expect()calls for push operations and actor execution provide specific, actionable error messages that will aid in test failure diagnosis.Also applies to: 97-97
src/server.rs (4)
534-538: Excellent test fixture error handling.The
.expect()calls in test fixtures provide clear, specific error messages for socket operations that will greatly improve debugging of test failures.Also applies to: 614-616, 648-651
770-770: Clear server execution error messages.The
.expect()calls for server run operations provide appropriate context for timeout and execution failures in tests.Also applies to: 797-797, 832-832
873-877: Good listener and address operation error handling.The descriptive
.expect()messages for TCP listener operations and address retrieval provide excellent debugging context for test failures.Also applies to: 918-922, 926-926, 929-929
960-960: Comprehensive connection test error handling.The
.expect()calls throughout the connection panic test provide thorough error context for setup callbacks, socket operations, and server execution, improving test maintainability.Also applies to: 964-972, 988-994, 1001-1001
tests/connection_actor.rs (12)
61-61: Run-path.expect()message is clearThe added context
"actor run failed"is explicit enough for debugging.
93-93: No issues spottedMessage reads well and follows the same pattern as Line 61.
235-240: Message clarity OK
"value 42 should be present"succinctly explains the invariant; no action required.
260-260: Run-path.expect()message is sufficientNo further feedback.
282-282: Run-path.expect()message remains consistentLooks good.
322-322: Run-path.expect()message remains consistentNo issues.
340-340: Run-path.expect()message remains consistentNo issues.
397-397: Run-path.expect()message remains consistentNo issues.
454-455: Run-path.expect()message remains consistentNo issues.
485-486: Run-path.expect()message remains consistentNo issues.
534-535: Run-path.expect()message remains consistentNo issues.
555-556: Run-path.expect()message remains consistentNo issues.
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.
Replace verbose await/expect pairs with the push_expect! helper to standardise low- and high-priority push failure messages. Rewriting let-chain expressions to nested conditionals keeps the build on stable Rust, and lint expectations were relaxed accordingly.
leynos
force-pushed
the
codex/replace-.unwrap-with-.expect
branch
from
ac27ef4 to
23a2bbe
Compare
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.
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.
|
@coderabbitai new GitHub issue please: ConnectionActor::process_high and process_low contain almost identical logic, indicating duplicate code that could be extracted into a helper. |
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (2)
tests/connection_actor.rs (1)
55-56: LGTM: Consistent use of push_expect! macro with descriptive messagesThe replacement of direct
.await.unwrap()calls withpush_expect!macros providing contextual error messages ("push high-priority", "push low-priority") follows the established pattern and addresses the previous review feedback about standardising failure messages.Also applies to: 80-80, 82-82, 114-117, 122-125
src/push.rs (1)
316-321: LGTM: Excellent error handling with clear documentationThe explicit handling of
bounded_with_rate_dlqresult with explanatory comment, debug assertion, and descriptive expect message addresses the previous review feedback perfectly. This documents why failure is impossible whilst providing guard against future regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (22)
docs/wireframe-testing-crate.md(1 hunks)src/connection.rs(4 hunks)src/frame/tests.rs(2 hunks)src/push.rs(4 hunks)src/server.rs(11 hunks)tests/app_data.rs(2 hunks)tests/async_stream.rs(1 hunks)tests/connection_actor.rs(16 hunks)tests/extractor.rs(3 hunks)tests/lifecycle.rs(4 hunks)tests/metadata.rs(1 hunks)tests/middleware.rs(1 hunks)tests/middleware_order.rs(1 hunks)tests/preamble.rs(6 hunks)tests/push.rs(8 hunks)tests/push_policies.rs(4 hunks)tests/response.rs(5 hunks)tests/routes.rs(4 hunks)tests/server.rs(2 hunks)tests/session_registry.rs(1 hunks)tests/wireframe_protocol.rs(3 hunks)wireframe_testing/src/helpers.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/async_stream.rstests/middleware.rstests/lifecycle.rstests/session_registry.rstests/metadata.rstests/app_data.rstests/preamble.rstests/middleware_order.rstests/response.rssrc/frame/tests.rstests/extractor.rstests/push.rstests/wireframe_protocol.rstests/push_policies.rssrc/connection.rssrc/push.rstests/connection_actor.rswireframe_testing/src/helpers.rstests/routes.rssrc/server.rstests/server.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/async_stream.rstests/middleware.rstests/lifecycle.rstests/session_registry.rstests/metadata.rstests/app_data.rstests/preamble.rstests/middleware_order.rstests/response.rssrc/frame/tests.rstests/extractor.rstests/push.rstests/wireframe_protocol.rstests/push_policies.rssrc/connection.rssrc/push.rstests/connection_actor.rswireframe_testing/src/helpers.rstests/routes.rssrc/server.rstests/server.rs
docs/**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state.Conventions for writing project documentation as described in the documentation style guide
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-oxendict) for documentation text.
The word "outwith" is acceptable in documentation.
Keep US spelling when used in an API, for examplecolor.
Use the Oxford comma in documentation text.
Treat company names as collective nouns in documentation (e.g., "Lille Industries are expanding").
Write headings in sentence case in documentation.
Use Markdown headings (#,##,###, etc.) in order without skipping levels.
Follow markdownlint recommendations for Markdown files.
Provide code blocks and lists using standard Markdown syntax.
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text.
Use-as the first level bullet and renumber lists when items change.
Prefer inline links using[text](url)or angle brackets around the URL; avoid reference-style links like[foo][bar].
Ensure blank lines before and after bulleted lists and fenced blocks in Markdown.
Ensure tables have a delimiter line below the header row in Markdown.
Expand any uncommon acronym on first use, for example, Continuous Integration (CI).
Wrap paragraphs at 80 columns in documentation.
Wrap code at 120 columns in documentation.
Do not wrap tables in documentation.
Use sequentially numbered footnotes referenced with[^1]and place definitions at the end of the file.
Where it adds clarity, include Mermaid diagrams in documentation.
When embedding figures, useand provide concise ...
Files:
docs/wireframe-testing-crate.md
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.md: Documentation must use en-GB-oxendict spelling and grammar, except for the naming of the "LICENSE" file.
Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/wireframe-testing-crate.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/wireframe-testing-crate.md
docs/**/*.{md,rs}
📄 CodeRabbit Inference Engine (docs/multi-packet-and-streaming-responses-design.md)
docs/**/*.{md,rs}: The official documentation and examples must exclusively use the declarativeResponsemodel for handler responses.
The async-stream pattern must be documented as the canonical approach for dynamic stream generation.
Files:
docs/wireframe-testing-crate.md
🧬 Code Graph Analysis (9)
tests/lifecycle.rs (3)
tests/routes.rs (3)
BincodeSerializer(72-73)BincodeSerializer(118-119)BincodeSerializer(126-127)src/app.rs (3)
new(201-201)new(257-257)default(234-247)wireframe_testing/src/helpers.rs (1)
run_app_with_frame(342-352)
tests/session_registry.rs (1)
tests/connection_actor.rs (1)
queues(25-27)
tests/metadata.rs (2)
src/app.rs (5)
default(234-247)serializer(453-469)serializer(513-513)new(201-201)new(257-257)src/server.rs (1)
new(96-112)
tests/app_data.rs (2)
src/extractor.rs (4)
from_message_request(184-187)from_message_request(265-271)from_message_request(328-335)from_message_request(372-379)tests/extractor.rs (2)
request(18-18)empty_payload(25-25)
tests/response.rs (3)
src/app.rs (4)
default(234-247)new(201-201)new(257-257)from(143-143)wireframe_testing/src/helpers.rs (1)
processor(22-22)src/message.rs (1)
from_bytes(50-55)
tests/extractor.rs (2)
src/extractor.rs (4)
from_message_request(184-187)from_message_request(265-271)from_message_request(328-335)from_message_request(372-379)tests/app_data.rs (2)
request(19-19)empty_payload(26-26)
tests/push.rs (2)
tests/connection_actor.rs (1)
queues(25-27)src/push.rs (5)
bounded(292-295)push_low_priority(169-171)push_high_priority(143-145)recv(442-448)bounded_with_rate(348-354)
src/connection.rs (1)
tests/connection_actor.rs (1)
shutdown_token(34-36)
tests/routes.rs (2)
src/message.rs (1)
from_bytes(50-55)src/app.rs (6)
new_with_envelope(270-270)default(234-247)new(201-201)new(257-257)id(179-179)id(209-209)
🪛 GitHub Check: build-test
src/connection.rs
[warning] 196-196:
Diff in /home/runner/work/wireframe/wireframe/src/connection.rs
[warning] 459-459:
Diff in /home/runner/work/wireframe/wireframe/src/connection.rs
tests/connection_actor.rs
[warning] 22-22:
Diff in /home/runner/work/wireframe/wireframe/tests/connection_actor.rs
🪛 GitHub Actions: CI
src/connection.rs
[warning] 40-40: Prettier-like formatting warning: function active_connection_count should be formatted in a single line.
[warning] 187-187: Prettier-like formatting warning: function set_response should be formatted in a single line.
[warning] 196-196: Prettier-like formatting warning: function shutdown_token should be formatted in a single line.
[warning] 450-450: Prettier-like formatting warning: async function wait_shutdown should be formatted in a single line.
[warning] 459-459: Prettier-like formatting warning: async function recv_push should be formatted in a single line.
[warning] 541-541: Prettier-like formatting warning: function is_active should be formatted in a single line.
[warning] 541-541: Prettier-like formatting warning: function is_shutting_down should be formatted in a single line.
[warning] 541-541: Prettier-like formatting warning: function is_done should be formatted in a single line.
src/push.rs
[warning] 97-97: Prettier-like formatting warning: function from_arc should be formatted in a single line.
[warning] 255-255: Prettier-like formatting warning: function downgrade should be formatted in a single line.
tests/connection_actor.rs
[warning] 22-22: Prettier-like formatting warning: function queues should be formatted in a single line.
[warning] 31-31: Prettier-like formatting warning: function shutdown_token should be formatted in a single line.
[warning] 40-40: Prettier-like formatting warning: function empty_stream should be formatted in a single line.
src/server.rs
[warning] 229-229: Prettier-like formatting warning: function worker_count should be formatted in a single line.
🔇 Additional comments (63)
wireframe_testing/src/helpers.rs (2)
457-457: LGTM: Clear error context added.The replacement of
.unwrap()with.expect("server task panicked")provides explicit context for task failures, aligning perfectly with the PR objectives.
490-520: Well-designed test helper macros.The
push_expect!andrecv_expect!macros provide excellent standardisation for asynchronous test error handling. The inclusion of file and line information will significantly improve debugging efficiency.Key strengths:
- Clear documentation with debug build context
- Two variants allowing custom messages
- Proper macro export for test usage
- Consistent error message formatting
tests/async_stream.rs (1)
32-32: LGTM: Improved error context for actor failures.The replacement provides clear context when the actor run fails, enhancing test failure diagnostics.
tests/middleware.rs (1)
58-58: LGTM: Clear middleware failure context.The descriptive error message will help identify middleware call failures more effectively during testing.
tests/metadata.rs (1)
22-22: LGTM: Enhanced error context for app setup.Both changes provide clear, specific error messages for common test setup failures:
- App creation failure context at line 22
- Route registration failure context at line 26
These improvements will significantly aid debugging when test setup fails.
Also applies to: 26-26
tests/session_registry.rs (1)
34-35: LGTM: Clear async operation error context.Both changes provide descriptive error messages for push and receive operations, improving test failure diagnostics. The explicit
.expect()calls are appropriate and consistent with the PR objectives.tests/lifecycle.rs (6)
52-58: Excellent error message improvements.The replacement of
.unwrap()with descriptive.expect()messages enhances test failure diagnostics whilst maintaining the same control flow.
77-80: LGTM! Consistent error handling improvement.The expect messages align well with the previous function and provide clear failure context.
92-95: Consistent error handling enhancement.The expect messages maintain consistency across the test suite and provide clear diagnostic information.
124-124: Good error message for route registration.The descriptive expect message clearly identifies the failing operation.
130-132: Proper British spelling in error message.The expect message correctly uses "serialise" following the en-GB spelling requirements in the coding guidelines.
134-141: Clear error messages for encoding and execution.The expect messages provide good diagnostic context for frame encoding and application execution failures.
tests/response.rs (8)
42-43: Consistent app creation error handling.The expect message aligns with the pattern used throughout the test suite.
48-50: Clear error message for response sending.The expect message provides good context for send_response failures.
54-58: Comprehensive error handling for decoding pipeline.The expect messages clearly identify each step in the decode-decode-deserialize chain, improving failure diagnostics.
70-70: Consistent decode error handling.The expect message maintains consistency with other decoding operations.
82-82: Consistent decode error handling.The expect message maintains consistency across decode operations.
123-123: Clear encoding error message.The expect message provides good context for encoding failures.
125-128: Consistent decode error handling.The expect messages provide clear context for both decode operations and frame presence checks.
193-193: Correct app creation error message.The expect message properly identifies the app creation operation.
src/frame/tests.rs (2)
27-30: Clear conversion error message.The expect message provides good context for bytes-to-u64 conversion failures in test assertions.
48-48: Descriptive encoding error message.The expect message clearly identifies the u64 encoding operation that failed.
docs/wireframe-testing-crate.md (1)
123-133: Excellent documentation for helper macros.The new section clearly explains the purpose and usage of the
push_expect!andrecv_expect!macros, with practical examples that demonstrate their value in reducing test boilerplate.tests/routes.rs (11)
43-44: Consistent app creation error handling.The expect message maintains consistency with the pattern used throughout the test suite.
55-56: Clear route registration error message.The expect message properly identifies the route registration operation.
57-57: Appropriate encoding error message.The expect message provides clear context for message encoding failures.
70-71: Clear frame decoding error messages.The expect messages provide good diagnostic context for both decode operations and frame presence checks.
72-74: Consistent deserialization error handling.The expect message clearly identifies the deserialization operation.
75-75: Contextually appropriate error message.The expect message specifically mentions "echo" which provides good context for this test case.
82-83: Consistent app creation error handling.The expect message maintains consistency across app creation operations.
88-89: Consistent route registration error handling.The expect message maintains consistency with other route registration operations.
93-93: Consistent encoding error handling.The expect message maintains consistency with other encoding operations.
104-104: Consistent encoding error handling.The expect message maintains consistency with other encoding operations.
116-129: Comprehensive and consistent error handling.All expect messages maintain consistency with the established patterns and provide clear diagnostic context for the decode-deserialize pipeline operations.
tests/app_data.rs (2)
34-35: LGTM - Clear error message for state extraction.The
.expect()call provides a descriptive error message that will aid debugging if the extraction fails.
46-46: LGTM - Appropriate error message for test expectation.The error message clearly indicates this is an expected error condition in the test context.
tests/middleware_order.rs (3)
56-63: LGTM - Clear error messages for app setup.The
.expect()calls provide descriptive error messages for app creation, route registration, and middleware wrapping operations.
69-75: LGTM - Descriptive error messages for I/O operations.The error messages clearly identify which serialization, encoding, or I/O operation failed, improving test debugging.
84-90: LGTM - Clear error context for frame processing.The chained
.expect()calls provide specific error messages for each step of frame decoding and deserialization.tests/extractor.rs (3)
38-38: LGTM - Proper error message with British spelling.The
.expect()call uses "serialise" (British spelling) which aligns with the coding guidelines requiring en-GB spelling.
41-42: LGTM - Type-specific error message.The error message clearly identifies the specific type (TestMsg) being extracted, improving debugging context.
55-56: LGTM - Consistent error messaging pattern.Both ConnectionInfo and SharedState extraction use descriptive error messages that clearly identify the failed operation.
Also applies to: 70-71
tests/push_policies.rs (3)
43-49: LGTM - Clear push operation error messages.The error messages distinguish between different push operations ("push high priority failed" vs "try_push failed"), providing specific debugging context.
79-80: LGTM - Consistent queue creation error handling.The "queue creation failed" message is used consistently across different test scenarios, improving debugging clarity.
Also applies to: 138-139
92-92: LGTM - DLQ-specific error messages.The error messages are specific to Dead Letter Queue operations ("dlq recv failed"), making test failures easier to diagnose.
Also applies to: 108-108
tests/preamble.rs (3)
71-73: LGTM - Clear server operation error messages.The error messages provide specific context for server setup and lifecycle operations ("bind", "server run failed", "server join failed").
Also applies to: 82-82, 87-87
94-95: LGTM - Consistent I/O operation error messages.The I/O error messages follow a consistent pattern ("write failed", "read failed", "shutdown failed") that will aid in debugging network operations.
Also applies to: 110-111, 164-165, 205-206, 216-216, 218-218
145-145: LGTM - Appropriate concurrency error message.The "lock poisoned" message clearly indicates a concurrency issue, which is the standard terminology for this type of failure in Rust.
Also applies to: 155-155
tests/push.rs (3)
8-8: LGTM: Proper import of new testing macrosThe import of
push_expect!andrecv_expect!macros aligns with the PR's objective to standardise error handling in tests.
15-16: LGTM: Consistent use of testing macrosThe replacement of
.await.unwrap()withpush_expect!andrecv_expect!macros provides better error diagnostics whilst maintaining the same test logic.Also applies to: 18-19
69-70: LGTM: Descriptive error messages for queue creationThe
.expect("queue creation failed")calls provide clear context whenbounded_with_ratefails, which is appropriate for test scenarios.Also applies to: 103-104, 120-121, 157-158
tests/wireframe_protocol.rs (2)
54-56: LGTM: Clear error messages for app creationThe
.expect("failed to create app")provides clear context whenWireframeApp::new()fails.Also applies to: 78-80
84-87: LGTM: Descriptive error handling for async operationsThe explicit
.expect()calls with descriptive messages ("push failed", "actor run failed") improve error diagnostics over generic unwrap panics.Also applies to: 97-97
tests/server.rs (1)
45-50: LGTM: Context-specific error messages for server operationsThe replacement of
.unwrap()with descriptive.expect()calls provides clear failure context for socket operations, binding, and server execution.Also applies to: 52-52, 62-62
tests/connection_actor.rs (1)
62-62: LGTM: Descriptive error messages for actor operationsThe
.expect("actor run failed")calls provide clear context whenConnectionActor::run()fails, improving error diagnostics over generic unwrap panics.Also applies to: 88-88, 182-182, 247-247, 266-266, 306-306, 324-324, 381-381, 432-432, 463-463, 512-512, 533-533
src/push.rs (1)
394-398: LGTM: Rate validation logic restructuringThe restructuring from combined
if letwith condition to nested structure maintains the same validation semantics whilst potentially improving readability.src/server.rs (7)
577-581: LGTM!Excellent replacement of
.unwrap()with meaningful.expect()messages that clearly indicate the failure context for test setup.
657-659: LGTM!Good use of
.expect()with a descriptive message that clearly indicates what should be available after binding.
691-693: LGTM!Clear and descriptive
.expect()message that properly indicates the expected state after binding.
563-566: Clarify the attribute change rationale.The change from
#[expect(dead_code)]to#[allow(dead_code)]appears unrelated to the main PR objective of replacing.unwrap()with.expect(). While both are valid lint suppressions, this change should be explained.Based on the coding guidelines preference for
#[expect]over#[allow], was this change intentional? The guidelines state "Preferexpectoverallow."
916-920: LGTM!Excellent replacement of
.unwrap()calls with meaningful.expect()messages throughout the test code. These provide clear context for debugging test failures.Also applies to: 961-965, 969-969, 972-972
512-516: LGTM!The restructuring from a combined
if letwith logical AND to nestedif letstatements improves readability without changing the logic. This aligns with the coding guidelines for clarity.
1052-1052: LGTM!Comprehensive replacement of
.unwrap()calls with descriptive.expect()messages throughout the test. These provide excellent debugging context for test failures.Also applies to: 1056-1060, 1062-1064, 1073-1073, 1080-1086, 1093-1093
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll examine the ConnectionActor code first to understand the duplication, then create the GitHub issue for you. Now I'll create the GitHub issue: @leynos I've created a GitHub issue to track the duplicate code in 🏁 Script executed: Length of output: 147 🏁 Script executed: Length of output: 64058 🏁 Script executed: Length of output: 306 🏁 Script executed: Length of output: 1671 🏁 Script executed: Length of output: 731 @leynos I've successfully created GitHub issue #251 to track the duplicate code in Looking at the code, I can see the duplication you identified. Both methods follow an almost identical pattern:
The main differences are:
This duplication could indeed be extracted into a helper method that takes the priority-specific callbacks as parameters. The issue has been created with the appropriate title, description, and backlinks to this PR and comment for future reference. 🐰✨ ears perk up with organizational satisfaction |
* Handle missing TryRecvError and fix test message * Apply formatting
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.
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.
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.
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.
|
@coderabbitai Are |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6968 🏁 Script executed: Length of output: 1309 🏁 Script executed: Length of output: 2325 @leynos Based on my analysis of the codebase, both
Recommendation:
This will eliminate the dead code warnings and clean up the codebase. If these were meant to serve as examples or placeholders, they're not fulfilling that purpose currently. 🐰🧹 tidying up with a satisfied hop |
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.
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.
1 participant
Summary
.unwrap()calls with.expect()across tests and code.expect()Testing
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_688bc793d3588322b35989f097ecab1c
Summary by Sourcery
Replace .unwrap() calls with .expect() throughout the codebase and tests, adding descriptive error messages for improved debugging
Enhancements:
Tests: