[ISSUE #6213] [Test🧪] Add unit tests for QueueTimeSpan #6309
Conversation
rocketmq-rust-bot
requested review from
SpaceXCN,
mxsm and
rocketmq-rust-bot
|
🔊@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
WalkthroughAdds a test module for 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. 🎉 🧹 Recent nitpick comments
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
This PR adds comprehensive unit tests for the QueueTimeSpan struct in the rocketmq-remoting module, addressing issue #6213. The tests cover struct initialization, getters/setters, serialization/deserialization, Clone trait, and time string formatting methods.
Changes:
- Added 7 test functions covering initialization, cloning, serialization, deserialization, getters, setters, and time formatting
- Tests verify correct behavior of all public methods in the QueueTimeSpan struct
- Includes validation of JSON serialization format with proper field naming
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🤖 Fix all issues with AI agents
In `@rocketmq-remoting/src/protocol/body/queue_time_span.rs`:
- Around line 180-193: Rename the test function from qeueu_span_time_str to
queue_span_time_str and make the assertions timezone-independent by computing
expected strings with the same formatter used in production
(time_millis_to_human_string2) rather than hardcoding UTC+1 values; for example,
construct the QueueTimeSpan as shown and assert that
body.get_min_time_stamp_str() ==
time_millis_to_human_string2(body.min_time_stamp) (and similarly for
get_max_time_stamp_str/get_consume_time_stamp_str) so the test uses the same
formatting logic and will pass on any CI host.
🧹 Nitpick comments (1)
rocketmq-remoting/src/protocol/body/queue_time_span.rs (1)
113-130: Test name is misleading — this is a roundtrip test, not a pure deserialization test.This test serializes then deserializes, so it's effectively a roundtrip. Consider renaming to
queue_span_roundtripand adding a separate deserialization test that starts from a hardcoded JSON string. A dedicated deserialization test would catch issues where field renames or serde attributes break ingestion of externally-produced JSON.
| #[test] | ||
| fn qeueu_span_time_str() { | ||
| let body: QueueTimeSpan = QueueTimeSpan { | ||
| message_queue: Some(MessageQueue::new()), | ||
| min_time_stamp: 1, | ||
| max_time_stamp: 2, | ||
| consume_time_stamp: 3, | ||
| delay_time: 4, | ||
| }; | ||
|
|
||
| assert_eq!(body.get_min_time_stamp_str(), "1970-01-01 01:00:00,001".to_string()); | ||
| assert_eq!(body.get_max_time_stamp_str(), "1970-01-01 01:00:00,002".to_string()); | ||
| assert_eq!(body.get_consume_time_stamp_str(), "1970-01-01 01:00:00,003".to_string()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'fn time_millis_to_human_string2' --type rust -A 20Repository: mxsm/rocketmq-rust
Length of output: 3084
🏁 Script executed:
cd rocketmq-remoting/src/protocol/body && head -20 queue_time_span.rs && echo "---" && sed -n '180,193p' queue_time_span.rsRepository: mxsm/rocketmq-rust
Length of output: 1462
Typo in test name and timezone-dependent assertions will cause CI failures.
Two issues:
- Typo:
qeueu_span_time_str→queue_span_time_str. - Timezone-sensitive: The expected strings (e.g.,
"1970-01-01 01:00:00,001") assume UTC+1.time_millis_to_human_string2formats usingLocal::timestamp_millis_opt(), so this test will fail on any machine not running in UTC+1 (including most CI environments). Compute expected strings dynamically using the same formatting function:
Fix both issues
- fn qeueu_span_time_str() {
+ fn queue_span_time_str() {
let body: QueueTimeSpan = QueueTimeSpan {
message_queue: Some(MessageQueue::new()),
min_time_stamp: 1,
max_time_stamp: 2,
consume_time_stamp: 3,
delay_time: 4,
};
- assert_eq!(body.get_min_time_stamp_str(), "1970-01-01 01:00:00,001".to_string());
- assert_eq!(body.get_max_time_stamp_str(), "1970-01-01 01:00:00,002".to_string());
- assert_eq!(body.get_consume_time_stamp_str(), "1970-01-01 01:00:00,003".to_string());
+ assert_eq!(body.get_min_time_stamp_str(), time_millis_to_human_string2(1));
+ assert_eq!(body.get_max_time_stamp_str(), time_millis_to_human_string2(2));
+ assert_eq!(body.get_consume_time_stamp_str(), time_millis_to_human_string2(3));🤖 Prompt for AI Agents
In `@rocketmq-remoting/src/protocol/body/queue_time_span.rs` around lines 180 -
193, Rename the test function from qeueu_span_time_str to queue_span_time_str
and make the assertions timezone-independent by computing expected strings with
the same formatter used in production (time_millis_to_human_string2) rather than
hardcoding UTC+1 values; for example, construct the QueueTimeSpan as shown and
assert that body.get_min_time_stamp_str() ==
time_millis_to_human_string2(body.min_time_stamp) (and similarly for
get_max_time_stamp_str/get_consume_time_stamp_str) so the test uses the same
formatting logic and will pass on any CI host.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6309 +/- ##
==========================================
+ Coverage 42.20% 42.48% +0.27%
==========================================
Files 901 909 +8
Lines 126236 127280 +1044
==========================================
+ Hits 53284 54069 +785
- Misses 72952 73211 +259 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
left a comment
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
3 participants
Which Issue(s) This PR Fixes(Closes)
Brief Description
How Did You Test This Change?
Summary by CodeRabbit