[ISSUE #6211][Test🧪] Add unit tests for PopProcessQueueInfo#6317
Merged
[ISSUE #6211][Test🧪] Add unit tests for PopProcessQueueInfo#6317
Conversation
rocketmq-rust-bot
requested review from
TeslaRustor,
mxsm and
rocketmq-rust-bot
Collaborator
|
🔊@oopscompiled 🚀Thanks for your contribution🎉！ 💡CodeRabbit(AI) will review your code first🔥！ Note 🚨The code review suggestions from CodeRabbit are to be used as a reference only, and the PR submitter can decide whether to make changes based on their own judgment. Ultimately, the project management personnel will conduct the final code review💥. |
rocketmq-rust-bot
added
AI review first
Contributor
WalkthroughAdds a cfg(test) unit-test module for PopProcessQueueInfo in rocketmq-remoting that exercises initialization, getters/setters, Clone/Copy semantics, Display formatting, JSON (de)serialization, and edge-case integer values. No public API or runtime logic changes. (≤50 words) Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@rocketmq-remoting/src/protocol/body/pop_process_queue_info.rs`:
- Around line 90-97: The test pop_process_queue_clone unnecessarily calls
clone() on a Copy type (PopProcessQueueInfo) and Clippy flags clone_on_copy; to
preserve the explicit Clone test, suppress the lint locally by adding
#[allow(clippy::clone_on_copy)] to the test function (the function named
pop_process_queue_clone) so the test still calls queue.clone() while silencing
the Clippy warning.
🧹 Nitpick comments (1)
rocketmq-remoting/src/protocol/body/pop_process_queue_info.rs (1)
66-117: Consider adding edge-case and serialization tests per the linked issue checklist.The current tests cover the happy path well, but the issue checklist also calls for:
- Zero and boundary values (
i32::MIN,u64::MAX)- Serde JSON roundtrip (if
Serialize/Deserializeare derived or planned)Debugtrait outputThese would help reach the ≥ 80% coverage target mentioned in the issue.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive unit tests for the PopProcessQueueInfo struct to improve code coverage and ensure correctness of the struct's functionality. The tests verify initialization, getter/setter methods, trait implementations (Clone, Copy, Display), and overall behavior of the data structure.
Changes:
- Added 5 test functions covering initialization, setters, clone, display formatting, and copy behavior for
PopProcessQueueInfo
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+108
to
+116
|
|
||
| #[test] | ||
| fn pop_process_queue_copy() { | ||
| let queue: PopProcessQueueInfo = PopProcessQueueInfo::new(10, false, 123456789); | ||
| let copied = queue; | ||
| assert_eq!(copied.wait_ack_count(), 10); | ||
| assert!(!copied.droped()); | ||
| assert_eq!(copied.last_pop_timestamp(), 123456789); | ||
| } |
There was a problem hiding this comment.
The test pop_process_queue_copy is redundant. Since PopProcessQueueInfo derives Copy (line 15), it automatically implements the Copy trait which is implicitly used when assigning let copied = queue. This behavior is already tested by the pop_process_queue_clone test, which verifies that the struct can be copied correctly via the Clone trait. Additionally, Copy types automatically implement Clone, making this test functionally equivalent to the clone test. Consider removing this test to avoid redundancy.
Suggested change
| #[test] | |
| fn pop_process_queue_copy() { | |
| let queue: PopProcessQueueInfo = PopProcessQueueInfo::new(10, false, 123456789); | |
| let copied = queue; | |
| assert_eq!(copied.wait_ack_count(), 10); | |
| assert!(!copied.droped()); | |
| assert_eq!(copied.last_pop_timestamp(), 123456789); | |
| } |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@rocketmq-remoting/src/protocol/body/pop_process_queue_info.rs`:
- Around line 176-181: The CI failure is due to formatting in the test snippet
around PopProcessQueueInfo display assertion; run rustfmt (cargo fmt) to
reformat the file and re-push the changes so the code matches the project's
style guidelines, ensuring the block that defines queue_negative,
display_negative and the assert_eq! (which references PopProcessQueueInfo::new
and the formatted string) is properly formatted.
🧹 Nitpick comments (1)
rocketmq-remoting/src/protocol/body/pop_process_queue_info.rs (1)
66-68: Consider adding aDebugtrait test as outlined in the linked issue.The issue checklist calls for testing
Debug,Default(if derived), and JSON serde.Debugis derived on the struct but not tested here. A quickformat!("{:?}", queue)assertion would cover it.Defaultand serde don't apply since they aren't derived, so those can be skipped.
rocketmq-rust-bot
approved these changes
Feb 13, 2026
Collaborator
rocketmq-rust-bot
left a comment
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6317 +/- ##
==========================================
+ Coverage 40.55% 42.57% +2.02%
==========================================
Files 886 911 +25
Lines 122833 127906 +5073
==========================================
+ Hits 49810 54451 +4641
- Misses 73023 73455 +432 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
added
approved
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.
Which Issue(s) This PR Fixes(Closes)
Brief Description
How Did You Test This Change?
Summary by CodeRabbit