[ISSUE #1347]🧪Add unit test for route_data_view #1348
Merged
Conversation
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
WalkthroughThe changes introduce a new test module in the Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
🔊@ButterflyEffect1991 🚀Thanks for your contribution 🎉. CodeRabbit(AI) will review your code first 🔥 |
mxsm
approved these changes
Nov 26, 2024
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
rocketmq-remoting/src/protocol/route/route_data_view.rs (1)
181-287: Add tests for missing scenariosThe test suite would benefit from additional test cases to cover:
- Empty broker addresses in
select_broker_addr- Comparison operations (
Ord,PartialOrdtraits)- Edge cases for
remove_broker_by_addr(non-existent broker/address)Here's an example of additional test cases to consider:
#[test] fn broker_data_select_broker_addr_returns_none_when_empty() { let broker_data = BrokerData::new( CheetahString::from("cluster1"), CheetahString::from("broker1"), HashMap::new(), None, ); assert!(broker_data.select_broker_addr().is_none()); } #[test] fn broker_data_comparison_works_correctly() { let broker1 = BrokerData::new( CheetahString::from("cluster1"), CheetahString::from("broker1"), HashMap::new(), None, ); let broker2 = BrokerData::new( CheetahString::from("cluster1"), CheetahString::from("broker2"), HashMap::new(), None, ); assert!(broker1 < broker2); } #[test] fn broker_data_remove_nonexistent_broker_works_correctly() { let mut broker_data = BrokerData::new( CheetahString::from("cluster1"), CheetahString::from("broker1"), HashMap::from([(1, CheetahString::from("127.0.0.1"))]), None, ); broker_data.remove_broker_by_addr(2, "127.0.0.2"); assert_eq!(broker_data.broker_addrs().len(), 1); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
rocketmq-remoting/src/protocol/route/route_data_view.rs(1 hunks)
🔇 Additional comments (1)
rocketmq-remoting/src/protocol/route/route_data_view.rs (1)
182-187: LGTM: Test module structure follows best practices
The test module is well-organized with proper use of #[cfg(test)] attribute and necessary imports.
Comment on lines
+234
to
+249
| #[test] | ||
| fn broker_data_remove_broker_by_addr_works_correctly() { | ||
| let mut broker_data = BrokerData::new( | ||
| CheetahString::from("cluster1"), | ||
| CheetahString::from("broker1"), | ||
| HashMap::from([ | ||
| (1, CheetahString::from("127.0.0.1")), | ||
| (2, CheetahString::from("127.0.0.2")), | ||
| ]), | ||
| None, | ||
| ); | ||
|
|
||
| broker_data.remove_broker_by_addr(1, "127.0.0.1"); | ||
| //assert!(broker_data.broker_addrs.get(&1).is_none()); | ||
| assert!(broker_data.broker_addrs.get(&2).is_some()); | ||
| } |
There was a problem hiding this comment.
Uncomment and fix the broker removal test
The test broker_data_remove_broker_by_addr_works_correctly has a commented-out assertion that should be included to properly verify the broker removal functionality.
Apply this diff to complete the test:
broker_data.remove_broker_by_addr(1, "127.0.0.1");
- //assert!(broker_data.broker_addrs.get(&1).is_none());
+ assert!(broker_data.broker_addrs.get(&1).is_none());
assert!(broker_data.broker_addrs.get(&2).is_some());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[test] | |
| fn broker_data_remove_broker_by_addr_works_correctly() { | |
| let mut broker_data = BrokerData::new( | |
| CheetahString::from("cluster1"), | |
| CheetahString::from("broker1"), | |
| HashMap::from([ | |
| (1, CheetahString::from("127.0.0.1")), | |
| (2, CheetahString::from("127.0.0.2")), | |
| ]), | |
| None, | |
| ); | |
| broker_data.remove_broker_by_addr(1, "127.0.0.1"); | |
| //assert!(broker_data.broker_addrs.get(&1).is_none()); | |
| assert!(broker_data.broker_addrs.get(&2).is_some()); | |
| } | |
| #[test] | |
| fn broker_data_remove_broker_by_addr_works_correctly() { | |
| let mut broker_data = BrokerData::new( | |
| CheetahString::from("cluster1"), | |
| CheetahString::from("broker1"), | |
| HashMap::from([ | |
| (1, CheetahString::from("127.0.0.1")), | |
| (2, CheetahString::from("127.0.0.2")), | |
| ]), | |
| None, | |
| ); | |
| broker_data.remove_broker_by_addr(1, "127.0.0.1"); | |
| assert!(broker_data.broker_addrs.get(&1).is_none()); | |
| assert!(broker_data.broker_addrs.get(&2).is_some()); | |
| } |
Comment on lines
+264
to
+275
| #[test] | ||
| fn broker_data_select_broker_addr_returns_random_if_no_master() { | ||
| let broker_data = BrokerData::new( | ||
| CheetahString::from("cluster1"), | ||
| CheetahString::from("broker1"), | ||
| HashMap::from([(2, CheetahString::from("127.0.0.2"))]), | ||
| None, | ||
| ); | ||
|
|
||
| let selected_addr = broker_data.select_broker_addr(); | ||
| assert_eq!(selected_addr.unwrap(), CheetahString::from("127.0.0.2")); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance random broker selection test coverage
The current test for select_broker_addr with no master doesn't properly verify the random selection behavior since it only includes one address.
Consider adding multiple addresses and running the test multiple times to verify random selection:
let broker_data = BrokerData::new(
CheetahString::from("cluster1"),
CheetahString::from("broker1"),
- HashMap::from([(2, CheetahString::from("127.0.0.2"))]),
+ HashMap::from([
+ (2, CheetahString::from("127.0.0.2")),
+ (3, CheetahString::from("127.0.0.3")),
+ (4, CheetahString::from("127.0.0.4")),
+ ]),
None,
);
- let selected_addr = broker_data.select_broker_addr();
- assert_eq!(selected_addr.unwrap(), CheetahString::from("127.0.0.2"));
+ // Run multiple times to verify random selection
+ let mut seen_addresses = std::collections::HashSet::new();
+ for _ in 0..50 {
+ let selected_addr = broker_data.select_broker_addr().unwrap();
+ seen_addresses.insert(selected_addr);
+ }
+ // Should see multiple different addresses
+ assert!(seen_addresses.len() > 1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[test] | |
| fn broker_data_select_broker_addr_returns_random_if_no_master() { | |
| let broker_data = BrokerData::new( | |
| CheetahString::from("cluster1"), | |
| CheetahString::from("broker1"), | |
| HashMap::from([(2, CheetahString::from("127.0.0.2"))]), | |
| None, | |
| ); | |
| let selected_addr = broker_data.select_broker_addr(); | |
| assert_eq!(selected_addr.unwrap(), CheetahString::from("127.0.0.2")); | |
| } | |
| #[test] | |
| fn broker_data_select_broker_addr_returns_random_if_no_master() { | |
| let broker_data = BrokerData::new( | |
| CheetahString::from("cluster1"), | |
| CheetahString::from("broker1"), | |
| HashMap::from([ | |
| (2, CheetahString::from("127.0.0.2")), | |
| (3, CheetahString::from("127.0.0.3")), | |
| (4, CheetahString::from("127.0.0.4")), | |
| ]), | |
| None, | |
| ); | |
| // Run multiple times to verify random selection | |
| let mut seen_addresses = std::collections::HashSet::new(); | |
| for _ in 0..50 { | |
| let selected_addr = broker_data.select_broker_addr().unwrap(); | |
| seen_addresses.insert(selected_addr); | |
| } | |
| // Should see multiple different addresses | |
| assert!(seen_addresses.len() > 1); | |
| } |
Comment on lines
+181
to
+287
| #[test] | ||
| fn broker_data_new_initializes_correctly() { | ||
| let cluster = CheetahString::from("test_cluster"); | ||
| let broker_name = CheetahString::from("test_broker"); | ||
| let broker_addrs = HashMap::new(); | ||
| let zone_name = Some(CheetahString::from("test_zone")); | ||
|
|
||
| let broker_data = BrokerData::new( | ||
| cluster.clone(), | ||
| broker_name.clone(), | ||
| broker_addrs.clone(), | ||
| zone_name.clone(), | ||
| ); | ||
|
|
||
| assert_eq!(broker_data.cluster, cluster); | ||
| assert_eq!(broker_data.broker_name, broker_name); | ||
| assert_eq!(broker_data.broker_addrs, broker_addrs); | ||
| assert_eq!(broker_data.zone_name, zone_name); | ||
| assert!(!broker_data.enable_acting_master); | ||
| } | ||
|
|
||
| #[test] | ||
| fn broker_data_setters_work_correctly() { | ||
| let mut broker_data = BrokerData::new( | ||
| CheetahString::from("cluster1"), | ||
| CheetahString::from("broker1"), | ||
| HashMap::new(), | ||
| None, | ||
| ); | ||
|
|
||
| broker_data.set_cluster(CheetahString::from("cluster2")); | ||
| broker_data.set_broker_name(CheetahString::from("broker2")); | ||
| broker_data.set_broker_addrs(HashMap::from([(1, CheetahString::from("127.0.0.1"))])); | ||
| broker_data.set_zone_name(Some(CheetahString::from("zone1"))); | ||
| broker_data.set_enable_acting_master(true); | ||
|
|
||
| assert_eq!(broker_data.cluster, CheetahString::from("cluster2")); | ||
| assert_eq!(broker_data.broker_name, CheetahString::from("broker2")); | ||
| assert_eq!( | ||
| broker_data.broker_addrs.get(&1).unwrap(), | ||
| &CheetahString::from("127.0.0.1") | ||
| ); | ||
| assert_eq!(broker_data.zone_name, Some(CheetahString::from("zone1"))); | ||
| assert!(broker_data.enable_acting_master); | ||
| } | ||
|
|
||
| #[test] | ||
| fn broker_data_remove_broker_by_addr_works_correctly() { | ||
| let mut broker_data = BrokerData::new( | ||
| CheetahString::from("cluster1"), | ||
| CheetahString::from("broker1"), | ||
| HashMap::from([ | ||
| (1, CheetahString::from("127.0.0.1")), | ||
| (2, CheetahString::from("127.0.0.2")), | ||
| ]), | ||
| None, | ||
| ); | ||
|
|
||
| broker_data.remove_broker_by_addr(1, "127.0.0.1"); | ||
| //assert!(broker_data.broker_addrs.get(&1).is_none()); | ||
| assert!(broker_data.broker_addrs.get(&2).is_some()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn broker_data_select_broker_addr_returns_master_if_exists() { | ||
| let broker_data = BrokerData::new( | ||
| CheetahString::from("cluster1"), | ||
| CheetahString::from("broker1"), | ||
| HashMap::from([(mix_all::MASTER_ID, CheetahString::from("127.0.0.1"))]), | ||
| None, | ||
| ); | ||
|
|
||
| let selected_addr = broker_data.select_broker_addr(); | ||
| assert_eq!(selected_addr.unwrap(), CheetahString::from("127.0.0.1")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn broker_data_select_broker_addr_returns_random_if_no_master() { | ||
| let broker_data = BrokerData::new( | ||
| CheetahString::from("cluster1"), | ||
| CheetahString::from("broker1"), | ||
| HashMap::from([(2, CheetahString::from("127.0.0.2"))]), | ||
| None, | ||
| ); | ||
|
|
||
| let selected_addr = broker_data.select_broker_addr(); | ||
| assert_eq!(selected_addr.unwrap(), CheetahString::from("127.0.0.2")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn queue_data_new_initializes_correctly() { | ||
| let queue_data = QueueData::new(CheetahString::from("broker1"), 4, 4, 6, 0); | ||
|
|
||
| assert_eq!(queue_data.broker_name, CheetahString::from("broker1")); | ||
| assert_eq!(queue_data.read_queue_nums, 4); | ||
| assert_eq!(queue_data.write_queue_nums, 4); | ||
| assert_eq!(queue_data.perm, 6); | ||
| assert_eq!(queue_data.topic_sys_flag, 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Tests need additional coverage for missing functionality
The current test suite has gaps in coverage:
- Missing tests for
BrokerData:- Getters:
cluster(),broker_name(),broker_addrs(),broker_addrs_mut(),zone_name(),enable_acting_master() PartialOrdandOrdtrait implementations
- Getters:
- Missing tests for
QueueData:- Getters:
broker_name(),read_queue_nums(),write_queue_nums(),perm(),topic_sys_flag()
- Getters:
- The
remove_broker_by_addrtest has a commented-out assertion that should be uncommented and verified
Additionally, there's a potential issue in the broker_data_select_broker_addr_returns_random_if_no_master test as it assumes deterministic behavior for a random selection.
🔗 Analysis chain
Verify test coverage metrics
While the PR adds unit tests as requested, let's verify the coverage metrics to ensure comprehensive testing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for the route_data_view module
# Note: This assumes cargo-tarpaulin is available in the CI environment
# Run tarpaulin to generate coverage report
cargo tarpaulin --out Xml --output-dir coverage --packages rocketmq-remoting --target-dir target-coverage -- --test-threads=1
# Parse the coverage report
if [ -f coverage/cobertura.xml ]; then
echo "Coverage report for route_data_view.rs:"
grep -A 5 "route_data_view.rs" coverage/cobertura.xml
fi
Length of output: 259
Script:
#!/bin/bash
# Let's analyze the test coverage by examining the test cases and their assertions
# First, let's find all the struct definitions in the file to understand what needs to be tested
ast-grep --pattern 'struct $name {
$$$
}' rocketmq-remoting/src/protocol/route/route_data_view.rs
# Then, let's find all the impl blocks to see what methods are defined
ast-grep --pattern 'impl $name {
$$$
}' rocketmq-remoting/src/protocol/route/route_data_view.rs
# Finally, let's look at the actual file content to compare with test coverage
cat rocketmq-remoting/src/protocol/route/route_data_view.rs
Length of output: 9045
mxsm
approved these changes
Nov 26, 2024
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)
Fixes #1347
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
BrokerDataandQueueData.BrokerDataandQueueData.