Replace #[allow] with #[expect] in tests and annotate#438
Conversation
Removed multiple clippy and rustc lint suppression attributes from rstest single-line fixtures in test modules. Replaced them with concise comments explaining the fixture purpose to improve code clarity and maintainability. Also applied similar style simplification in wireframe_testing source files related to fixtures and async functions. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Reviewer's GuideReplaces test lint suppressions using #[allow(...)] with explicit #[expect(...)] where needed, removes a macro used to wrap rstest fixtures, and simplifies fixtures into straightforward functions with comments explaining their intent, while keeping behavior unchanged. File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughSummarise the conversion of several single-line rstest fixtures into explicit functions (some kept as fixtures, some converted to plain functions), removal or replacement of broad Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
FIXME(#333)abovecollect_multi_packetnow points at the issue this PR closes; consider updating this to a new follow-up issue or a more generic TODO so it doesn’t become a stale reference once merged. - Now that the
single_line_fixturemacro is removed, thequeuesandshutdown_tokenfixtures are duplicated across multiple test modules with very similar comments; consider extracting shared fixtures into a common test helper to reduce repetition and keep behavior aligned.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `FIXME(#333)` above `collect_multi_packet` now points at the issue this PR closes; consider updating this to a new follow-up issue or a more generic TODO so it doesn’t become a stale reference once merged.
- Now that the `single_line_fixture` macro is removed, the `queues` and `shutdown_token` fixtures are duplicated across multiple test modules with very similar comments; consider extracting shared fixtures into a common test helper to reduce repetition and keep behavior aligned.
## Individual Comments
### Comment 1
<location> `wireframe_testing/src/multi_packet.rs:31` </location>
<code_context>
#[must_use]
#[track_caller]
-#[allow(ungated_async_fn_track_caller)] // track_caller on async is unstable
+// FIXME(#333): Remove when `track_caller` on async fns is stabilised.
+#[expect(
+ ungated_async_fn_track_caller,
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The FIXME marker references `#333` but does not include a concrete link as required by the lint-suppression guidelines.
The instructions specify that when using `#[expect(...)]` and a fix is anticipated, the `FIXME:` comment should include an actual link (e.g. to an issue or tracking document). Here it only contains `#333`, which may not be a clickable or globally meaningful link. Please update this to include a full link (for example, the repository issue URL) so it complies with the guideline.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
Include `FIXME:` with link if a fix is expected.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/app_data.rs`:
- Around line 15-25: Update the inline comments on the two fixtures in
tests/app_data.rs to reference this module's purpose (shared state/app data
extraction) rather than "extractor tests": change the comment above the
request() fixture (function name request) from "Default request for extractor
tests" to something like "Default request for app data/shared state tests" and
similarly change the comment above empty_payload() (function name empty_payload)
from "Empty payload for extractor tests" to "Empty payload for app data/shared
state tests" so the comments match the module context.
- Updated comments in tests/app_data.rs to specify shared state extraction context - Added tracking URL and adjusted FIXME comment in wireframe_testing/src/multi_packet.rs for async track_caller issue Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wireframe_testing/src/multi_packet.rs`:
- Around line 31-36: Update the FIXME comment above the
ungated_async_fn_track_caller attribute in multi_packet.rs to use en-GB-oxendict
spelling by replacing "stabilised" with "stabilized" (i.e., change "Remove when
`track_caller` on async fns is stabilised." to use "stabilized"); ensure the
rest of the comment text and the tracking URL remain unchanged and the change is
limited to that word in the FIXME line referencing `track_caller`.
|
@coderabbitai please create a new GitHub issue for the following: The FIXME(#333) above collect_multi_packet now points at the issue this PR closes; create a new follow-up issue so it doesn’t become a stale reference once merged. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1749 🏁 Script executed: Length of output: 46
The issue includes:
|
…in comment Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Summary
Changes
Rationale
Testing
Notes
◳ Generated by DevBoxer ◰
ℹ️ Tag @devboxerhub to ask questions and address PR feedback
📎 Task: https://www.devboxer.com/task/10aca5b8-2872-408d-892c-cc49c31776ea
📝 Closes #333
Summary by Sourcery
Align lint handling in tests with project standards by replacing legacy lint suppressions and simplifying fixtures while documenting their intent.
Enhancements:
Tests: