Conversation
|
🔊@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
WalkthroughComprehensive unit tests have been added to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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
Adds/expands unit test coverage for RocketMQ remoting protocol request headers, primarily focusing on QueryTopicsByConsumerRequestHeader serialization/deserialization and basic accessor behavior.
Changes:
- Add a comprehensive suite of unit tests for
QueryTopicsByConsumerRequestHeader(debug, getters/setters, serde edge cases). - Add new unit tests for
GetConsumeStatsRequestHeader(getters/setters and serde round-trip) and remove obsolete commented constants.
Reviewed changes
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| rocketmq-remoting/src/protocol/header/query_topics_by_consumer_request_header.rs | Adds multiple unit tests around debug output, getters/setters, serde error cases, and round-trip behavior. |
| rocketmq-remoting/src/protocol/header/get_consume_stats_request_header.rs | Removes commented-out constants and introduces a new test module for header accessors + serde behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| use std::mem; | ||
| let _sz = mem::size_of::<QueryTopicsByConsumerRequestHeader>(); | ||
| assert!(_sz > 0); | ||
|
|
There was a problem hiding this comment.
This test asserts size_of::<QueryTopicsByConsumerRequestHeader>() > 0, which is effectively a tautology here and doesn’t validate any observable behavior. Consider removing the size check or replacing it with a meaningful invariant that the code relies on (e.g., serialization/deserialization behavior only).
| use std::mem; | |
| let _sz = mem::size_of::<QueryTopicsByConsumerRequestHeader>(); | |
| assert!(_sz > 0); |
rocketmq-rust-bot
left a comment
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
rocketmq-remoting/src/protocol/header/query_topics_by_consumer_request_header.rs (3)
129-134: Test name is misleading — this tests serde's behavior, not the#[required]macro attribute.
#[required]is a custom attribute consumed by theRequestHeaderCodecV2/FromMapcodec, not by serde. What this test actually validates is that serde refuses to deserialize a struct when a non-optional, non-defaulted field (group: CheetahString) is absent. Consider renaming toserde_deserialize_missing_group_field_errorsand adding a complementary test that verifiesFromMapreturns an error whengroupis absent (exercising the actual#[required]constraint).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rocketmq-remoting/src/protocol/header/query_topics_by_consumer_request_header.rs` around lines 129 - 134, Rename the test serde_deserialize_missing_required_field_errors to serde_deserialize_missing_group_field_errors to accurately reflect it verifies serde refusal to deserialize QueryTopicsByConsumerRequestHeader when the non-optional field group: CheetahString is missing, and add a new test that constructs a Map (or uses RequestHeaderCodecV2::from_map / FromMap for QueryTopicsByConsumerRequestHeader) with the group key omitted and asserts that FromMap returns an Err to exercise the #[required] attribute enforcement; update test names and assertions accordingly to keep both serde and codec behaviors covered.
119-127: Prefer direct public field access over getter/setter methods in tests.The struct fields
groupandrpc_request_headerare alreadypub. Testingget_group()/set_group()couples the tests to convenience methods that may be out of pattern with the rest of the codebase. Direct field access better reflects the established convention.♻️ Suggested alternative
fn getter_setter_multiple_calls() { let mut header = QueryTopicsByConsumerRequestHeader::new("g1"); - assert_eq!(header.get_group(), "g1"); - header.set_group(CheetahString::from("g2")); - assert_eq!(header.get_group(), "g2"); - header.set_group(CheetahString::from("g3")); - assert_eq!(header.get_group(), "g3"); + assert_eq!(header.group, "g1"); + header.group = CheetahString::from("g2"); + assert_eq!(header.group, "g2"); + header.group = CheetahString::from("g3"); + assert_eq!(header.group, "g3"); }Based on learnings: header structs in
rocketmq-remoting/src/protocol/header/should consistently use public fields rather than private fields with getters/setters; direct public access is the preferred pattern across this module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rocketmq-remoting/src/protocol/header/query_topics_by_consumer_request_header.rs` around lines 119 - 127, The test getter_setter_multiple_calls uses the convenience methods get_group and set_group on QueryTopicsByConsumerRequestHeader but this module prefers direct public field access; update the test to read and write header.group directly (e.g., assert_eq!(header.group, "g1") and header.group = CheetahString::from("g2")) and similarly replace subsequent get/set calls with direct assignments and assertions; ensure any use of rpc_request_header in other tests follows the same public-field access pattern.
235-246:mem::size_of::<T>() > 0is a trivially-true no-op assertion.Any non-ZST Rust type has a non-zero size. This assertion never catches a real regression. Either remove the
_szcheck or replace it with a stronger structural invariant (e.g., minimum expected size).♻️ Suggested simplification
fn struct_size_check_and_empty_group_behavior() { - use std::mem; - let _sz = mem::size_of::<QueryTopicsByConsumerRequestHeader>(); - assert!(_sz > 0); - let header_empty = QueryTopicsByConsumerRequestHeader::new(""); let json = serde_json::to_string(&header_empty).unwrap(); assert!(json.contains("\"group\":\"\"")); let header2: QueryTopicsByConsumerRequestHeader = serde_json::from_str(&json).unwrap(); assert_eq!(header2.group, ""); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rocketmq-remoting/src/protocol/header/query_topics_by_consumer_request_header.rs` around lines 235 - 246, The test struct_size_check_and_empty_group_behavior contains a meaningless assertion using mem::size_of::<QueryTopicsByConsumerRequestHeader>() > 0; remove that no-op assertion (or replace it with a concrete expected-size check if you want a real invariant) and keep the rest of the test (creating QueryTopicsByConsumerRequestHeader::new(""), serializing to JSON, asserting it contains "\"group\":\"\"", and round-tripping via serde to assert header2.group == ""); update the test to either drop the _sz variable entirely or change it to assert_eq!(mem::size_of::<QueryTopicsByConsumerRequestHeader>(), <expected_size>) if you have a specific expected byte size in mind.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@rocketmq-remoting/src/protocol/header/query_topics_by_consumer_request_header.rs`:
- Around line 129-134: Rename the test
serde_deserialize_missing_required_field_errors to
serde_deserialize_missing_group_field_errors to accurately reflect it verifies
serde refusal to deserialize QueryTopicsByConsumerRequestHeader when the
non-optional field group: CheetahString is missing, and add a new test that
constructs a Map (or uses RequestHeaderCodecV2::from_map / FromMap for
QueryTopicsByConsumerRequestHeader) with the group key omitted and asserts that
FromMap returns an Err to exercise the #[required] attribute enforcement; update
test names and assertions accordingly to keep both serde and codec behaviors
covered.
- Around line 119-127: The test getter_setter_multiple_calls uses the
convenience methods get_group and set_group on
QueryTopicsByConsumerRequestHeader but this module prefers direct public field
access; update the test to read and write header.group directly (e.g.,
assert_eq!(header.group, "g1") and header.group = CheetahString::from("g2")) and
similarly replace subsequent get/set calls with direct assignments and
assertions; ensure any use of rpc_request_header in other tests follows the same
public-field access pattern.
- Around line 235-246: The test struct_size_check_and_empty_group_behavior
contains a meaningless assertion using
mem::size_of::<QueryTopicsByConsumerRequestHeader>() > 0; remove that no-op
assertion (or replace it with a concrete expected-size check if you want a real
invariant) and keep the rest of the test (creating
QueryTopicsByConsumerRequestHeader::new(""), serializing to JSON, asserting it
contains "\"group\":\"\"", and round-tripping via serde to assert header2.group
== ""); update the test to either drop the _sz variable entirely or change it to
assert_eq!(mem::size_of::<QueryTopicsByConsumerRequestHeader>(),
<expected_size>) if you have a specific expected byte size in mind.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6376 +/- ##
==========================================
+ Coverage 42.42% 42.46% +0.04%
==========================================
Files 921 921
Lines 129596 129703 +107
==========================================
+ Hits 54980 55079 +99
- Misses 74616 74624 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
added
approved
oopscompiled
deleted the
test/add-test-case-QueryTopicsByConsumerRequestHeader
branch
4 participants
Which Issue(s) This PR Fixes(Closes)
Brief Description
How Did You Test This Change?
Summary by CodeRabbit