Skip to content

Conversation

@WaterWhisperer
Copy link
Contributor

@WaterWhisperer WaterWhisperer commented Jan 4, 2026

Which Issue(s) This PR Fixes(Closes)

Fixes #5375

Brief Description

How Did You Test This Change?

Summary by CodeRabbit

  • Tests
    • Expanded test coverage for protocol error variants and error message formatting.

✏️ Tip: You can customize this high-level summary in your review settings.

@rocketmq-rust-bot
Copy link
Collaborator

🔊@WaterWhisperer 🚀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💥.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 4, 2026

Walkthrough

Unit tests are added for various ProtocolError variants in the unified protocol error module, verifying correct error message formatting for scenarios including unsupported versions, missing headers and bodies, invalid messages, decode errors, and unsupported serialization types.

Changes

Cohort / File(s) Summary
Protocol Error Tests
rocketmq-error/src/unified/protocol.rs
Adds six unit test cases verifying ProtocolError variant message formatting: UnsupportedVersion, HeaderMissing, BodyMissing, InvalidMessage, DecodeError, and UnsupportedSerializationType. Tests validate expected error message output for each variant.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Poem

🐰 A rabbit tests each error's call,
Versions, headers, messages all!
With assertions crisp and clean,
The finest protocol tests e'er seen! 🧪✨

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly indicates the PR adds test cases for ProtocolError, which matches the main change adding unit tests for various ProtocolError variants.
Linked Issues check ✅ Passed The PR successfully implements test cases for ProtocolError as required by issue #5375, with comprehensive coverage of multiple error variants and their expected formatting.
Out of Scope Changes check ✅ Passed All changes are focused on adding unit tests for ProtocolError variants with no modifications to production code, public APIs, or unrelated functionality.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
rocketmq-error/src/unified/protocol.rs (1)

90-116: Consider adding helper methods for remaining variants.

For consistency with existing patterns (e.g., invalid_command, header_missing), consider adding helper methods for UnsupportedVersion, BodyMissing, DecodeError, and UnsupportedSerializationType. This would provide a uniform API and make the tests more consistent.

Alternatively, you could split these into separate test functions (e.g., test_unsupported_version, test_body_missing) for better test isolation and clearer failure reporting.

Example: Adding helper methods
impl ProtocolError {
    // ... existing helpers ...

    /// Create an unsupported version error
    #[inline]
    pub fn unsupported_version(version: i32) -> Self {
        Self::UnsupportedVersion { version }
    }

    /// Create a body missing error
    #[inline]
    pub fn body_missing() -> Self {
        Self::BodyMissing
    }

    /// Create a decode error
    #[inline]
    pub fn decode_error(ext_fields_len: usize, header_len: usize) -> Self {
        Self::DecodeError { ext_fields_len, header_len }
    }

    /// Create an unsupported serialization type error
    #[inline]
    pub fn unsupported_serialization_type(serialize_type: u8) -> Self {
        Self::UnsupportedSerializationType { serialize_type }
    }
}
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b12f7a6 and f44baa5.

📒 Files selected for processing (1)
  • rocketmq-error/src/unified/protocol.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: Build & Test (windows-latest)
  • GitHub Check: Build & Test (macos-latest)
  • GitHub Check: Build & Test (ubuntu-latest)
  • GitHub Check: Code Coverage
  • GitHub Check: auto-approve
🔇 Additional comments (1)
rocketmq-error/src/unified/protocol.rs (1)

90-116: Excellent test coverage additions!

The new test cases correctly verify the error message formatting for all previously untested ProtocolError variants. Each assertion accurately matches the corresponding #[error] attribute definition.

@codecov
Copy link

codecov bot commented Jan 4, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 37.84%. Comparing base (b12f7a6) to head (f44baa5).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5445      +/-   ##
==========================================
+ Coverage   37.83%   37.84%   +0.01%     
==========================================
  Files         811      811              
  Lines      109905   109919      +14     
==========================================
+ Hits        41582    41602      +20     
+ Misses      68323    68317       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Collaborator

@rocketmq-rust-bot rocketmq-rust-bot left a comment

Choose a reason for hiding this comment

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

LGTM - All CI checks passed ✅

@mxsm mxsm merged commit b5cceec into mxsm:main Jan 5, 2026
12 of 22 checks passed
@rocketmq-rust-bot rocketmq-rust-bot added approved PR has approved and removed ready to review waiting-review waiting review this PR labels Jan 5, 2026
@WaterWhisperer WaterWhisperer deleted the test-5375 branch January 5, 2026 05:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Test🧪] Add test case for ProtocolError

4 participants