Skip to content

[ISSUE #1342]🧪Add Unit test for BrokerMemberGroup #1343

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 26, 2024
Merged

[ISSUE #1342]🧪Add Unit test for BrokerMemberGroup #1343

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 84 additions & 0 deletions rocketmq-remoting/src/protocol/body/broker_body/broker_member_group.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,87 @@ impl BrokerMemberGroup {
pub struct GetBrokerMemberGroupResponseBody {
pub broker_member_group: Option<BrokerMemberGroup>,
}

#[cfg(test)]
mod tests {
use std::collections::HashMap;

use cheetah_string::CheetahString;

use super::*;

#[test]
fn new_creates_broker_member_group_with_empty_broker_addrs() {
let cluster = CheetahString::from("test_cluster");
let broker_name = CheetahString::from("test_broker");
let group = BrokerMemberGroup::new(cluster.clone(), broker_name.clone());

assert_eq!(group.cluster, cluster);
assert_eq!(group.broker_name, broker_name);
assert!(group.broker_addrs.is_empty());
}

#[test]
fn broker_member_group_serializes_correctly() {
let cluster = CheetahString::from("test_cluster");
let broker_name = CheetahString::from("test_broker");
let mut broker_addrs = HashMap::new();
broker_addrs.insert(1, CheetahString::from("127.0.0.1:10911"));
let group = BrokerMemberGroup {
cluster: cluster.clone(),
broker_name: broker_name.clone(),
broker_addrs: broker_addrs.clone(),
};

let serialized = serde_json::to_string(&group).unwrap();
let expected = format!(
r#"{{"cluster":"{}","brokerName":"{}","brokerAddrs":{{"1":"{}"}}}}"#,
cluster,
broker_name,
broker_addrs.get(&1).unwrap()
);
assert_eq!(serialized, expected);
}
Comment on lines +66 to +86
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve serialization test robustness

The current test has some potential issues:

  1. Manual string comparison is fragile as JSON field order isn't guaranteed
  2. Limited test coverage for broker_addrs variations

Consider this improved approach:

#[test]
fn broker_member_group_serializes_correctly() {
    let mut group = BrokerMemberGroup::new(
        CheetahString::from("test_cluster"),
        CheetahString::from("test_broker")
    );
    
    // Test empty broker_addrs
    let serialized = serde_json::to_string(&group).unwrap();
    let deserialized: serde_json::Value = serde_json::from_str(&serialized).unwrap();
    assert_eq!(deserialized["brokerAddrs"], serde_json::json!({}));
    
    // Test multiple broker addresses
    group.broker_addrs.insert(1, CheetahString::from("127.0.0.1:10911"));
    group.broker_addrs.insert(2, CheetahString::from("127.0.0.1:10912"));
    
    let serialized = serde_json::to_string(&group).unwrap();
    let deserialized: serde_json::Value = serde_json::from_str(&serialized).unwrap();
    assert_eq!(deserialized["cluster"], "test_cluster");
    assert_eq!(deserialized["brokerAddrs"]["1"], "127.0.0.1:10911");
    assert_eq!(deserialized["brokerAddrs"]["2"], "127.0.0.1:10912");
}


#[test]
fn broker_member_group_deserializes_correctly() {
let data = r#"{"cluster":"test_cluster","brokerName":"test_broker","brokerAddrs":{"1":"127.0.0.1:10911"}}"#;
let group: BrokerMemberGroup = serde_json::from_str(data).unwrap();

assert_eq!(group.cluster, CheetahString::from("test_cluster"));
assert_eq!(group.broker_name, CheetahString::from("test_broker"));
assert_eq!(
group.broker_addrs.get(&1).unwrap(),
&CheetahString::from("127.0.0.1:10911")
);
}
Comment on lines +88 to +99
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance deserialization test coverage

The current test only covers the happy path. Consider adding tests for:

  1. Error cases with malformed JSON
  2. Missing optional fields
  3. Additional unknown fields (should be ignored)
#[test]
fn broker_member_group_deserializes_correctly() {
    // Test missing fields
    let data = r#"{"cluster":"test_cluster","brokerName":"test_broker"}"#;
    let group: BrokerMemberGroup = serde_json::from_str(data).unwrap();
    assert!(group.broker_addrs.is_empty());
    
    // Test unknown fields
    let data = r#"{"cluster":"test_cluster","brokerName":"test_broker","unknown":"field"}"#;
    let _group: BrokerMemberGroup = serde_json::from_str(data).unwrap();
    
    // Test error cases
    let data = r#"{"cluster":123,"brokerName":"test_broker"}"#;
    assert!(serde_json::from_str::<BrokerMemberGroup>(data).is_err());
}


#[test]
fn get_broker_member_group_response_body_serializes_correctly() {
let cluster = CheetahString::from("test_cluster");
let broker_name = CheetahString::from("test_broker");
let group = BrokerMemberGroup::new(cluster.clone(), broker_name.clone());
let response_body = GetBrokerMemberGroupResponseBody {
broker_member_group: Some(group.clone()),
};

let serialized = serde_json::to_string(&response_body).unwrap();
let expected = format!(
r#"{{"brokerMemberGroup":{{"cluster":"{}","brokerName":"{}","brokerAddrs":{{}}}}}}"#,
cluster, broker_name
);
assert_eq!(serialized, expected);
}
Comment on lines +101 to +116
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve response body serialization test robustness

Similar to the earlier serialization test, this test could be improved by:

  1. Avoiding manual string comparison
  2. Testing both Some and None cases for broker_member_group
#[test]
fn get_broker_member_group_response_body_serializes_correctly() {
    // Test None case
    let response_body = GetBrokerMemberGroupResponseBody { broker_member_group: None };
    let serialized = serde_json::to_string(&response_body).unwrap();
    let deserialized: serde_json::Value = serde_json::from_str(&serialized).unwrap();
    assert!(deserialized["brokerMemberGroup"].is_null());
    
    // Test Some case
    let group = BrokerMemberGroup::new(
        CheetahString::from("test_cluster"),
        CheetahString::from("test_broker")
    );
    let response_body = GetBrokerMemberGroupResponseBody {
        broker_member_group: Some(group),
    };
    
    let serialized = serde_json::to_string(&response_body).unwrap();
    let deserialized: serde_json::Value = serde_json::from_str(&serialized).unwrap();
    assert_eq!(deserialized["brokerMemberGroup"]["cluster"], "test_cluster");
}


#[test]
fn get_broker_member_group_response_body_deserializes_correctly() {
let data = r#"{"brokerMemberGroup":{"cluster":"test_cluster","brokerName":"test_broker","brokerAddrs":{}}}"#;
let response_body: GetBrokerMemberGroupResponseBody = serde_json::from_str(data).unwrap();

assert!(response_body.broker_member_group.is_some());
let group = response_body.broker_member_group.unwrap();
assert_eq!(group.cluster, CheetahString::from("test_cluster"));
assert_eq!(group.broker_name, CheetahString::from("test_broker"));
assert!(group.broker_addrs.is_empty());
}
Comment on lines +118 to +128
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance response body deserialization test coverage

The test should cover additional cases:

  1. Deserializing when brokerMemberGroup is null/missing
  2. Error cases with malformed JSON
#[test]
fn get_broker_member_group_response_body_deserializes_correctly() {
    // Test null case
    let data = r#"{"brokerMemberGroup":null}"#;
    let response: GetBrokerMemberGroupResponseBody = serde_json::from_str(data).unwrap();
    assert!(response.broker_member_group.is_none());
    
    // Test missing field
    let data = r#"{}"#;
    let response: GetBrokerMemberGroupResponseBody = serde_json::from_str(data).unwrap();
    assert!(response.broker_member_group.is_none());
    
    // Test malformed data
    let data = r#"{"brokerMemberGroup":{"cluster":123}}"#;
    assert!(serde_json::from_str::<GetBrokerMemberGroupResponseBody>(data).is_err());
}

}
Loading