test: add unit tests for cim-common module#188
Merged
crossoverJie merged 3 commits intoApr 5, 2026
Merged
Conversation
- Add StatusEnumTest with 13 test methods covering all enum operations - Improve HeartBeatHandlerTest with better exception handling tests - Enhance StringUtilTest with comprehensive edge case coverage - Optimize SnowflakeIdWorkerTest by removing redundant test method - Fix RouteInfoParseUtilTest naming convention All tests follow JUnit 5 standards and improve code coverage .
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughFour new JUnit 5 test classes are added to the cim-common module, providing test coverage for enums and utilities: StatusEnumTest, RouteInfoParseUtilTest, SnowflakeIdWorkerTest, and StringUtilTest, totaling 49 test methods across ~635 lines of new test code. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
cim-common/src/test/java/com/crossoverjie/cim/common/util/StringUtilTest.java (1)
273-279: 性能阈值断言容易受环境噪声影响，建议改为稳定断言。
<100ms的 wall-clock 断言在 CI 上不稳定，且不真正衡量算法复杂度。这里更适合断言“可执行且结果正确”。♻️ 建议改法（示例）
`@Test` public void testPerformanceWithLargeString() { String largeString = "a".repeat(10000); - long startTime = System.currentTimeMillis(); - StringUtil.isNullOrEmpty(largeString); - long endTime = System.currentTimeMillis(); - assertTrue((endTime - startTime) < 100, "处理大字符串应该在100ms内完成"); + assertFalse(StringUtil.isNullOrEmpty(largeString), "大字符串应判定为非空"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cim-common/src/test/java/com/crossoverjie/cim/common/util/StringUtilTest.java` around lines 273 - 279, The performance-based wall-clock assertion in testPerformanceWithLargeString is flaky; replace the timing check with a stable correctness-and-safety assertion: call StringUtil.isNullOrEmpty(largeString) inside an assertion that it does not throw (e.g., assertDoesNotThrow) and assert the returned boolean is false (or the expected value) to validate behavior for a large input rather than asserting (endTime - startTime) < 100; keep the test method name testPerformanceWithLargeString and the call to StringUtil.isNullOrEmpty to locate and update the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@cim-common/src/test/java/com/crossoverjie/cim/common/kit/HeartBeatHandlerTest.java`:
- Around line 46-49: The tests (testProcessSuccess and the repeated-call test)
never exercise the non-null channel path because mockCtx.channel() isn't
stubbed; update the tests in HeartBeatHandlerTest to stub mockCtx.channel() to
return a mock Channel (e.g., when(mockCtx.channel()).thenReturn(mockChannel)) so
the code path that uses the non-null channel (the branch asserted around Line
91) is executed, then verify interactions on that mockChannel and any side
effects of testHandler.process(mockCtx) instead of only counting calls to
mockCtx.channel().
- Around line 45-49: The tests are exercising the test-local
TestHeartBeatHandler instead of the production HeartBeatHandler, giving false
confidence; replace uses of TestHeartBeatHandler in the test methods (e.g.,
testProcessSuccess and the other methods at 67-82, 87-94) with an instance of
the real HeartBeatHandler class, call its process(mockCtx) and then assert the
expected interactions on mockCtx (e.g., verify channel() or write/flush/close
behavior) to validate module behavior; remove or repurpose the inner
TestHeartBeatHandler helper so assertions target production code paths
(reference symbols: TestHeartBeatHandler, HeartBeatHandler, testProcessSuccess,
mockCtx, verify).
- Around line 67-71: The test testProcessChannelInactive in HeartBeatHandlerTest
incorrectly mocks mockCtx.channel() to return null; instead create a mock
Channel (e.g., mockChannel), stub
when(mockCtx.channel()).thenReturn(mockChannel) and
when(mockChannel.isActive()).thenReturn(false) to represent an inactive channel,
then call testHandler.process(mockCtx) and verify interactions (e.g., that
mockCtx.channel() was called and any expected close/cleanup methods on
mockChannel or mockCtx were invoked); update assertions to match behavior when
channel is non-null but inactive.
In
`@cim-common/src/test/java/com/crossoverjie/cim/common/metastore/ZKMetaStoreImplTest.java`:
- Around line 3-4: The test file ZKMetaStoreImplTest is empty and should either
be removed or replaced with real unit tests for ZKMetaStoreImpl; implement a
JUnit 5 + Mockito test class that mocks ZK/ZooKeeper dependencies and exercises
ZKMetaStoreImpl's key behaviors (happy path, exception path, and edge cases) —
e.g., test methods for registerNode/registerService (success and when the
underlying client throws), getNode/getService (existing vs missing), and
close/cleanup behavior; use `@ExtendWith`(MockitoExtension.class), `@InjectMocks`
for ZKMetaStoreImpl, `@Mock` for the ZooKeeper/Curator client and any serializer,
verify interactions and expected exceptions, and assert returned values, or
alternatively delete the empty ZKMetaStoreImplTest class if you don't want to
add tests now.
In
`@cim-common/src/test/java/com/crossoverjie/cim/common/util/RouteInfoParseUtilTest.java`:
- Around line 51-55: The test testParseToSpecialCharacters only verifies
RouteInfoParseUtil.parse(RouteInfo) for an IPv6 address but doesn't exercise
parse(String) round-trip; update the test to either (A) assert the current
behavior by calling RouteInfoParseUtil.parse(result) and expecting a
failure/exception (explicitly assert the exception from parse(String)) or (B)
change/fix the implementation so that parse(String) can round-trip IPv6 (modify
RouteInfoParseUtil.parse(String) to handle IPv6 colons) and then assert that
RouteInfoParseUtil.parse(RouteInfo) -> String -> RouteInfo yields an equal
RouteInfo; reference the test method testParseToSpecialCharacters and the
methods RouteInfoParseUtil.parse(RouteInfo) and RouteInfoParseUtil.parse(String)
when making the change.
In
`@cim-common/src/test/java/com/crossoverjie/cim/common/util/StringUtilTest.java`:
- Around line 199-219: The two tests use weak assertions; update
testFormatLikeWhenSqlInjectionShouldHandleSafely and
testFormatLikeWhenWildcardShouldEscapeProperly to assert the exact
escaping/formatting behavior of StringUtil.formatLike instead of just
notNull/contains. Specifically, assert that the result is wrapped with '%' if
formatLike is supposed to add wildcards (e.g., startsWith("%") and
endsWith("%")), and assert how special chars are escaped according to the
implementation (for example verify single quotes are escaped/doubled or
backslash-escaped, and verify '%' and '_' are escaped as the implementation
specifies). Replace the loose assertions with exact equality or concrete
contains/notContains checks against the expected escaped output produced by
StringUtil.formatLike.
---
Nitpick comments:
In
`@cim-common/src/test/java/com/crossoverjie/cim/common/util/StringUtilTest.java`:
- Around line 273-279: The performance-based wall-clock assertion in
testPerformanceWithLargeString is flaky; replace the timing check with a stable
correctness-and-safety assertion: call StringUtil.isNullOrEmpty(largeString)
inside an assertion that it does not throw (e.g., assertDoesNotThrow) and assert
the returned boolean is false (or the expected value) to validate behavior for a
large input rather than asserting (endTime - startTime) < 100; keep the test
method name testPerformanceWithLargeString and the call to
StringUtil.isNullOrEmpty to locate and update the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c817560e-1ef5-4588-a948-798b609dc076
📒 Files selected for processing (6)
cim-common/src/test/java/com/crossoverjie/cim/common/enums/StatusEnumTest.javacim-common/src/test/java/com/crossoverjie/cim/common/kit/HeartBeatHandlerTest.javacim-common/src/test/java/com/crossoverjie/cim/common/metastore/ZKMetaStoreImplTest.javacim-common/src/test/java/com/crossoverjie/cim/common/util/RouteInfoParseUtilTest.javacim-common/src/test/java/com/crossoverjie/cim/common/util/SnowflakeIdWorkerTest.javacim-common/src/test/java/com/crossoverjie/cim/common/util/StringUtilTest.java
Comment on lines
+45
to
+49
| @Test | ||
| public void testProcessSuccess() throws Exception { | ||
| testHandler.process(mockCtx); | ||
| verify(mockCtx, times(1)).channel(); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
These tests validate a test-local implementation instead of module behavior.
All test methods assert behavior of TestHeartBeatHandler (Lines 87-94), which is defined inside the test file. This gives limited confidence about cim-common runtime behavior and can inflate coverage without protecting real code paths.
Also applies to: 67-82, 87-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@cim-common/src/test/java/com/crossoverjie/cim/common/kit/HeartBeatHandlerTest.java`
around lines 45 - 49, The tests are exercising the test-local
TestHeartBeatHandler instead of the production HeartBeatHandler, giving false
confidence; replace uses of TestHeartBeatHandler in the test methods (e.g.,
testProcessSuccess and the other methods at 67-82, 87-94) with an instance of
the real HeartBeatHandler class, call its process(mockCtx) and then assert the
expected interactions on mockCtx (e.g., verify channel() or write/flush/close
behavior) to validate module behavior; remove or repurpose the inner
TestHeartBeatHandler helper so assertions target production code paths
(reference symbols: TestHeartBeatHandler, HeartBeatHandler, testProcessSuccess,
mockCtx, verify).
Comment on lines
+46
to
+49
| public void testProcessSuccess() throws Exception { | ||
| testHandler.process(mockCtx); | ||
| verify(mockCtx, times(1)).channel(); | ||
| } |
Contributor
There was a problem hiding this comment.
“Success” and repeated-call tests do not exercise the non-null channel path.
At Line 46 and Line 77, mockCtx.channel() is never stubbed to non-null, so Line 91 is never reached. These tests currently only verify the null branch and method-call counts.
Proposed test fix
+import io.netty.channel.Channel;
@@
`@Mock`
private ChannelHandlerContext mockCtx;
+ `@Mock`
+ private Channel mockChannel;
@@
public void testProcessSuccess() throws Exception {
+ when(mockCtx.channel()).thenReturn(mockChannel);
testHandler.process(mockCtx);
- verify(mockCtx, times(1)).channel();
+ verify(mockCtx, times(2)).channel();
+ verify(mockChannel, times(1)).isActive();
}
@@
public void testProcessMultipleCalls() throws Exception {
+ when(mockCtx.channel()).thenReturn(mockChannel);
for (int i = 0; i < 5; i++) {
testHandler.process(mockCtx);
}
- verify(mockCtx, times(5)).channel();
+ verify(mockCtx, times(10)).channel();
+ verify(mockChannel, times(5)).isActive();
}Also applies to: 77-82, 90-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@cim-common/src/test/java/com/crossoverjie/cim/common/kit/HeartBeatHandlerTest.java`
around lines 46 - 49, The tests (testProcessSuccess and the repeated-call test)
never exercise the non-null channel path because mockCtx.channel() isn't
stubbed; update the tests in HeartBeatHandlerTest to stub mockCtx.channel() to
return a mock Channel (e.g., when(mockCtx.channel()).thenReturn(mockChannel)) so
the code path that uses the non-null channel (the branch asserted around Line
91) is executed, then verify interactions on that mockChannel and any side
effects of testHandler.process(mockCtx) instead of only counting calls to
mockCtx.channel().
Comment on lines
+67
to
+71
| public void testProcessChannelInactive() throws Exception { | ||
| when(mockCtx.channel()).thenReturn(null); | ||
| testHandler.process(mockCtx); | ||
| verify(mockCtx, times(1)).channel(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Netty 4.x, can ChannelHandlerContext#channel() be null, and how is an inactive channel typically represented in handlers?
💡 Result:
No, ChannelHandlerContext#channel cannot be null in Netty 4.x. It always returns the associated Channel via the pipeline, as implemented in AbstractChannelHandlerContext.channel which delegates to pipeline.channel. The Channel object itself exists for the lifetime of the ChannelHandlerContext and is never null. An inactive channel is typically represented in handlers by the channelInactive(ChannelHandlerContext ctx) event, which is fired when the channel transitions from active to inactive state (e.g., connection closed). Handlers override this method to detect and handle channel inactivity. The channel is still accessible via ctx.channel during this event, but ctx.channel.isActive will return false.
Citations:
- 1: https://netty.io/4.1/api/io/netty/channel/ChannelHandlerContext.html
- 2: https://netty.io/4.2/api/io/netty/channel/ChannelHandlerContext.html
- 3: https://netty.io/4.1/xref/io/netty/channel/AbstractChannelHandlerContext.html
- 4: channelInactive shouldn't call before channelActive netty/netty#11291
- 5: https://stackoverflow.com/questions/23859182/netty-simplechannelinboundhandler-close-channel
Test setup violates Netty 4.x contract—channel() should never return null.
The test models inactivity by mocking channel() to return null, but in Netty 4.x, ChannelHandlerContext#channel() always returns a non-null Channel instance. An inactive channel should be represented as a non-null channel with isActive() returning false. Update the test to mock a real channel object and set its inactivity state appropriately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@cim-common/src/test/java/com/crossoverjie/cim/common/kit/HeartBeatHandlerTest.java`
around lines 67 - 71, The test testProcessChannelInactive in
HeartBeatHandlerTest incorrectly mocks mockCtx.channel() to return null; instead
create a mock Channel (e.g., mockChannel), stub
when(mockCtx.channel()).thenReturn(mockChannel) and
when(mockChannel.isActive()).thenReturn(false) to represent an inactive channel,
then call testHandler.process(mockCtx) and verify interactions (e.g., that
mockCtx.channel() was called and any expected close/cleanup methods on
mockChannel or mockCtx were invoked); update assertions to match behavior when
channel is non-null but inactive.
- Update StringUtilTest to use exact assertions for formatLike behavior and stabilize performance tests. - Add verification for IPv6 parsing limitations in RouteInfoParseUtilTest. - Remove unimplemented HeartBeat and ZK tests.
Owner
|
@bingege-0729 Thanks for your contribution, please check the CI |
- Update `testFormatLikeWhenStringWithSpaces`: change expected result to `% hello %` to match the current behavior where input is not trimmed. - Update `testPrivateConstructor`: verify successful instantiation instead of expecting an exception, as the constructor is currently public.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #188 +/- ##
============================================
+ Coverage 50.34% 52.53% +2.18%
- Complexity 320 342 +22
============================================
Files 79 79
Lines 1738 1738
Branches 134 134
============================================
+ Hits 875 913 +38
+ Misses 822 783 -39
- Partials 41 42 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Owner
|
@bingege-0729 Thanks for your contribution |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
change:
This PR addresses the issue "Create some unit test for cim-common" by adding
comprehensive unit tests to improve test coverage in the cim-common module.
include:
StatusEnumTest.java: Complete unit tests for StatusEnum with 13 test methods
covering all enum values, getter methods, findStatus(), and utility methods
StringUtilTest.java: Comprehensive tests for StringUtil with 27 test methods
covering isNullOrEmpty(), isEmpty(), isNotEmpty(), and formatLike() methods
with various edge cases (null, empty, whitespace, special characters, SQL injection)
HeartBeatHandlerTest.java: Unit tests for HeartBeatHandler with Mockito,
including success, exception, and channel inactive scenarios
SnowflakeIdWorkerTest.java: Tests for SnowflakeIdWorker including basic
functionality, concurrent execution (100 threads × 1000 IDs), and monotonic
increasing verification
RouteInfoParseUtilTest.java: Tests for RouteInfoParseUtil covering port
number boundaries, IPv6 addresses, and round-trip conversion
Impact:
Related Issue: #135
Summary by CodeRabbit