Skip to content
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.

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

[ISSUE #1321]🧪Add unit test for OperationResult #1322

Merged
merged 1 commit into from
Nov 26, 2024
Merged
Changes from all commits
Commits
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
45 changes: 45 additions & 0 deletions rocketmq-broker/src/transaction/operation_result.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,48 @@ impl Default for OperationResult {
}
}
}

#[cfg(test)]
mod tests {
use rocketmq_common::common::message::message_ext::MessageExt;
use rocketmq_remoting::code::response_code::ResponseCode;

use super::*;

#[test]
fn default_operation_result_has_none_fields() {
let result = OperationResult::default();
assert!(result.prepare_message.is_none());
assert!(result.response_remark.is_none());
assert_eq!(result.response_code, ResponseCode::Success);
}

#[test]
fn operation_result_with_some_fields() {
let message = MessageExt::default();
let remark = Some(String::from("Test remark"));
let response_code = ResponseCode::SystemError;

let result = OperationResult {
prepare_message: Some(message.clone()),
response_remark: remark.clone(),
response_code,
};

assert_eq!(result.response_remark, remark);
assert_eq!(result.response_code, response_code);
}
Comment on lines +53 to +67
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

Test coverage and efficiency improvements needed

There are several issues to address:

  1. The prepare_message field is not being asserted after initialization
  2. Unnecessary clone() calls can be removed
     #[test]
     fn operation_result_with_some_fields() {
         let message = MessageExt::default();
         let remark = Some(String::from("Test remark"));
         let response_code = ResponseCode::SystemError;

         let result = OperationResult {
-            prepare_message: Some(message.clone()),
-            response_remark: remark.clone(),
+            prepare_message: Some(message),
+            response_remark: remark,
             response_code,
         };

+        assert!(result.prepare_message.is_some(), "prepare_message should be Some");
         assert_eq!(result.response_remark, remark);
         assert_eq!(result.response_code, response_code);
     }

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


#[test]
fn operation_result_with_none_fields() {
let result = OperationResult {
prepare_message: None,
response_remark: None,
response_code: ResponseCode::Success,
};

assert!(result.prepare_message.is_none());
assert!(result.response_remark.is_none());
assert_eq!(result.response_code, ResponseCode::Success);
}
Comment on lines +69 to +80
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

Consider removing redundant test

This test appears to be redundant with default_operation_result_has_none_fields as it tests the same scenario. Consider either:

  1. Removing this test entirely, or
  2. Modifying it to test a different scenario (e.g., transitioning from Some to None values)

If you choose option 2, here's a suggested replacement:

#[test]
fn operation_result_can_transition_to_none() {
    // Start with Some values
    let result = OperationResult {
        prepare_message: Some(MessageExt::default()),
        response_remark: Some("initial".to_string()),
        response_code: ResponseCode::Success,
    };

    // Transition to None
    let result = OperationResult {
        prepare_message: None,
        response_remark: None,
        ..result
    };

    assert!(result.prepare_message.is_none(), "prepare_message should transition to None");
    assert!(result.response_remark.is_none(), "response_remark should transition to None");
}

}
Comment on lines +38 to +81
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

Additional test scenarios needed for comprehensive coverage

Consider adding tests for the following scenarios:

  1. Response codes other than Success/SystemError
  2. Response remarks with special characters or empty strings
  3. Complex MessageExt instances
  4. Boundary conditions and edge cases

Would you like me to provide example implementations for these additional test cases?

Loading