fix:code check(remove redundant log)#311
Merged
chilingling merged 1 commit intoopentiny:developfrom Apr 30, 2026
Merged
Conversation
WalkthroughTest utility refactoring: removes debug console logging from an encryption test and updates another test to use locale-independent lowercase conversion while removing unused imports and commented-out code. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
base/src/test/java/com/tinyengine/it/login/utils/SM3PasswordUtilTest.java (3)
401-430:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConcurrency test won’t reliably fail: worker thread assertions throw
AssertionError(not caught).In
concurrentCreateAndVerify, the thread catchesException, but assertion failures areAssertionError(extendsError), so they won’t be captured intoexceptions[0]. That can make the test “pass” even when assertions fail inside threads.Catch
Throwable(or at leastAssertionError), and store it for the main thread to assert on.Suggested change
void concurrentCreateAndVerify() throws Exception { int threadCount = 10; int iterationsPerThread = 100; - Exception[] exceptions = new Exception[1]; + Throwable[] failures = new Throwable[1]; Runnable task = () -> { try { for (int i = 0; i < iterationsPerThread; i++) { String pwd = "pwd" + Thread.currentThread().getId() + "_" + i; PasswordResult result = SM3PasswordUtil.createPassword(pwd); assertTrue(SM3PasswordUtil.verifyPassword(pwd, result.getPasswordHash(), result.getSalt())); } - } catch (Exception e) { - synchronized (exceptions) { - exceptions[0] = e; + } catch (Throwable t) { + synchronized (failures) { + failures[0] = t; } } }; @@ for (Thread t : threads) { t.join(); } - assertNull(exceptions[0], "并发执行时不应抛出异常"); + assertNull(failures[0], "并发执行时不应抛出异常"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/src/test/java/com/tinyengine/it/login/utils/SM3PasswordUtilTest.java` around lines 401 - 430, The concurrentCreateAndVerify test currently only catches Exception in the worker Runnable so AssertionError (and other Throwables) escape and aren’t reported; change the worker exception handling in SM3PasswordUtilTest.concurrentCreateAndVerify to catch Throwable (or at minimum AssertionError) and record it into the shared holder (replace Exception[] exceptions with an AtomicReference<Throwable> or volatile Throwable) so the main thread can assert that no throwable occurred after joining threads; ensure you still synchronize/compare-and-set when writing the shared holder to avoid lost updates.
150-155:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix misleading test naming:
sm3HashWithNullData_ThrowsNullPointerExceptionassertsIllegalArgumentException.The
@DisplayNamesays it should throwNullPointerException, but the assertion isassertThrows(IllegalArgumentException.class, ...). This is confusing for future maintenance and can hide contract drift.Either update the display name/method name to match
IllegalArgumentException, or adjust the expected exception type to what the implementation actually throws.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/src/test/java/com/tinyengine/it/login/utils/SM3PasswordUtilTest.java` around lines 150 - 155, The test method sm3HashWithNullData_ThrowsNullPointerException has a mismatched expectation: the `@DisplayName` says NullPointerException but the assertion expects IllegalArgumentException; update the test to be consistent by either renaming the method/@DisplayName to reflect IllegalArgumentException or change the assertThrows to expect NullPointerException depending on the SM3PasswordUtil.sm3Hash contract — ensure SM3PasswordUtil.sm3Hash(null, "salt") is asserted against the actual thrown exception and that method/display name and message align.
16-19:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClose
MockitoAnnotations.openMocks(this)to avoid resource leakage.
MockitoAnnotations.openMocks(this)returns anAutoCloseablethat should be closed (typically in an@AfterEach). Otherwise you risk leaks / warnings and non-deterministic behavior across larger test suites.Suggested change
class SM3PasswordUtilTest { + private AutoCloseable mocks; `@BeforeEach` void setUp() throws Exception { - MockitoAnnotations.openMocks(this); + mocks = MockitoAnnotations.openMocks(this); } + + `@AfterEach` + void tearDown() throws Exception { + if (mocks != null) { + mocks.close(); + } + } ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/src/test/java/com/tinyengine/it/login/utils/SM3PasswordUtilTest.java` around lines 16 - 19, The call to MockitoAnnotations.openMocks(this) in setUp() returns an AutoCloseable that must be closed to avoid resource leaks; change the test class to store the returned AutoCloseable (e.g., a private field) when calling MockitoAnnotations.openMocks(this) in setUp(), and implement an `@AfterEach` method (e.g., tearDown()) that calls close() on that field (handling or declaring Exception as needed) to ensure the mocks are properly released; reference the setUp() method and the new tearDown() method when applying the change.
🧹 Nitpick comments (1)
base/src/test/java/com/tinyengine/it/login/utils/SM3PasswordUtilTest.java (1)
445-459: ⚡ Quick winStrengthen
sm3ConsistencyWithKnownVectors: add an actual standard expected-hash assertion.Right now this test only verifies determinism (
hash1 == hash2) forinput="abc"andsalt="". That won’t catch regressions where SM3 is implemented incorrectly but consistently.Reintroduce the published SM3 known-vector assertion for
"abc"(with whatever exact input semanticsSM3PasswordUtil.sm3Hash(data, salt)uses whensalt=""). Ifsm3Hashcombinessalt + datadifferently than “hash(data)”, the expected value must be adjusted accordingly.Suggested change
void sm3ConsistencyWithKnownVectors() throws Exception { @@ String input = "abc"; String salt = ""; String hash1 = SM3PasswordUtil.sm3Hash(input, salt); - String hash2 = SM3PasswordUtil.sm3Hash(input, salt); - assertEquals(hash1, hash2, "相同输入应得到相同哈希"); + // SM3("abc") known vector (verify SM3PasswordUtil.sm3Hash(salt="", data) semantics match this). + String expected = "66c7f0f462eeedd9d1f2d46bdc10e4e24167c4875cf2f7a2297da02b8f4ba8e"; + assertEquals(expected, hash1, "SM3 known-vector mismatch"); + + String hash2 = SM3PasswordUtil.sm3Hash(input, salt); + assertEquals(hash1, hash2, "相同输入应得到相同哈希"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/src/test/java/com/tinyengine/it/login/utils/SM3PasswordUtilTest.java` around lines 445 - 459, The test sm3ConsistencyWithKnownVectors currently only checks determinism; update it to assert against the official SM3 known-vector for input "abc" by computing expected = "<standard SM3 hex for 'abc' given the utility's salt semantics>" and replacing or adding assertEquals(expected, hash1) (use SM3PasswordUtil.sm3Hash("abc", "") as produced); if SM3PasswordUtil.sm3Hash treats salt as prefix/suffix or empty differently, compute the expected value accordingly and assert that SM3PasswordUtil.sm3Hash(input, salt) equals that expected constant to catch implementation regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@base/src/test/java/com/tinyengine/it/login/utils/SM3PasswordUtilTest.java`:
- Around line 401-430: The concurrentCreateAndVerify test currently only catches
Exception in the worker Runnable so AssertionError (and other Throwables) escape
and aren’t reported; change the worker exception handling in
SM3PasswordUtilTest.concurrentCreateAndVerify to catch Throwable (or at minimum
AssertionError) and record it into the shared holder (replace Exception[]
exceptions with an AtomicReference<Throwable> or volatile Throwable) so the main
thread can assert that no throwable occurred after joining threads; ensure you
still synchronize/compare-and-set when writing the shared holder to avoid lost
updates.
- Around line 150-155: The test method
sm3HashWithNullData_ThrowsNullPointerException has a mismatched expectation: the
`@DisplayName` says NullPointerException but the assertion expects
IllegalArgumentException; update the test to be consistent by either renaming
the method/@DisplayName to reflect IllegalArgumentException or change the
assertThrows to expect NullPointerException depending on the
SM3PasswordUtil.sm3Hash contract — ensure SM3PasswordUtil.sm3Hash(null, "salt")
is asserted against the actual thrown exception and that method/display name and
message align.
- Around line 16-19: The call to MockitoAnnotations.openMocks(this) in setUp()
returns an AutoCloseable that must be closed to avoid resource leaks; change the
test class to store the returned AutoCloseable (e.g., a private field) when
calling MockitoAnnotations.openMocks(this) in setUp(), and implement an
`@AfterEach` method (e.g., tearDown()) that calls close() on that field (handling
or declaring Exception as needed) to ensure the mocks are properly released;
reference the setUp() method and the new tearDown() method when applying the
change.
---
Nitpick comments:
In `@base/src/test/java/com/tinyengine/it/login/utils/SM3PasswordUtilTest.java`:
- Around line 445-459: The test sm3ConsistencyWithKnownVectors currently only
checks determinism; update it to assert against the official SM3 known-vector
for input "abc" by computing expected = "<standard SM3 hex for 'abc' given the
utility's salt semantics>" and replacing or adding assertEquals(expected, hash1)
(use SM3PasswordUtil.sm3Hash("abc", "") as produced); if SM3PasswordUtil.sm3Hash
treats salt as prefix/suffix or empty differently, compute the expected value
accordingly and assert that SM3PasswordUtil.sm3Hash(input, salt) equals that
expected constant to catch implementation regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ce5bd56-8ff4-43f0-a9bf-390271e74a5a
📒 Files selected for processing (2)
base/src/test/java/com/tinyengine/it/login/utils/SM2EncryptionUtilTest.javabase/src/test/java/com/tinyengine/it/login/utils/SM3PasswordUtilTest.java
💤 Files with no reviewable changes (1)
- base/src/test/java/com/tinyengine/it/login/utils/SM2EncryptionUtilTest.java
chilingling
approved these changes
Apr 30, 2026
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.
[English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
](fix:code check)
Summary by CodeRabbit
Tests