Skip to content

[ISSUE #1360]🧪Add unit test for DeleteTopicRequestHeader #1361

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 27, 2024
Merged

[ISSUE #1360]🧪Add unit test for DeleteTopicRequestHeader #1361

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/header/delete_topic_request_header.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,87 @@ impl FromMap for DeleteTopicRequestHeader {
})
}
}

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

use super::*;

#[test]
fn delete_topic_request_header_to_map() {
let header = DeleteTopicRequestHeader {
topic: CheetahString::from("test_topic"),
topic_request_header: None,
};

let map = header.to_map().unwrap();
assert_eq!(
map.get(&CheetahString::from_static_str(
DeleteTopicRequestHeader::TOPIC
))
.unwrap(),
&CheetahString::from("test_topic")
);
}

#[test]
fn delete_topic_request_header_to_map_with_topic_request_header() {
let topic_request_header = TopicRequestHeader {
// Initialize fields as needed
rpc_request_header: None,
lo: None,
};
let header = DeleteTopicRequestHeader {
topic: CheetahString::from("test_topic"),
topic_request_header: Some(topic_request_header),
};

let map = header.to_map().unwrap();
assert_eq!(
map.get(&CheetahString::from_static_str(
DeleteTopicRequestHeader::TOPIC
))
.unwrap(),
&CheetahString::from("test_topic")
);
// Add assertions for fields from topic_request_header
}
Comment on lines +91 to +112
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Complete the test implementation for topic_request_header fields.

The test is incomplete:

  1. The comment "Initialize fields as needed" suggests missing implementation
  2. No assertions for topic_request_header fields in the resulting map
  3. The test doesn't verify the behavior of the flattened fields

Consider implementing the test like this:

     let topic_request_header = TopicRequestHeader {
-        // Initialize fields as needed
-        rpc_request_header: None,
-        lo: None,
+        rpc_request_header: Some(RpcRequestHeader {
+            // Initialize with meaningful test values
+        }),
+        lo: Some(42),
     };
     // ... existing code ...
     assert_eq!(
         map.get(&CheetahString::from_static_str(DeleteTopicRequestHeader::TOPIC))
         .unwrap(),
         &CheetahString::from("test_topic")
     );
-    // Add assertions for fields from topic_request_header
+    // Verify topic_request_header fields
+    assert_eq!(
+        map.get(&CheetahString::from_static_str("lo")).unwrap(),
+        &CheetahString::from("42")
+    );
+    // Add assertions for other topic_request_header fields

Committable suggestion skipped: line range outside the PR's diff.


#[test]
fn delete_topic_request_header_from_map() {
let mut map = HashMap::new();
map.insert(
CheetahString::from_static_str(DeleteTopicRequestHeader::TOPIC),
CheetahString::from("test_topic"),
);

let header = <DeleteTopicRequestHeader as FromMap>::from(&map).unwrap();
assert_eq!(header.topic, CheetahString::from("test_topic"));
assert!(!header.topic_request_header.is_none());
}

Comment on lines +114 to +126
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix incorrect assertion for topic_request_header.

There's a logical error in the test:

  1. The map doesn't contain any topic_request_header fields
  2. The assertion !header.topic_request_header.is_none() expects topic_request_header to be Some
  3. The double negative makes the code harder to read

Fix the assertion:

     let header = <DeleteTopicRequestHeader as FromMap>::from(&map).unwrap();
     assert_eq!(header.topic, CheetahString::from("test_topic"));
-    assert!(!header.topic_request_header.is_none());
+    assert!(header.topic_request_header.is_none());
📝 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 delete_topic_request_header_from_map() {
let mut map = HashMap::new();
map.insert(
CheetahString::from_static_str(DeleteTopicRequestHeader::TOPIC),
CheetahString::from("test_topic"),
);
let header = <DeleteTopicRequestHeader as FromMap>::from(&map).unwrap();
assert_eq!(header.topic, CheetahString::from("test_topic"));
assert!(!header.topic_request_header.is_none());
}
#[test]
fn delete_topic_request_header_from_map() {
let mut map = HashMap::new();
map.insert(
CheetahString::from_static_str(DeleteTopicRequestHeader::TOPIC),
CheetahString::from("test_topic"),
);
let header = <DeleteTopicRequestHeader as FromMap>::from(&map).unwrap();
assert_eq!(header.topic, CheetahString::from("test_topic"));
assert!(header.topic_request_header.is_none());
}

#[test]
fn delete_topic_request_header_from_map_with_topic_request_header() {
let mut map = HashMap::new();
map.insert(
CheetahString::from_static_str(DeleteTopicRequestHeader::TOPIC),
CheetahString::from("test_topic"),
);
// Add entries for fields from topic_request_header

let header = <DeleteTopicRequestHeader as FromMap>::from(&map).unwrap();
assert_eq!(header.topic, CheetahString::from("test_topic"));
assert!(header.topic_request_header.is_some());
// Add assertions for fields from topic_request_header
}
Comment on lines +127 to +140
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Complete the test implementation for topic_request_header fields.

The test is incomplete:

  1. The comment suggests missing topic_request_header field entries
  2. No actual verification of the topic_request_header field values

Complete the implementation:

     let mut map = HashMap::new();
     map.insert(
         CheetahString::from_static_str(DeleteTopicRequestHeader::TOPIC),
         CheetahString::from("test_topic"),
     );
-    // Add entries for fields from topic_request_header
+    // Add topic_request_header fields
+    map.insert(
+        CheetahString::from_static_str("lo"),
+        CheetahString::from("42")
+    );
+    // Add other topic_request_header fields as needed

     let header = <DeleteTopicRequestHeader as FromMap>::from(&map).unwrap();
     assert_eq!(header.topic, CheetahString::from("test_topic"));
     assert!(header.topic_request_header.is_some());
-    // Add assertions for fields from topic_request_header
+    if let Some(topic_request_header) = header.topic_request_header {
+        assert_eq!(topic_request_header.lo, Some(42));
+        // Add assertions for other fields
+    }

Committable suggestion skipped: line range outside the PR's diff.


#[test]
fn delete_topic_request_header_from_map_missing_topic() {
let map = HashMap::new();

let header = <DeleteTopicRequestHeader as FromMap>::from(&map).unwrap();
assert_eq!(header.topic, CheetahString::default());
assert!(!header.topic_request_header.is_none());
}
Comment on lines +142 to +149
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix incorrect assertion for topic_request_header.

The test has the same logical error as the delete_topic_request_header_from_map test:

  1. Empty map shouldn't result in Some(topic_request_header)
  2. The double negative makes the code harder to read

Fix the assertion:

     let header = <DeleteTopicRequestHeader as FromMap>::from(&map).unwrap();
     assert_eq!(header.topic, CheetahString::default());
-    assert!(!header.topic_request_header.is_none());
+    assert!(header.topic_request_header.is_none());
📝 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 delete_topic_request_header_from_map_missing_topic() {
let map = HashMap::new();
let header = <DeleteTopicRequestHeader as FromMap>::from(&map).unwrap();
assert_eq!(header.topic, CheetahString::default());
assert!(!header.topic_request_header.is_none());
}
#[test]
fn delete_topic_request_header_from_map_missing_topic() {
let map = HashMap::new();
let header = <DeleteTopicRequestHeader as FromMap>::from(&map).unwrap();
assert_eq!(header.topic, CheetahString::default());
assert!(header.topic_request_header.is_none());
}

}
Loading