Feat/dev ut#316
Conversation
WalkthroughAdds null/input validation guards to ChangesProduction Null Guards and Visibility Change
New JUnit 5 Test Suites
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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.
Actionable comments posted: 11
🧹 Nitpick comments (1)
base/src/test/java/com/tinyengine/it/common/utils/JsonUtilsTest.java (1)
26-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the repeated string encode cases into one parameterized test.
These methods only differ by the
"string"value, so the current form adds noise without increasing coverage. Ifjunit-jupiter-paramsis already on the test classpath, one@ParameterizedTestwill keep the same assertions with much less duplication.♻️ Proposed consolidation
+import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; ... - `@Test` - void testEncode_Success_with_string2() { - Map<String, Object> data = Map.of("key", "value", "string", "2"); - String json = JsonUtils.encode(data); - assertNotNull(json); - assertTrue(json.contains("\"key\":\"value\"")); - assertTrue(json.contains("\"string\":\"2\"")); - } - `@Test` - void testEncode_Success_with_string3() { - Map<String, Object> data = Map.of("key", "value", "string", "3"); - String json = JsonUtils.encode(data); - assertNotNull(json); - assertTrue(json.contains("\"key\":\"value\"")); - assertTrue(json.contains("\"string\":\"3\"")); - } - `@Test` - void testEncode_Success_with_string4() { - Map<String, Object> data = Map.of("key", "value", "string", "4"); - String json = JsonUtils.encode(data); - assertNotNull(json); - assertTrue(json.contains("\"key\":\"value\"")); - assertTrue(json.contains("\"string\":\"4\"")); - } - `@Test` - void testEncode_Success_with_string5() { - Map<String, Object> data = Map.of("key", "value", "string", "5"); - String json = JsonUtils.encode(data); - assertNotNull(json); - assertTrue(json.contains("\"key\":\"value\"")); - assertTrue(json.contains("\"string\":\"5\"")); - } - `@Test` - void testEncode_Success_with_string() { - Map<String, Object> data = Map.of("key", "value", "string", "123"); - String json = JsonUtils.encode(data); - assertNotNull(json); - assertTrue(json.contains("\"key\":\"value\"")); - assertTrue(json.contains("\"string\":\"123\"")); - } + `@ParameterizedTest` + `@ValueSource`(strings = {"2", "3", "4", "5", "123"}) + void testEncode_Success_withStringValues(String value) { + Map<String, Object> data = Map.of("key", "value", "string", value); + String json = JsonUtils.encode(data); + assertNotNull(json); + assertTrue(json.contains("\"key\":\"value\"")); + assertTrue(json.contains("\"string\":\"" + value + "\"")); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/src/test/java/com/tinyengine/it/common/utils/JsonUtilsTest.java` around lines 26 - 90, The JsonUtilsTest class has several near-identical encode tests that only vary by the "string" input value, so consolidate them into a single parameterized test. Update the repeated methods in JsonUtilsTest to use one `@ParameterizedTest` (with a source for the different string values) while keeping the same assertions against JsonUtils.encode, and remove the redundant testEncode_Success_with_string2/3/4/5 methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@base/src/main/java/com/tinyengine/it/login/utils/JwtUtil.java`:
- Around line 258-260: The token validation in JwtUtil is only rejecting null
values, but the IllegalArgumentException message says "null or empty," so make
the behavior and message consistent. Either update the check in the token
validation path to also reject blank/empty input before parsing, or change the
exception text to mention only null; use the JwtUtil token validation logic and
its parse/validate flow as the place to adjust so the behavior matches
validateTokenReturnsFalseForEmptyToken.
In `@base/src/main/java/com/tinyengine/it/login/utils/SM3PasswordUtil.java`:
- Around line 60-62: The null-check in SM3PasswordUtil.verifyPassword currently
throws IllegalArgumentException for missing password, hash, or salt, which turns
a bad-login case into an uncaught exception. Update verifyPassword to treat null
auth inputs as a normal authentication failure by returning false, or move the
null handling to LoginController.login so omitted passwords or decrypted null
salts are mapped to the existing login-fail path instead of propagating an
exception.
In `@base/src/test/java/com/tinyengine/it/common/utils/JsonUtilsTest.java`:
- Around line 120-127: Update the JsonUtils tests to verify the app-level
exception contract exposed by JsonUtils.decode/JsonUtils.invoke rather than
Jackson’s message text. In JsonUtilsTest, replace broad
assertThrows(Exception.class) checks with the wrapped ServiceException (or the
concrete exception type thrown by JsonUtils), and avoid asserting on
parser-specific strings like “Unrecognized token”; if needed, only assert stable
application details from the wrapper. Apply the same change pattern to the other
invalid JSON test cases referenced by this suite.
In
`@base/src/test/java/com/tinyengine/it/login/service/impl/LoginServiceImplTest.java`:
- Around line 70-91: The forgotPassword tests are using non-Base64 public keys,
so they fail in getPublicKeyFromBase64 before reaching the intended
validatorPublicKey and salt-handling branches. Update
testForgotPassword_MismatchedKeys, testForgotPassword_ValidKeys, and
testForgotPassword_InvalidSalt to use real SM2 key pairs like
testValidatorPublicKey_ValidKeys via generateSM2KeyPair(), and adjust the
assertions so each test matches its scenario and exercises the correct branch in
LoginServiceImpl.forgotPassword.
In `@base/src/test/java/com/tinyengine/it/login/utils/JwtUtilTest.java`:
- Around line 255-326: The tampered-token tests in JwtUtilTest are currently
only exercising the blacklist short-circuit because validateToken returns false
as soon as tokenBlacklistService.isTokenBlacklisted(...) is true. Update
validateTokenReturnsFalseForTokenWithMissingClaims,
validateTokenHandlesTokenWithInvalidExpirationDate,
validateTokenHandlesTokenWithNullClaims,
validateTokenHandlesTokenWithEmptyClaims, and
validateTokenHandlesTokenWithInvalidAlgorithm to stub isTokenBlacklisted(...) as
false, then build truly malformed, expired, or wrong-algorithm JWTs instead of
using token.replace(...) on encoded tokens. Use the existing createExpiredToken
pattern or similar helpers so JwtUtil.validateToken reaches the parsing and
claims-validation branches.
- Around line 60-65: The
`validateSecretConfigurationThrowsExceptionForInvalidSecret` test is exercising
the wrong path because `resolveSecret(...)` reads from the environment, not
`System.setProperty`. Update `JwtUtilTest` so the secret source is actually
controllable in the test (either by supplying a real short secret through a test
seam or by mocking the resolution path), and keep the assertion focused on
`jwtUtil.validateSecretConfiguration` throwing for an invalid-length secret
rather than a missing one.
In `@base/src/test/java/com/tinyengine/it/login/utils/SM2EncryptionUtilTest.java`:
- Around line 228-239: The SM2EncryptionUtilTest case is misleading because
encrypt/decrypt currently operate on UTF-8 Strings, not arbitrary raw bytes.
Update the test method names and assertions in SM2EncryptionUtilTest to reflect
the actual String round-trip contract, or if binary coverage is required, add
tests against a byte[]-based API instead of constructing the input via
ISO-8859-1 and claiming non-UTF-8 support. Keep the focus on the
SM2EncryptionUtil.encrypt and SM2EncryptionUtil.decrypt behavior rather than
binary payload preservation.
- Around line 140-145: The SM2EncryptionUtilTest cases are using
assertThrows(Exception.class), which is too broad and can mask unrelated
regressions. Update decryptWithInvalidCipherText and the other affected tests to
assert the specific exception type expected from SM2EncryptionUtil.decrypt or
the relevant setup path, so each test only passes on the intended failure
boundary and not on any generic exception.
In `@base/src/test/java/com/tinyengine/it/login/utils/SM3PasswordUtilTest.java`:
- Around line 165-176: The null-parameter test in SM3PasswordUtilTest is too
broad because it uses assertThrows(Exception.class), which can pass for
unrelated failures instead of the intended guard. Update the three assertions in
verifyPasswordWithNullParams_ThrowsException to expect IllegalArgumentException
specifically, so the test verifies the new null-check contract in
SM3PasswordUtil.verifyPassword rather than any hashing-related exception.
- Around line 407-418: The worker task in SM3PasswordUtilTest currently only
catches Exception, so AssertionError from assertTrue in the Runnable can be lost
and the main test thread never sees it. Update the thread-handling logic around
the task to propagate any worker failure back to the test thread after join,
either by catching Throwable inside the Runnable and storing it in exceptions or
by switching to a Future-based approach and rethrowing the failure in the test
method.
- Around line 265-275: The performance tests in SM3PasswordUtilTest are too
brittle for the default suite, so move the heavy checks out of regular execution
or relax them to smoke-level assertions. Update the perf-focused methods such as
performance_BatchCreateAndVerify and the 10 MB hashing test to run only under a
dedicated perf tag/profile, or replace the strict `@Timeout` bounds with looser,
CI-safe limits while keeping the functional assertions.
---
Nitpick comments:
In `@base/src/test/java/com/tinyengine/it/common/utils/JsonUtilsTest.java`:
- Around line 26-90: The JsonUtilsTest class has several near-identical encode
tests that only vary by the "string" input value, so consolidate them into a
single parameterized test. Update the repeated methods in JsonUtilsTest to use
one `@ParameterizedTest` (with a source for the different string values) while
keeping the same assertions against JsonUtils.encode, and remove the redundant
testEncode_Success_with_string2/3/4/5 methods.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3cf0f30d-5627-44c6-8af8-65c8e4abcee6
📒 Files selected for processing (8)
base/src/main/java/com/tinyengine/it/login/service/impl/LoginServiceImpl.javabase/src/main/java/com/tinyengine/it/login/utils/JwtUtil.javabase/src/main/java/com/tinyengine/it/login/utils/SM3PasswordUtil.javabase/src/test/java/com/tinyengine/it/common/utils/JsonUtilsTest.javabase/src/test/java/com/tinyengine/it/login/service/impl/LoginServiceImplTest.javabase/src/test/java/com/tinyengine/it/login/utils/JwtUtilTest.javabase/src/test/java/com/tinyengine/it/login/utils/SM2EncryptionUtilTest.javabase/src/test/java/com/tinyengine/it/login/utils/SM3PasswordUtilTest.java
| if (token == null) { | ||
| throw new IllegalArgumentException("Token must not be null or empty"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Message claims "null or empty" but only null is rejected.
An empty/blank token falls through to parsing and returns false (covered by validateTokenReturnsFalseForEmptyToken). Either align the message with the actual check or also reject blank tokens.
🩹 Align message with the check
- if (token == null) {
- throw new IllegalArgumentException("Token must not be null or empty");
- }
+ if (token == null) {
+ throw new IllegalArgumentException("Token must not be null");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (token == null) { | |
| throw new IllegalArgumentException("Token must not be null or empty"); | |
| } | |
| if (token == null) { | |
| throw new IllegalArgumentException("Token must not be null"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@base/src/main/java/com/tinyengine/it/login/utils/JwtUtil.java` around lines
258 - 260, The token validation in JwtUtil is only rejecting null values, but
the IllegalArgumentException message says "null or empty," so make the behavior
and message consistent. Either update the check in the token validation path to
also reject blank/empty input before parsing, or change the exception text to
mention only null; use the JwtUtil token validation logic and its parse/validate
flow as the place to adjust so the behavior matches
validateTokenReturnsFalseForEmptyToken.
| if(inputPassword == null || storedHash == null || salt == null) { | ||
| throw new IllegalArgumentException("输入密码、存储哈希和盐值不能为空"); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don't surface bad-login inputs as uncaught exceptions.
Line 60 changes verifyPassword from a normal auth mismatch path into an exception path for null inputs. LoginController.login passes the request password and decrypted salt straight into this method, so an omitted password or null decrypted salt now escapes the login flow instead of returning the usual auth failure result. Please keep null-auth cases mapped at the boundary, or return false here for auth-style inputs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@base/src/main/java/com/tinyengine/it/login/utils/SM3PasswordUtil.java` around
lines 60 - 62, The null-check in SM3PasswordUtil.verifyPassword currently throws
IllegalArgumentException for missing password, hash, or salt, which turns a
bad-login case into an uncaught exception. Update verifyPassword to treat null
auth inputs as a normal authentication failure by returning false, or move the
null handling to LoginController.login so omitted passwords or decrypted null
salts are mapped to the existing login-fail path instead of propagating an
exception.
| @Test | ||
| void testDecode_InvalidJson() { | ||
| // Arrange | ||
| String invalidJson = "{key:value}"; | ||
|
|
||
| // Act & Assert | ||
| Exception exception = assertThrows(Exception.class, () -> JsonUtils.decode(invalidJson, Map.class)); | ||
| assertFalse(exception.getMessage().contains("Unrecognized token")); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the JsonUtils exception contract, not Jackson message text.
JsonUtils.invoke in base/src/main/java/com/tinyengine/it/common/utils/JsonUtils.java Lines 67-74 wraps decode failures in ServiceException. These tests currently accept any Exception and couple themselves to parser-specific message wording, which makes them weaker and brittle across mapper upgrades. Assert the wrapped exception type instead, and only check stable app-level details if needed.
Also applies to: 224-231, 377-385, 567-575
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@base/src/test/java/com/tinyengine/it/common/utils/JsonUtilsTest.java` around
lines 120 - 127, Update the JsonUtils tests to verify the app-level exception
contract exposed by JsonUtils.decode/JsonUtils.invoke rather than Jackson’s
message text. In JsonUtilsTest, replace broad assertThrows(Exception.class)
checks with the wrapped ServiceException (or the concrete exception type thrown
by JsonUtils), and avoid asserting on parser-specific strings like “Unrecognized
token”; if needed, only assert stable application details from the wrapper.
Apply the same change pattern to the other invalid JSON test cases referenced by
this suite.
| @Test | ||
| void testForgotPassword_MismatchedKeys() throws Exception { | ||
| // Arrange | ||
| User user = new User(); | ||
| user.setUsername("testUser"); | ||
| user.setPublicKey("testPublicKey"); | ||
| user.setSalt("testSalt"); | ||
|
|
||
| User existingUser = new User(); | ||
| existingUser.setPrivateKey(Base64.getEncoder().encodeToString(new byte[16])); | ||
| existingUser.setSalt("encryptedSalt"); | ||
|
|
||
| when(userMapper.queryUserByCondition(any())).thenReturn(List.of(existingUser)); | ||
|
|
||
| // Act | ||
| Result result = loginService.forgotPassword(user); | ||
|
|
||
| // Assert | ||
| assertNotNull(result); | ||
| assertFalse(result.isSuccess()); | ||
| assertEquals(ExceptionEnum.CM344.getResultCode(), result.getCode()); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Several forgotPassword tests don't exercise the branch their names imply.
testForgotPassword_MismatchedKeys, testForgotPassword_ValidKeys, and testForgotPassword_InvalidSalt all pass non-Base64 public keys ("testPublicKey" / "validPublicKey"). getPublicKeyFromBase64(...) throws and forgotPassword returns CM344 immediately, so execution never reaches validatorPublicKey or the salt-encryption logic. In particular testForgotPassword_ValidKeys actually asserts failure (assertFalse(result.isSuccess())), contradicting its name.
Use real SM2 key pairs (as testValidatorPublicKey_ValidKeys does via generateSM2KeyPair()) so the intended mismatch/valid/salt branches are covered.
Also applies to: 114-135, 182-203
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@base/src/test/java/com/tinyengine/it/login/service/impl/LoginServiceImplTest.java`
around lines 70 - 91, The forgotPassword tests are using non-Base64 public keys,
so they fail in getPublicKeyFromBase64 before reaching the intended
validatorPublicKey and salt-handling branches. Update
testForgotPassword_MismatchedKeys, testForgotPassword_ValidKeys, and
testForgotPassword_InvalidSalt to use real SM2 key pairs like
testValidatorPublicKey_ValidKeys via generateSM2KeyPair(), and adjust the
assertions so each test matches its scenario and exercises the correct branch in
LoginServiceImpl.forgotPassword.
| @Test | ||
| void validateSecretConfigurationThrowsExceptionForInvalidSecret() { | ||
| when(environment.getActiveProfiles()).thenReturn(new String[]{"prod"}); | ||
| System.setProperty("SECRET_STRING", "short"); | ||
| assertThrows(IllegalStateException.class, jwtUtil::validateSecretConfiguration); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first
git ls-files 'base/src/test/java/com/tinyengine/it/login/utils/JwtUtilTest.java' \
'base/src/main/java/com/tinyengine/it/login/utils/JwtUtil.java' \
'base/src/main/java/com/tinyengine/it/login/utils/*' \
'base/src/test/java/com/tinyengine/it/login/utils/*'
# Show compact structure for the target files
ast-grep outline base/src/test/java/com/tinyengine/it/login/utils/JwtUtilTest.java --view expanded || true
ast-grep outline base/src/main/java/com/tinyengine/it/login/utils/JwtUtil.java --view expanded || true
# Read the relevant sections with line numbers
printf '\n=== JwtUtilTest.java ===\n'
sed -n '1,220p' base/src/test/java/com/tinyengine/it/login/utils/JwtUtilTest.java | cat -n
printf '\n=== JwtUtil.java ===\n'
sed -n '1,260p' base/src/main/java/com/tinyengine/it/login/utils/JwtUtil.java | cat -nRepository: opentiny/tiny-engine-backend-java
Length of output: 21434
validateSecretConfigurationThrowsExceptionForInvalidSecret is testing the wrong branch
System.setProperty("SECRET_STRING", "short")is ignored here becauseresolveSecret(...)readsSystem.getenv(SECRET_STRING), so this test currently passes when the secret is missing, not when it is too short.- Remove the system property override or add a test seam that can supply an actual short secret; otherwise the test name and coverage do not match.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@base/src/test/java/com/tinyengine/it/login/utils/JwtUtilTest.java` around
lines 60 - 65, The `validateSecretConfigurationThrowsExceptionForInvalidSecret`
test is exercising the wrong path because `resolveSecret(...)` reads from the
environment, not `System.setProperty`. Update `JwtUtilTest` so the secret source
is actually controllable in the test (either by supplying a real short secret
through a test seam or by mocking the resolution path), and keep the assertion
focused on `jwtUtil.validateSecretConfiguration` throwing for an invalid-length
secret rather than a missing one.
| void decryptWithInvalidCipherText() { | ||
| String invalidCipherText = "invalidCipherText"; | ||
| PrivateKey privateKey = keyPair.getPrivate(); | ||
|
|
||
| assertThrows(Exception.class, () -> SM2EncryptionUtil.decrypt(invalidCipherText, privateKey), | ||
| "Decrypting invalid cipher text should throw an exception"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don't let assertThrows(Exception.class) hide unrelated failures.
These tests pass on any checked exception, including regressions outside the scenario under test. Please assert the specific failure boundary for each case instead of Exception.class, so an unrelated parsing/setup bug does not make the test go green.
Also applies to: 201-210, 242-253
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@base/src/test/java/com/tinyengine/it/login/utils/SM2EncryptionUtilTest.java`
around lines 140 - 145, The SM2EncryptionUtilTest cases are using
assertThrows(Exception.class), which is too broad and can mask unrelated
regressions. Update decryptWithInvalidCipherText and the other affected tests to
assert the specific exception type expected from SM2EncryptionUtil.decrypt or
the relevant setup path, so each test only passes on the intended failure
boundary and not on any generic exception.
| void encryptAndDecryptWithNonUTF8Characters() throws Exception { | ||
| String plainText = new String(new byte[]{(byte) 0xC3, (byte) 0x28}, "ISO-8859-1"); | ||
| PublicKey publicKey = keyPair.getPublic(); | ||
| PrivateKey privateKey = keyPair.getPrivate(); | ||
|
|
||
| String encryptedText = SM2EncryptionUtil.encrypt(plainText, publicKey); | ||
| assertNotNull(encryptedText, "Encrypted text should not be null"); | ||
|
|
||
| String decryptedText = SM2EncryptionUtil.decrypt(encryptedText, privateKey); | ||
| assertNotNull(decryptedText, "Decrypted text should not be null"); | ||
| assertEquals(plainText, decryptedText, "Decrypted text should match the original plain text"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
These cases are not actually testing raw non-UTF-8 or binary payload support.
SM2EncryptionUtil.encrypt always converts the input String to UTF-8 bytes, and decrypt reconstructs a String from UTF-8 bytes again (base/src/main/java/com/tinyengine/it/login/utils/SM2EncryptionUtil.java:62-94). Converting bytes to a String first means these assertions only prove character round-trips, not preservation of arbitrary byte sequences. Either rename the tests to match the current contract, or move true binary coverage behind a byte[] API.
Also applies to: 257-268
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@base/src/test/java/com/tinyengine/it/login/utils/SM2EncryptionUtilTest.java`
around lines 228 - 239, The SM2EncryptionUtilTest case is misleading because
encrypt/decrypt currently operate on UTF-8 Strings, not arbitrary raw bytes.
Update the test method names and assertions in SM2EncryptionUtilTest to reflect
the actual String round-trip contract, or if binary coverage is required, add
tests against a byte[]-based API instead of constructing the input via
ISO-8859-1 and claiming non-UTF-8 support. Keep the focus on the
SM2EncryptionUtil.encrypt and SM2EncryptionUtil.decrypt behavior rather than
binary payload preservation.
| @DisplayName("verifyPassword 传入 null 参数应抛出异常") | ||
| void verifyPasswordWithNullParams_ThrowsException() { | ||
| // 先创建一个合法结果 | ||
| assertThrows(Exception.class, () -> { | ||
| SM3PasswordUtil.verifyPassword(null, "hash", "salt"); | ||
| }); | ||
| assertThrows(Exception.class, () -> { | ||
| SM3PasswordUtil.verifyPassword("pwd", null, "salt"); | ||
| }); | ||
| assertThrows(Exception.class, () -> { | ||
| SM3PasswordUtil.verifyPassword("pwd", "hash", null); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the new contract, not any exception.
These cases are exercising the new null guard, so assertThrows(Exception.class, ...) will still pass on unrelated failures from the hashing path. Please narrow all three assertions to IllegalArgumentException so the test actually protects the intended behavior.
Suggested fix
- assertThrows(Exception.class, () -> {
+ assertThrows(IllegalArgumentException.class, () -> {
SM3PasswordUtil.verifyPassword(null, "hash", "salt");
});
- assertThrows(Exception.class, () -> {
+ assertThrows(IllegalArgumentException.class, () -> {
SM3PasswordUtil.verifyPassword("pwd", null, "salt");
});
- assertThrows(Exception.class, () -> {
+ assertThrows(IllegalArgumentException.class, () -> {
SM3PasswordUtil.verifyPassword("pwd", "hash", null);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @DisplayName("verifyPassword 传入 null 参数应抛出异常") | |
| void verifyPasswordWithNullParams_ThrowsException() { | |
| // 先创建一个合法结果 | |
| assertThrows(Exception.class, () -> { | |
| SM3PasswordUtil.verifyPassword(null, "hash", "salt"); | |
| }); | |
| assertThrows(Exception.class, () -> { | |
| SM3PasswordUtil.verifyPassword("pwd", null, "salt"); | |
| }); | |
| assertThrows(Exception.class, () -> { | |
| SM3PasswordUtil.verifyPassword("pwd", "hash", null); | |
| }); | |
| `@DisplayName`("verifyPassword 传入 null 参数应抛出异常") | |
| void verifyPasswordWithNullParams_ThrowsException() { | |
| // 先创建一个合法结果 | |
| assertThrows(IllegalArgumentException.class, () -> { | |
| SM3PasswordUtil.verifyPassword(null, "hash", "salt"); | |
| }); | |
| assertThrows(IllegalArgumentException.class, () -> { | |
| SM3PasswordUtil.verifyPassword("pwd", null, "salt"); | |
| }); | |
| assertThrows(IllegalArgumentException.class, () -> { | |
| SM3PasswordUtil.verifyPassword("pwd", "hash", null); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@base/src/test/java/com/tinyengine/it/login/utils/SM3PasswordUtilTest.java`
around lines 165 - 176, The null-parameter test in SM3PasswordUtilTest is too
broad because it uses assertThrows(Exception.class), which can pass for
unrelated failures instead of the intended guard. Update the three assertions in
verifyPasswordWithNullParams_ThrowsException to expect IllegalArgumentException
specifically, so the test verifies the new null-check contract in
SM3PasswordUtil.verifyPassword rather than any hashing-related exception.
| @Test | ||
| @DisplayName("批量创建并验证 1000 个密码应在 2 秒内完成") | ||
| @Timeout(2) | ||
| void performance_BatchCreateAndVerify() throws Exception { | ||
| int iterations = 1000; | ||
| for (int i = 0; i < iterations; i++) { | ||
| String plain = "user" + i + "P@ss"; | ||
| PasswordResult result = SM3PasswordUtil.createPassword(plain); | ||
| assertTrue(SM3PasswordUtil.verifyPassword(plain, result.getPasswordHash(), result.getSalt())); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='base/src/test/java/com/tinyengine/it/login/utils/SM3PasswordUtilTest.java'
echo '== file length =='
wc -l "$file"
echo
echo '== relevant ranges =='
sed -n '240,310p' "$file"
echo
sed -n '450,520p' "$file"
echo
echo '== timeout annotations in file =='
rg -n '`@Timeout`|`@DisplayName`' "$file"
echo
echo '== surrounding class/method names =='
python3 - <<'PY'
from pathlib import Path
path = Path('base/src/test/java/com/tinyengine/it/login/utils/SM3PasswordUtilTest.java')
for i, line in enumerate(path.read_text().splitlines(), 1):
if line.strip().startswith('`@Test`') or 'class ' in line:
print(f'{i}: {line.rstrip()}')
PYRepository: opentiny/tiny-engine-backend-java
Length of output: 6257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== test annotations near performance methods =='
sed -n '350,490p' base/src/test/java/com/tinyengine/it/login/utils/SM3PasswordUtilTest.java
echo
echo '== junit/surefire config hints =='
rg -n 'junit|surefire|failsafe|timeout|Tag\(|`@Tag`|`@Disabled`|performance' -g 'pom.xml' -g '*.xml' -g '*.gradle' -g '*.properties' -g '*.yml' -g '*.yaml' .
echo
echo '== all timeout annotations in repo =='
rg -n '`@Timeout`\(' base/src/test/javaRepository: opentiny/tiny-engine-backend-java
Length of output: 5421
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== implementation outline =='
fd -a 'SM3PasswordUtil.java' .
fd -a 'PasswordResult.java' .
echo
echo '== read implementation if found =='
for f in $(fd -a 'SM3PasswordUtil.java' . 2>/dev/null); do
echo "-- $f --"
wc -l "$f"
sed -n '1,260p' "$f"
done
echo
echo '== pom excerpts for junit/surefire/jupiter =='
rg -n 'junit-jupiter|surefire|failsafe|maven-surefire-plugin|maven-failsafe-plugin|junit' pom.xml base/pom.xml . -g 'pom.xml' -g '*.xml' -g '*.gradle' -g '*.properties' -g '*.yml' -g '*.yaml'
echo
echo '== test runtime annotations elsewhere =='
rg -n '`@Timeout`\(|`@RepeatedTest`\(|`@Execution`\(' base/src/test/java/com/tinyengine/it/login/utils/SM3PasswordUtilTest.javaRepository: opentiny/tiny-engine-backend-java
Length of output: 3828
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== pom around test deps/plugins =='
sed -n '120,260p' pom.xml
echo
echo '== all references to Timeout import in the test file =='
rg -n 'org.junit.jupiter.api.Timeout|import .*Timeout' base/src/test/java/com/tinyengine/it/login/utils/SM3PasswordUtilTest.java
echo
echo '== any tags or profile gating around performance tests =='
rg -n '`@Tag`|performance|slow|profile' base/src/test/java/com/tinyengine/it/login/utils/SM3PasswordUtilTest.java pom.xmlRepository: opentiny/tiny-engine-backend-java
Length of output: 4313
Move these perf checks out of the default suite
@Timeout(2) for 1,000 create/verify cycles and @Timeout(1) for hashing a 10 MB payload twice are brittle in CI; gate them behind a perf tag/profile or relax them to smoke-level bounds.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@base/src/test/java/com/tinyengine/it/login/utils/SM3PasswordUtilTest.java`
around lines 265 - 275, The performance tests in SM3PasswordUtilTest are too
brittle for the default suite, so move the heavy checks out of regular execution
or relax them to smoke-level assertions. Update the perf-focused methods such as
performance_BatchCreateAndVerify and the 10 MB hashing test to run only under a
dedicated perf tag/profile, or replace the strict `@Timeout` bounds with looser,
CI-safe limits while keeping the functional assertions.
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='base/src/test/java/com/tinyengine/it/login/utils/SM3PasswordUtilTest.java'
wc -l "$file"
sed -n '360,460p' "$file" | cat -nRepository: opentiny/tiny-engine-backend-java
Length of output: 4398
🌐 Web query:
JUnit 5 uncaught exception in spawned thread assertion failure does not fail test
💡 Result:
JUnit Jupiter (JUnit 5) only detects exceptions that occur within the thread executing the test method [1][2][3]. When code running in a spawned thread throws an uncaught exception, JUnit 5 is unaware of it, meaning the test will not fail automatically and assertions like assertThrows will not catch it [4][1][3]. To handle this, you must explicitly communicate the outcome from the background thread to the main test thread. You can achieve this using the following approaches: 1. Synchronize and Propagate (Manual Approach): Use a thread-safe mechanism to capture the exception in the background thread and rethrow it in the main test thread [1][3]. - Create a shared variable (e.g., an AtomicReference or a volatile field) to store the exception [1]. - Ensure the main thread waits for the background thread to finish (e.g., using Thread.join or a CountDownLatch) [1][5]. - Check the shared variable after the background thread joins and throw the captured exception if present [1]. 2. Use an Extension: You can use existing libraries or create a custom JUnit 5 extension that captures uncaught exceptions across all threads and reports them as test failures [5]. These extensions typically hook into the test lifecycle to inspect exceptions that propagate to the Thread's UncaughtExceptionHandler [5]. 3. Refactor for Unit Testability: Often, testing exceptions in spawned threads indicates that the logic is difficult to test in isolation [6]. Consider refactoring the code so the logic inside the Runnable or task can be executed directly within the test method, allowing standard JUnit 5 assertions like assertThrows to function correctly [6]. Avoid relying on assertTimeoutPreemptively for error testing, as it executes code in a separate thread and carries risks regarding ThreadLocal state (such as breaking transactional tests) [7][8][9].
Citations:
- 1: https://stackoverflow.com/questions/2596493/junit-assert-in-thread-throws-exception
- 2: https://github.com/junit-team/junit-framework/blob/r6.1.1/documentation/modules/ROOT/pages/writing-tests/exception-handling.adoc
- 3: https://tarikbilla.com/junit-assert-in-thread-throws-exception/
- 4: https://stackoverflow.com/questions/73241137/junit-5-assertthrows-not-catching-exception-on-seperate-thread
- 5: https://github.com/sahabpardaz/uncaught-exception-rule
- 6: https://stackoverflow.com/questions/62995202/junit5-check-exception-is-thrown-in-thread
- 7: https://docs.junit.org/5.14.2/writing-tests/assertions.html
- 8: https://docs.junit.org/5.13.0/api/org.junit.jupiter.api/org/junit/jupiter/api/Assertions.html
- 9: Assertions.assertTimeoutPreemptively() breaks Spring test transaction rollback junit-team/junit-framework#1701
Propagate worker-thread assertion failures
assertTrue(...) throws AssertionError, which is not caught by catch (Exception). If verifyPassword(...) starts returning false, the worker thread can die and this test can still pass because nothing is rethrown on the main thread after join(). Catch Throwable or propagate the result through Future and rethrow it in the test thread.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@base/src/test/java/com/tinyengine/it/login/utils/SM3PasswordUtilTest.java`
around lines 407 - 418, The worker task in SM3PasswordUtilTest currently only
catches Exception, so AssertionError from assertTrue in the Runnable can be lost
and the main test thread never sees it. Update the thread-handling logic around
the task to propagate any worker failure back to the test thread after join,
either by catching Throwable inside the Runnable and storing it in exceptions or
by switching to a Future-based approach and rethrowing the failure in the test
method.
1 participant
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
Summary by CodeRabbit
Bug Fixes
Tests