-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix: trim peer str #1115
fix: trim peer str #1115
Conversation
Hi @mindfocus, welcome to SOFAStack community, Please sign Contributor License Agreement! After you signed CLA, we will automatically sync the status of this pull request in 3 minutes. |
WalkthroughThe overall change enhances the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Configuration
participant StringUtils
Client->>Configuration: Call parse(conf)
Configuration->>StringUtils: StringUtils.trim(peerStr)
StringUtils-->>Configuration: Returns trimmed peerStr
Configuration->>Peer: peer.parse(trimmed peerStr)
Peer-->>Configuration: Returns parse result
Configuration-->>Client: Returns from parse method
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update ConfigurationTest.java
There was a problem hiding this 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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- jraft-core/src/test/java/com/alipay/sofa/jraft/conf/ConfigurationTest.java (1 hunks)
Additional comments not posted (1)
jraft-core/src/test/java/com/alipay/sofa/jraft/conf/ConfigurationTest.java (1)
44-63
: Comprehensive test for space tolerance in configuration parsingThe added test method
testToStringParseWithSpace
effectively validates the robustness of theConfiguration
class's parsing logic against leading and trailing spaces in peer strings. This is crucial for ensuring that configuration strings are parsed correctly regardless of formatting discrepancies that might occur in real-world scenarios.
- Lines 46-48, 55: The test confirms that the original and newly parsed configurations have the same size, which is a good practice for ensuring data consistency after parsing.
- Lines 49-51, 57-59: The loop and assertions verify that each
PeerId
starts with the expected string and that specific peers are correctly identified in the configuration. This is essential for validating the integrity of each peer entry.- Lines 52, 60: The
assertFalse
andassertEquals
assertions ensure that the configuration is not marked as empty and that the string representation remains consistent before and after parsing, which are good checks for data integrity.Overall, the test is well-structured and aligns with the objective of enhancing the parsing robustness by handling spaces effectively. This addition complements the existing suite of tests by covering a scenario that was previously untested, thereby improving the overall quality assurance of the
Configuration
class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
CI failed @mindfocus |
Head branch was pushed to by a user without write access
There was a problem hiding this 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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- jraft-core/src/test/java/com/alipay/sofa/jraft/conf/ConfigurationTest.java (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- jraft-core/src/test/java/com/alipay/sofa/jraft/conf/ConfigurationTest.java
@fengjiachun fixed. |
Motivation:
trim peerstr,使其更健壮?
Modification:
trim peerstr,使其更健壮?
Result:
Fixes #.
If there is no issue then describe the changes introduced by this PR.
Summary by CodeRabbit
Bug Fixes
Tests