[ISSUE #6210][Test🧪] Add unit tests for OffsetMovedEvent#6319
Merged
[ISSUE #6210][Test🧪] Add unit tests for OffsetMovedEvent#6319
Conversation
rocketmq-rust-bot
requested review from
SpaceXCN,
TeslaRustor 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
WalkthroughAdded a unit test module for OffsetMovedEvent in the remoting protocol, introducing JSON (de)serialization, Display formatting, getter/setter, and round-trip tests; no production API or logic changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 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: 2
🤖 Fix all issues with AI agents
In `@rocketmq-remoting/src/protocol/topic.rs`:
- Line 90: Rename the test function offest_moved_event_setters to
offset_moved_event_setters; update the fn declaration and any references to that
identifier (uses, test annotations like #[test], or module-level refs) so the
test name is spelled correctly and remains discoverable by the test runner.
- Around line 133-144: The test offset_moved_event_serialise for
OffsetMovedEvent only asserts consumer_group/offset fields and misses verifying
message_queue; update the test (offset_moved_event_serialise) to also assert
that the serialized json contains the message_queue key (e.g.
assert!(json.contains("\"message_queue\":")) or compare against a full expected
JSON string matching the serialization of MessageQueue::new()), so MessageQueue
serialization is validated alongside consumer_group and offsets.
🧹 Nitpick comments (1)
rocketmq-remoting/src/protocol/topic.rs (1)
71-155: Missing required OffsetMovedEvent test cases from issue#6210.The current suite doesn’t cover several explicit requirements: message_queue setter/field behavior, serde round‑trip, Debug/Clone (if derived), and negative/zero/large offset values. Please add these to meet the stated objectives and reach the target coverage.
Comment on lines
133
to
144
| fn offset_moved_event_serialise() { | ||
| let body = OffsetMovedEvent { | ||
| consumer_group: "test_group".to_string(), | ||
| message_queue: MessageQueue::new(), | ||
| offset_request: 100, | ||
| offset_new: 200, | ||
| }; | ||
| let json = serde_json::to_string(&body).unwrap(); | ||
| assert!(json.contains("\"consumer_group\":\"test_group\"")); | ||
| assert!(json.contains("\"offset_request\":100")); | ||
| assert!(json.contains("\"offset_new\":200")); | ||
| } |
Contributor
There was a problem hiding this comment.
Serialization test doesn’t validate message_queue.
You only assert consumer_group/offset fields. Add an assertion that the serialized JSON includes message_queue (or compare against a full expected JSON) to ensure it’s included.
🤖 Prompt for AI Agents
In `@rocketmq-remoting/src/protocol/topic.rs` around lines 133 - 144, The test
offset_moved_event_serialise for OffsetMovedEvent only asserts
consumer_group/offset fields and misses verifying message_queue; update the test
(offset_moved_event_serialise) to also assert that the serialized json contains
the message_queue key (e.g. assert!(json.contains("\"message_queue\":")) or
compare against a full expected JSON string matching the serialization of
MessageQueue::new()), so MessageQueue serialization is validated alongside
consumer_group and offsets.
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request adds unit tests for the OffsetMovedEvent struct in the rocketmq-remoting module to address issue #6210. The PR adds basic test coverage for field initialization, getters, setters, Display trait implementation, and JSON serialization/deserialization.
Changes:
- Added 6 unit tests for
OffsetMovedEventcovering initialization, getters, setters, Display formatting, serialization, and deserialization - Tests verify basic functionality of the struct's methods and trait implementations
- Includes JSON serialization/deserialization validation
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| #[test] | ||
| fn offest_moved_event_setters() { |
There was a problem hiding this comment.
Typo in test function name: "offest_moved_event_setters" should be "offset_moved_event_setters". This misspelling makes the test name inconsistent with other test names in the file and harder to find.
Suggested change
| fn offest_moved_event_setters() { | |
| fn offset_moved_event_setters() { |
Comment on lines
71
to
155
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn offset_moved_event_init() { | ||
| let body: OffsetMovedEvent = OffsetMovedEvent { | ||
| consumer_group: "test_group".to_string(), | ||
| message_queue: MessageQueue::new(), | ||
| offset_request: 100, | ||
| offset_new: 200, | ||
| }; | ||
| assert_eq!(body.get_consumer_group(), "test_group"); | ||
| assert_eq!(body.get_offset_request(), 100); | ||
| assert_eq!(body.get_offset_new(), 200); | ||
| assert_eq!(body.get_message_queue(), &MessageQueue::new()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn offest_moved_event_setters() { | ||
| let mut body = OffsetMovedEvent { | ||
| consumer_group: "test_group".to_string(), | ||
| message_queue: MessageQueue::new(), | ||
| offset_request: 100, | ||
| offset_new: 200, | ||
| }; | ||
| body.set_consumer_group("new_group".to_string()); | ||
| body.set_offset_request(150); | ||
| body.set_offset_new(250); | ||
| assert_eq!(body.get_consumer_group(), "new_group"); | ||
| assert_eq!(body.get_offset_request(), 150); | ||
| assert_eq!(body.get_offset_new(), 250); | ||
| } | ||
|
|
||
| #[test] | ||
| fn offset_moved_event_getters() { | ||
| let body = OffsetMovedEvent { | ||
| consumer_group: "test_group".to_string(), | ||
| message_queue: MessageQueue::new(), | ||
| offset_request: 100, | ||
| offset_new: 200, | ||
| }; | ||
| assert_eq!(body.get_consumer_group(), "test_group"); | ||
| assert_eq!(body.get_offset_request(), 100); | ||
| assert_eq!(body.get_offset_new(), 200); | ||
| } | ||
|
|
||
| #[test] | ||
| fn offset_moved_event_display() { | ||
| let body = OffsetMovedEvent { | ||
| consumer_group: "test_group".to_string(), | ||
| message_queue: MessageQueue::new(), | ||
| offset_request: 100, | ||
| offset_new: 200, | ||
| }; | ||
| let display = format!("{}", body); | ||
| assert!(display.contains("OffsetMovedEvent")); | ||
| assert!(display.contains("consumer_group=test_group")); | ||
| assert!(display.contains("offset_request=100")); | ||
| assert!(display.contains("offset_new=200")); | ||
| } | ||
| #[test] | ||
| fn offset_moved_event_serialise() { | ||
| let body = OffsetMovedEvent { | ||
| consumer_group: "test_group".to_string(), | ||
| message_queue: MessageQueue::new(), | ||
| offset_request: 100, | ||
| offset_new: 200, | ||
| }; | ||
| let json = serde_json::to_string(&body).unwrap(); | ||
| assert!(json.contains("\"consumer_group\":\"test_group\"")); | ||
| assert!(json.contains("\"offset_request\":100")); | ||
| assert!(json.contains("\"offset_new\":200")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn offset_moved_event_deserialise() { | ||
| let json = r#"{"consumer_group":"test_group","message_queue":{"topic":"","brokerName":"","queueId":0},"offset_request":100,"offset_new":200}"#; | ||
| let body: OffsetMovedEvent = serde_json::from_str(json).unwrap(); | ||
| assert_eq!(body.get_consumer_group(), "test_group"); | ||
| assert_eq!(body.get_offset_request(), 100); | ||
| assert_eq!(body.get_offset_new(), 200); | ||
| assert_eq!(body.get_message_queue(), &MessageQueue::new()); | ||
| } | ||
| } |
There was a problem hiding this comment.
The test coverage is incomplete based on the requirements in issue #6210. The following test cases are missing:
- Test with negative offset values
- Test with zero offset values
- Test with large offset values
- Test serialization/deserialization roundtrip
- Test Clone trait implementation (note: Clone is not currently derived on OffsetMovedEvent, so it cannot be cloned)
Consider adding tests for edge cases with negative, zero, and large offset values. Additionally, add a roundtrip test that serializes and then deserializes to ensure data integrity. If Clone trait is needed, add it to the struct's derive macro first.
Comment on lines
76
to
155
| fn offset_moved_event_init() { | ||
| let body: OffsetMovedEvent = OffsetMovedEvent { | ||
| consumer_group: "test_group".to_string(), | ||
| message_queue: MessageQueue::new(), | ||
| offset_request: 100, | ||
| offset_new: 200, | ||
| }; | ||
| assert_eq!(body.get_consumer_group(), "test_group"); | ||
| assert_eq!(body.get_offset_request(), 100); | ||
| assert_eq!(body.get_offset_new(), 200); | ||
| assert_eq!(body.get_message_queue(), &MessageQueue::new()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn offest_moved_event_setters() { | ||
| let mut body = OffsetMovedEvent { | ||
| consumer_group: "test_group".to_string(), | ||
| message_queue: MessageQueue::new(), | ||
| offset_request: 100, | ||
| offset_new: 200, | ||
| }; | ||
| body.set_consumer_group("new_group".to_string()); | ||
| body.set_offset_request(150); | ||
| body.set_offset_new(250); | ||
| assert_eq!(body.get_consumer_group(), "new_group"); | ||
| assert_eq!(body.get_offset_request(), 150); | ||
| assert_eq!(body.get_offset_new(), 250); | ||
| } | ||
|
|
||
| #[test] | ||
| fn offset_moved_event_getters() { | ||
| let body = OffsetMovedEvent { | ||
| consumer_group: "test_group".to_string(), | ||
| message_queue: MessageQueue::new(), | ||
| offset_request: 100, | ||
| offset_new: 200, | ||
| }; | ||
| assert_eq!(body.get_consumer_group(), "test_group"); | ||
| assert_eq!(body.get_offset_request(), 100); | ||
| assert_eq!(body.get_offset_new(), 200); | ||
| } | ||
|
|
||
| #[test] | ||
| fn offset_moved_event_display() { | ||
| let body = OffsetMovedEvent { | ||
| consumer_group: "test_group".to_string(), | ||
| message_queue: MessageQueue::new(), | ||
| offset_request: 100, | ||
| offset_new: 200, | ||
| }; | ||
| let display = format!("{}", body); | ||
| assert!(display.contains("OffsetMovedEvent")); | ||
| assert!(display.contains("consumer_group=test_group")); | ||
| assert!(display.contains("offset_request=100")); | ||
| assert!(display.contains("offset_new=200")); | ||
| } | ||
| #[test] | ||
| fn offset_moved_event_serialise() { | ||
| let body = OffsetMovedEvent { | ||
| consumer_group: "test_group".to_string(), | ||
| message_queue: MessageQueue::new(), | ||
| offset_request: 100, | ||
| offset_new: 200, | ||
| }; | ||
| let json = serde_json::to_string(&body).unwrap(); | ||
| assert!(json.contains("\"consumer_group\":\"test_group\"")); | ||
| assert!(json.contains("\"offset_request\":100")); | ||
| assert!(json.contains("\"offset_new\":200")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn offset_moved_event_deserialise() { | ||
| let json = r#"{"consumer_group":"test_group","message_queue":{"topic":"","brokerName":"","queueId":0},"offset_request":100,"offset_new":200}"#; | ||
| let body: OffsetMovedEvent = serde_json::from_str(json).unwrap(); | ||
| assert_eq!(body.get_consumer_group(), "test_group"); | ||
| assert_eq!(body.get_offset_request(), 100); | ||
| assert_eq!(body.get_offset_new(), 200); | ||
| assert_eq!(body.get_message_queue(), &MessageQueue::new()); | ||
| } | ||
| } |
There was a problem hiding this comment.
The test naming pattern is inconsistent with conventions found in similar test files in this codebase. For example, in protocol/admin/topic_offset.rs and protocol/admin/rollback_stats.rs, test names use underscores to separate words and typically follow patterns like test_<module>_<behavior> or <module>_<behavior>.
Consider renaming tests to follow a more consistent pattern:
offset_moved_event_init→offset_moved_event_field_initializationor similaroffset_moved_event_serialise→offset_moved_event_serializationoffset_moved_event_deserialise→offset_moved_event_deserialization
Using the full "serialization" and "deserialization" spelling aligns better with other test files in the codebase.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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 #6319 +/- ##
==========================================
+ Coverage 40.55% 42.56% +2.01%
==========================================
Files 886 911 +25
Lines 122833 127899 +5066
==========================================
+ Hits 49810 54446 +4636
- Misses 73023 73453 +430 ☔ 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