Conversation
… resource group's mod.rs
|
Warning Rate limit exceeded
⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughConsolidated duplicated test fixture factories by adding shared Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
🚥 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✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Refactors src/cmd/* test modules to deduplicate repeated sample_* fixture factories by promoting them to shared pub(crate) helpers within each resource group (typically in the group mod.rs, and for wiki in list::tests_helper), then updating tests to import the shared fixtures.
Changes:
- Added shared
#[cfg(test)] pub(crate) fn sample_*()fixtures in resource-group modules (e.g.,team,user,watch,space,project,issue/attachment,wiki/attachment). - Removed duplicated per-file
sample_*fixtures from sibling subcommand test modules. - Updated test imports/usages to reference the shared fixtures.
Reviewed changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/cmd/wiki/update.rs | Switch tests to use shared sample_wiki fixture. |
| src/cmd/wiki/show.rs | Switch tests to use shared sample_wiki fixture. |
| src/cmd/wiki/list.rs | Extend tests_helper with a shared sample_wiki() for other wiki tests. |
| src/cmd/wiki/delete.rs | Switch tests to use shared sample_wiki fixture. |
| src/cmd/wiki/create.rs | Switch tests to use shared sample_wiki fixture. |
| src/cmd/wiki/attachment/mod.rs | Introduce shared sample_attachment() fixture for wiki-attachment tests. |
| src/cmd/wiki/attachment/list.rs | Use shared sample_attachment() fixture; remove local duplicate. |
| src/cmd/wiki/attachment/delete.rs | Use shared sample_attachment() fixture; remove local duplicate. |
| src/cmd/wiki/attachment/add.rs | Use shared sample_attachment() fixture; remove local duplicate. |
| src/cmd/watch/update.rs | Use shared sample_watching() fixture; remove local duplicate. |
| src/cmd/watch/show.rs | Use shared sample_watching() fixture; remove local duplicate. |
| src/cmd/watch/mod.rs | Introduce shared sample_watching() fixture for watch tests. |
| src/cmd/watch/list.rs | Use shared sample_watching() fixture; remove local duplicate helpers. |
| src/cmd/watch/delete.rs | Use shared sample_watching() fixture; remove local duplicate. |
| src/cmd/watch/add.rs | Use shared sample_watching() fixture; remove local duplicate. |
| src/cmd/user/update.rs | Use shared sample_user() fixture; remove local duplicate. |
| src/cmd/user/show.rs | Use shared sample_user() fixture; remove local duplicate and adjust assertions. |
| src/cmd/user/mod.rs | Introduce shared sample_user() fixture for user tests. |
| src/cmd/user/list.rs | Use shared sample_user() fixture; remove local duplicate. |
| src/cmd/user/delete.rs | Use shared sample_user() fixture; remove local duplicate. |
| src/cmd/user/add.rs | Use shared sample_user() fixture; remove local duplicate. |
| src/cmd/team/update.rs | Use shared sample_team() fixture; remove local duplicate fixtures. |
| src/cmd/team/show.rs | Use shared sample_team() fixture; remove local duplicate fixtures. |
| src/cmd/team/mod.rs | Introduce shared sample_member() / sample_team() fixtures for team tests. |
| src/cmd/team/list.rs | Use shared sample_team() fixture; remove local duplicate fixtures. |
| src/cmd/team/delete.rs | Use shared sample_team() fixture; remove local duplicate fixtures. |
| src/cmd/team/add.rs | Use shared sample_team() fixture; remove local duplicate fixtures. |
| src/cmd/space/update_notification.rs | Use shared sample_notification() fixture; remove local duplicate and update assertions. |
| src/cmd/space/notification.rs | Use shared sample_notification() fixture; remove local duplicate. |
| src/cmd/space/mod.rs | Introduce shared sample_notification() fixture for space tests. |
| src/cmd/project/user.rs | Use shared sample_project_user() fixture (renamed from local sample_user). |
| src/cmd/project/update.rs | Use shared sample_project() fixture; remove local duplicate. |
| src/cmd/project/show.rs | Use shared sample_project() fixture; remove local duplicate. |
| src/cmd/project/mod.rs | Introduce shared sample_project() / sample_project_user() fixtures for project tests. |
| src/cmd/project/list.rs | Use shared sample_project() fixture; remove local duplicate. |
| src/cmd/project/delete.rs | Use shared sample_project() fixture; remove local duplicate. |
| src/cmd/project/create.rs | Use shared sample_project() fixture; remove local duplicate. |
| src/cmd/project/admin.rs | Use shared sample_project_user() fixture (renamed from local sample_user). |
| src/cmd/issue/attachment/mod.rs | Introduce shared sample_attachment() fixture for issue-attachment tests. |
| src/cmd/issue/attachment/list.rs | Use shared sample_attachment() fixture; remove local duplicate user/attachment fixtures. |
| src/cmd/issue/attachment/delete.rs | Use shared sample_attachment() fixture; remove local duplicate. |
![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
🧹 Nitpick comments (3)
src/cmd/wiki/list.rs (1)
82-99: Minor: Remove redundant import.The
use std::collections::BTreeMap;on line 83 is unnecessary sinceBTreeMapis already imported at line 66 in the enclosingtests_helpermodule scope.🧹 Suggested cleanup
pub fn sample_wiki() -> Wiki { - use std::collections::BTreeMap; Wiki { id: 1,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/wiki/list.rs` around lines 82 - 99, The function sample_wiki in src/cmd/wiki/list.rs contains a redundant local import `use std::collections::BTreeMap;`; remove that line so the function relies on the BTreeMap imported at the tests_helper module scope, leaving the rest of the sample_wiki implementation unchanged.src/cmd/wiki/delete.rs (1)
45-87: Consider: wiki fixtures location differs from other groups.The wiki module places shared fixtures in
list::tests_helper(requiring imports likecrate::cmd::wiki::list::tests_helper::sample_wiki), whereas other resource groups (space, watch, team, project, issue/attachment) place them directly inmod.rs(e.g.,crate::cmd::team::sample_team).This is fine for now but could be unified in a follow-up for consistency across all resource groups.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/wiki/delete.rs` around lines 45 - 87, Tests in delete.rs import sample_wiki from crate::cmd::wiki::list::tests_helper, which differs from other resource groups that expose fixtures at crate::cmd::...::sample_*; to make this consistent either re-export sample_wiki from the wiki module root or move the fixture into the wiki module's mod.rs so tests can import crate::cmd::wiki::sample_wiki; update delete.rs to import sample_wiki from crate::cmd::wiki::sample_wiki (and/or add a pub use in list::tests_helper) and ensure the sample_wiki symbol remains available to the tests.src/cmd/user/show.rs (1)
97-97: Test assertion is now ambiguous.The assertion
text.contains("1")passes for bothid(1) androle_type(1) in the shared fixture. The previous assertion likely checked for a more unique value. Consider a more specific assertion to verify the ID field distinctly.💡 Suggested improvement
- assert!(text.contains("1")); + assert!(text.contains("ID: 1"));Alternatively, you could check for the bold formatting applied to the ID, or use a regex to match the specific line.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/user/show.rs` at line 97, The test assertion text.contains("1") is ambiguous (matches both id and role_type); update the assertion in the test in show.rs to target the ID field explicitly — e.g. assert that the output contains the exact ID label and value (like "ID: 1" or the bold-formatted ID string), or use a regex that matches the ID line only (for example a multiline regex matching "^ID:\\s*1\\b"). Locate the failing assertion (text.contains("1")) and replace it with a more specific containment or regex check so it only verifies the record ID, not other fields such as role_type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cmd/wiki/show.rs`:
- Around line 52-53: The test in show.rs currently imports sample_wiki from the
sibling list::tests_helper which couples tests to list internals; change the
import to the wiki group-level helper exported by the wiki mod (use the
module-level tests_helper export) so show.rs imports sample_wiki from
crate::cmd::wiki::tests_helper (referenced symbol: sample_wiki) instead of
crate::cmd::wiki::list::tests_helper; update the use line in show.rs accordingly
to reference the module-level helper exported by mod.rs.
---
Nitpick comments:
In `@src/cmd/user/show.rs`:
- Line 97: The test assertion text.contains("1") is ambiguous (matches both id
and role_type); update the assertion in the test in show.rs to target the ID
field explicitly — e.g. assert that the output contains the exact ID label and
value (like "ID: 1" or the bold-formatted ID string), or use a regex that
matches the ID line only (for example a multiline regex matching
"^ID:\\s*1\\b"). Locate the failing assertion (text.contains("1")) and replace
it with a more specific containment or regex check so it only verifies the
record ID, not other fields such as role_type.
In `@src/cmd/wiki/delete.rs`:
- Around line 45-87: Tests in delete.rs import sample_wiki from
crate::cmd::wiki::list::tests_helper, which differs from other resource groups
that expose fixtures at crate::cmd::...::sample_*; to make this consistent
either re-export sample_wiki from the wiki module root or move the fixture into
the wiki module's mod.rs so tests can import crate::cmd::wiki::sample_wiki;
update delete.rs to import sample_wiki from crate::cmd::wiki::sample_wiki
(and/or add a pub use in list::tests_helper) and ensure the sample_wiki symbol
remains available to the tests.
In `@src/cmd/wiki/list.rs`:
- Around line 82-99: The function sample_wiki in src/cmd/wiki/list.rs contains a
redundant local import `use std::collections::BTreeMap;`; remove that line so
the function relies on the BTreeMap imported at the tests_helper module scope,
leaving the rest of the sample_wiki implementation unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7facf8e-1220-46b2-949d-906b6e3ed556
📒 Files selected for processing (41)
src/cmd/issue/attachment/delete.rssrc/cmd/issue/attachment/list.rssrc/cmd/issue/attachment/mod.rssrc/cmd/project/admin.rssrc/cmd/project/create.rssrc/cmd/project/delete.rssrc/cmd/project/list.rssrc/cmd/project/mod.rssrc/cmd/project/show.rssrc/cmd/project/update.rssrc/cmd/project/user.rssrc/cmd/space/mod.rssrc/cmd/space/notification.rssrc/cmd/space/update_notification.rssrc/cmd/team/add.rssrc/cmd/team/delete.rssrc/cmd/team/list.rssrc/cmd/team/mod.rssrc/cmd/team/show.rssrc/cmd/team/update.rssrc/cmd/user/add.rssrc/cmd/user/delete.rssrc/cmd/user/list.rssrc/cmd/user/mod.rssrc/cmd/user/show.rssrc/cmd/user/update.rssrc/cmd/watch/add.rssrc/cmd/watch/delete.rssrc/cmd/watch/list.rssrc/cmd/watch/mod.rssrc/cmd/watch/show.rssrc/cmd/watch/update.rssrc/cmd/wiki/attachment/add.rssrc/cmd/wiki/attachment/delete.rssrc/cmd/wiki/attachment/list.rssrc/cmd/wiki/attachment/mod.rssrc/cmd/wiki/create.rssrc/cmd/wiki/delete.rssrc/cmd/wiki/list.rssrc/cmd/wiki/show.rssrc/cmd/wiki/update.rs
…ser ID field
Addresses review comment: assert!(text.contains("1")) is too weak
…ralization Addresses review comment: import sample_wiki from list::tests_helper couples tests to list internals
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
mainwebsite/docs/,website/i18n/ja/,README.md)Summary
#[cfg(test)] pub(crate) fn sample_*()functions to each resource group'smod.rssample_*functions from sibling subcommand filesuse crate::cmd::<group>::sample_*Reason for change
Each test module under
src/cmd/defined its ownsample_*factory functions that were identical across siblings. These duplicates made changes (e.g. adding a field to a struct) require updates in many places.Changes
Groups affected:
team:sample_member,sample_team(5 files deduped)wiki:sample_wiki(4 files),wiki/attachment:sample_attachment(3 files)user:sample_user(5 files)watch:sample_watching(5 files)space:sample_notification(2 files)project:sample_project(5 files),sample_project_user(2 files, renamed fromsample_user)issue/attachment:sample_attachment(2 files)Closes #107