MOSIP-30573: test case coverage #474
Conversation
Signed-off-by: nagendra0721 <[email protected]>
WalkthroughAdds multiple new and expanded unit/integration test classes in kernel-keymanager-service covering client crypto, crypto core, Argon2 hashing, symmetric key conversion, and key generation; changes are test-only and do not modify production code or public APIs. Changes
Sequence Diagram(s)(Skipped — changes are test additions and do not introduce new control-flow behavior requiring sequence diagrams.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 2
🧹 Nitpick comments (6)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/cryptomanager/test/util/KeymanagerSymmetricKeyConverterTest.java (1)
60-73: Clarify test intent and tighten timestamp assertion
- In
testConvert_PreservesExistingDestinationValues(Lines 75–84) the assertions verify that destination fields are overwritten with source values, which contradicts the method name (“Preserves…”). Consider renaming to something liketestConvert_OverridesDestinationValues(or adjusting assertions if the intended behavior is to preserve existing values).- In
testConvert_WithEmptyStrings(Lines 60–73) you only assert thattimeStampis non-null while usingLocalDateTime.now()on the source. If you want stronger guarantees, you could mirror the pattern fromsetUp()and use a fixedLocalDateTimefor that test as well, then assert equality rather than just non-null.Also applies to: 75-84
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/cryptomanager/test/service/Argon2HashServiceTest.java (1)
125-191: Service-level tests currently only exercise Mockito stubbing, not real service logicThe tests
testGenerateArgon2Hash_Service_Success,testGenerateArgon2Hash_Service_WithProvidedSalt,testGenerateArgon2Hash_Service_InvalidInput,testGenerateArgon2Hash_Service_NullInput, andtestGenerateArgon2Hash_Service_EmptyInputoperate entirely on the mockedcryptomanagerService: they stubgenerateArgon2Hash(...)on the mock and then call the same mock, asserting the stubbed outcome or expected exception.This gives coverage for the test class and Mockito interactions, but it doesn’t exercise
CryptomanagerServiceImplor any real Argon2/validation behavior. If the goal is to increase functional coverage:
- Prefer constructing a real
CryptomanagerServiceImplwith its dependencies (similar to what you did intestGenerateArgon2Hash_RealService_ValidationError) and invoking that.- Or, if you only intend to verify controller–service wiring, keep the controller tests and consider dropping the direct “Service_*” mock tests to avoid a false sense of coverage.
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keygenerator/bouncycastle/test/KeyGeneratorTest.java (1)
33-43: Reduce redundancy and be conscious of reflection coupling to KeyGenerator internalsThe new tests generally look correct and useful (especially the
buildPrivateKeyandgetSecureRandomscenarios), but there are a couple of maintainability points:
testGetSymmetricKeyvsgetSymmetricKeyTest, andtestGetAsymmetricKeyvsgetAsymmetricKeyTest, are largely overlapping. You could merge each pair into a single test that asserts both type and basic properties (non-null, non-empty key material) to keep the suite leaner.- The SecureRandom tests rely on private fields
secureRandom,rngProviderEnabled, andrngProviderNameviaReflectionTestUtils. That’s reasonable for coverage, but it does tightly couple the tests to the current field names and implementation. If those internals change, these tests will silently break at compile-time or via reflection. Consider adding at least one higher-level test that only uses the public API but would fail if the RNG configuration behavior regressed.Also applies to: 44-77, 78-108
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/clientcrypto/test/service/ClientCryptoManagerServiceTest.java (3)
110-146: Instance-level Android service tests currently lock in placeholder behaviorTests such as:
testAndroidAsymmetricEncrypt_InstancetestAndroidSignDatatestAndroidGetSigningPublicParttestAndroidGetEncryptionPublicPartassert that the instance methods return zero-length arrays (and
isTPMInstance()isfalse), which matches the current stubbed implementation (new byte[0],false).If the plan is to later implement these instance methods with real crypto behavior, these tests will start failing and will then need to be completely rewritten. You may want to:
- Either update the implementation now and assert the desired “real” behavior (non-empty signature/ciphertext, etc.), or
- Clearly document in test names/comments that these are temporary stubs meant only to guard against regression to
null/exceptions, so they’re easier to revisit when the implementation matures.
172-190: Broadtry/catchblocks with minimal assertions weaken several testsIn multiple tests you wrap the core call in a broad
try { ... } catch (Exception e) { assertNotNull(e); }, e.g.:
testFacadeValidateSignature_NullClientTypetestFacadeValidateSignature_DefaultMethodtestFacadeEncrypt_DefaultMethodtestManagerServiceVerifytestLocalClientCryptoService_ReflectiontestTPMClientCryptoService_ReflectionThis pattern means the test passes whether the call returns a value or throws any exception; you’re effectively only asserting “something happened”.
For more robust tests:
- Decide whether each path is expected to succeed or to throw a specific exception and encode that explicitly via
@Test(expected = ...)or explicitfail(...)+ specific catch.- For reflection-based tests, if the class/method is expected to exist, let unexpected reflection failures fail the test instead of swallowing them.
This will make these tests much more effective at detecting real regressions in facade/manager wiring and reflection-based instantiation.
Also applies to: 198-206, 237-251, 309-339
221-305: Manager service tests could assert more on behavior, not just non-null responsesThe
ClientCryptoManagerServiceImpltests (testManagerServiceSign,testManagerServiceEncrypt,testManagerServiceDecrypt,testManagerServiceGetSigningPublicKey,testManagerServiceGetEncPublicKey) correctly wire in a mockedClientCryptoServicevia theClientCryptoFacadefield and assert that responses are non-null.To strengthen these without much extra effort, consider also asserting:
- That the expected mocked methods were invoked with the decoded values (e.g., using Mockito
verifyonclientCryptoService.signData(...),asymmetricDecrypt(...), etc.).- That the DTOs returned by the manager service contain the expected Base64-encoded representations of the mocked outputs.
This would validate that the manager is applying the correct Base64 transformations and delegating to the facade/service as intended, rather than only asserting “no exception and some object returned”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/clientcrypto/test/service/ClientCryptoManagerServiceTest.java(1 hunks)kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/crypto/jce/test/CryptoCoreTest.java(5 hunks)kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/cryptomanager/test/service/Argon2HashServiceTest.java(1 hunks)kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/cryptomanager/test/util/KeymanagerSymmetricKeyConverterTest.java(1 hunks)kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keygenerator/bouncycastle/test/KeyGeneratorTest.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/cryptomanager/test/util/KeymanagerSymmetricKeyConverterTest.java (1)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/util/KeymanagerSymmetricKeyConverter.java (1)
KeymanagerSymmetricKeyConverter(15-31)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/cryptomanager/test/service/Argon2HashServiceTest.java (1)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/exception/CryptoManagerSerivceException.java (1)
CryptoManagerSerivceException(11-39)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/crypto/jce/test/CryptoCoreTest.java (1)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanagerservice/test/util/KeymanagerUtilTest.java (1)
SpringBootTest(38-340)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/clientcrypto/test/service/ClientCryptoManagerServiceTest.java (2)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/clientcrypto/exception/ClientCryptoException.java (1)
ClientCryptoException(10-35)kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/clientcrypto/service/impl/AndroidClientCryptoServiceImpl.java (1)
AndroidClientCryptoServiceImpl(20-105)
🔇 Additional comments (1)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/cryptomanager/test/service/Argon2HashServiceTest.java (1)
261-310: Good use of reflection to cover private helpers and validation pathThe tests
testGetLongBytes_PrivateMethod,testGetSaltBytes_PrivateMethod_WithValidKey, andtestGenerateArgon2Hash_RealService_ValidationErrorare a reasonable compromise to get coverage on internal helpers and the validation hook inCryptomanagerServiceImplwithout over-complicating the setup.One thing to keep in mind is that these tests assume:
- The private method signatures (
getLongBytes(long),getSaltBytes(byte[], SecretKey)) stay stable.- The field name
cryptomanagerUtilremains unchanged.If those internals are likely to evolve, consider adding at least one public API–level test that would fail if the validation logic stopped being invoked, so you’re not relying solely on reflection to catch regressions.
.../src/test/java/io/mosip/kernel/clientcrypto/test/service/ClientCryptoManagerServiceTest.java
Show resolved
Hide resolved
.../kernel-keymanager-service/src/test/java/io/mosip/kernel/crypto/jce/test/CryptoCoreTest.java
Show resolved
Hide resolved
Signed-off-by: nagendra0721 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/clientcrypto/test/service/ClientCryptoManagerServiceTest.java (1)
155-158:testAndroidValidateSignature_InvalidSignatureexpectation contradicts implementationThe static
AndroidClientCryptoServiceImpl.validateSignature(...)method returns the boolean result ofsign.verify(signature)and only throwsClientCryptoExceptionwhen a JCA operation fails inside thetryblock (e.g., bad key format), not for a normal signature mismatch. With the current implementation, passing"invalid".getBytes()as the signature is expected to yield a boolean result (typicallyfalse), not aClientCryptoException.The test currently uses
@Test(expected = ClientCryptoException.class), which will fail once the method behaves as designed for invalid signatures.Consider asserting on the boolean return instead of expecting an exception, for example:
- @Test(expected = ClientCryptoException.class) - public void testAndroidValidateSignature_InvalidSignature() throws Exception { - AndroidClientCryptoServiceImpl.validateSignature(testPublicKey, "invalid".getBytes(), testData); - } + @Test + public void testAndroidValidateSignature_InvalidSignature() throws Exception { + boolean result = AndroidClientCryptoServiceImpl + .validateSignature(testPublicKey, "invalid".getBytes(), testData); + assertFalse(result); + }This keeps the test aligned with the current contract of
validateSignature.
🧹 Nitpick comments (4)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/clientcrypto/test/service/ClientCryptoManagerServiceTest.java (4)
91-113: Static reset helpers are overly aggressive and may null out constants/loggersBoth
resetStaticFieldsandresetAllStaticFieldsiterate all declared static fields and blindly set booleans, ints, and reference types to default values. This has a few drawbacks:
- It may mutate static constants and loggers (including
finalfields), making later behavior harder to reason about and fragile against future changes in these classes.- Any new static added to
ClientCryptoFacadeorAndroidClientCryptoServiceImplwill be silently reset, which can mask issues or introduce spurious NPEs in tests.You can keep the intent (cleaning mutable static state) while avoiding collateral damage by skipping
finalfields and narrowing what you reset. For example:- Field[] fields = ClientCryptoFacade.class.getDeclaredFields(); + Field[] fields = ClientCryptoFacade.class.getDeclaredFields(); for (Field field : fields) { - if (java.lang.reflect.Modifier.isStatic(field.getModifiers())) { + int modifiers = field.getModifiers(); + if (java.lang.reflect.Modifier.isStatic(modifiers) + && !java.lang.reflect.Modifier.isFinal(modifiers)) { field.setAccessible(true); if (field.getType() == boolean.class) { field.set(null, false); } else if (!field.getType().isPrimitive()) { field.set(null, null); } } }and similarly in
resetAllStaticFields:- for (Field field : fields) { - if (java.lang.reflect.Modifier.isStatic(field.getModifiers())) { + for (Field field : fields) { + int modifiers = field.getModifiers(); + if (java.lang.reflect.Modifier.isStatic(modifiers) + && !java.lang.reflect.Modifier.isFinal(modifiers)) { field.setAccessible(true); if (field.getType() == boolean.class) { field.set(null, false); } else if (field.getType() == int.class) { field.set(null, 0); } else if (!field.getType().isPrimitive()) { field.set(null, null); } } }If there are only a small number of mutable static fields that truly need resetting, an explicit whitelist would be even safer.
Also applies to: 115-141
172-199: Instance-method tests for Android service correctly mirror current stub behaviorThe instance-oriented tests (
testAndroidValidateSignature_Instance,testAndroidAsymmetricEncrypt_Instance,testAndroidAsymmetricDecrypt_Instance,testAndroidSignData) are consistent with the present stubbed implementation:
validateSignatureandasymmetricDecryptare expected to fail due to missing key material, resulting inClientCryptoException.asymmetricEncryptandsignDatacurrently return empty byte arrays.These tests are useful as guardrails while the implementation is still incomplete, but will need revisiting if/when the instance methods gain full functionality (e.g., start returning real signatures/ciphertexts instead of empty arrays).
228-287: Facade tests cover multiple branches, but some assertions are weakThe
ClientCryptoFacadetests hit several important paths:
- Android client validate/signature and encrypt success cases.
- Null
ClientTypeand default-method overloads.- Random-byte generation and TPM flag toggling.
However, for methods like
testFacadeValidateSignature_NullClientType,testFacadeValidateSignature_DefaultMethod, andtestFacadeEncrypt_DefaultMethod, the pattern:try { boolean result = ...; assertFalse(result); // or assertNotNull(result); } catch (Exception e) { assertNotNull(e); }allows both “expected value” and “any exception” as success, which weakens the tests; they will pass under a wide range of behaviors.
If possible, it would be better to decide on one contract per path and assert it explicitly: either expect a specific exception type or a concrete return value, not “one of the above”. That will make these tests more meaningful as behavioral specifications rather than just coverage tools.
375-389: Mock-only tests for Local/TPM services don’t exercise real implementations
testLocalClientCryptoService_MockandtestTPMClientCryptoService_Mockonly assert behavior on Mockito mocks ofClientCryptoService(isTPMInstance()returning true/false). They don’t touchLocalClientCryptoServiceImplorTPMClientCryptoServiceImplthemselves, so they mainly verify Mockito rather than your code.If you want meaningful coverage of those implementations, consider instantiating the real classes (or a thin wrapper) where feasible, or remove these tests if they’re only present for coverage metrics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/clientcrypto/test/service/ClientCryptoManagerServiceTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/clientcrypto/test/service/ClientCryptoManagerServiceTest.java (2)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/clientcrypto/exception/ClientCryptoException.java (1)
ClientCryptoException(10-35)kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/clientcrypto/service/impl/AndroidClientCryptoServiceImpl.java (1)
AndroidClientCryptoServiceImpl(20-105)
🔇 Additional comments (5)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/clientcrypto/test/service/ClientCryptoManagerServiceTest.java (5)
52-77: Setup/teardown and reflection-based wiring look appropriate for isolated testsUsing a generated RSA keypair, constructing
ClientCryptoFacade/ClientCryptoManagerServiceImpldirectly, and injectingcryptoCoreandclientCryptoFacadevia reflection gives you good isolation from Spring wiring while keeping tests realistic. The symmetric encrypt/decrypt stubs oncryptoCoreare also straightforward and readable.Also applies to: 79-83
143-153: Valid-signature path test is clear and aligned with implementation
testAndroidValidateSignature_Staticcorrectly exercises the “happy path” using a real RSA keypair andSignatureinstance, then delegates toAndroidClientCryptoServiceImpl.validateSignature(...)and asserts the result istrue. This gives solid coverage of the static validation behavior with realistic crypto primitives.
160-170: Static asymmetric-encryption tests exercise both success and failure pathsThe two tests:
testAndroidAsymmetricEncrypt_Staticwith a real RSA public key, andtestAndroidAsymmetricEncrypt_InvalidKeywith an invalid key byte array expectingClientCryptoExceptionnicely cover the main branches of the static
asymmetricEncrypthelper (normal encryption vs. key-spec failure). Assertions on non-null/length for the success case and exception for the invalid-key case look appropriate.
200-227: Getter and lifecycle tests for Android service are lightweight but fineThe tests for
getSigningPublicPart,getEncryptionPublicPart,isTPMInstance, andcloseSecurityInstancesimply assert the current stub semantics (zero-length arrays, false TPM flag, no exception on close). This is adequate smoke coverage and provides some protection against accidental signature changes.
289-373: Manager-service tests provide good end-to-end DTO flow coverageThe tests for
ClientCryptoManagerServiceImpl(testManagerServiceSign,testManagerServiceVerify,testManagerServiceEncrypt,testManagerServiceDecrypt,testManagerServiceGetSigningPublicKey,testManagerServiceGetEncPublicKey) add valuable coverage:
- They exercise base64 encoding/decoding via the request/response DTOs.
- They verify reflection-based wiring of
clientCryptoServiceintoClientCryptoFacade.- They validate that responses are non-null under both mocked-success paths (e.g., sign/decrypt) and current default behavior.
Overall, these tests look consistent with the manager’s role as an adapter around the facade and crypto service.
Signed-off-by: nagendra0721 <[email protected]>
Signed-off-by: nagendra0721 <[email protected]>
Signed-off-by: nagendra0721 <[email protected]>
Signed-off-by: nagendra0721 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/crypto/jce/test/CryptoCoreTest.java (1)
257-280: ClarifyverifySignatureTestas a negative/key‑mismatch scenario or add a positive one‑arg verify testHere you deliberately sign with
rsaPair.getPrivate()while embedding a different X.509 certificate, socryptoCore.verifySignature(signature)returningfalseis correct for a key‑mismatch case. The current method name (verifySignatureTest) reads like a positive/happy‑path test, which can be confusing.Consider either:
- Renaming to something like
verifySignatureMismatchedCertificateReturnsFalseand adding a brief comment about the intentional key mismatch, and/or- Adding a complementary test where the signing key matches the certificate so
verifySignature(String)returnstrue, clearly documenting both sides of the contract.
🧹 Nitpick comments (5)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/clientcrypto/test/service/ClientCryptoManagerServiceTest.java (5)
128-145: AndroidvalidateSignaturetests align with implementation; consider also covering the “invalid signature → false” caseThese two tests nicely pin down:
- Happy path with a real RSA key/signature (
trueresult), and- Error path where an invalid public key triggers
ClientCryptoException.Given
AndroidClientCryptoServiceImpl.validateSignaturecurrently returns the underlying signature‑verification boolean and only throws on key/JCA failures, it would be valuable to add a third test where:
- The public key is valid,
- The signature is deliberately corrupted,
- And the method is asserted to return
false(not throw).That locks in the distinction between “cryptographic mismatch” and “runtime failure” for future refactors.
88-115: Reflection + static field wiring makes tests tightly coupled to implementation detailsThe setup wires
ClientCryptoFacade,LocalClientCryptoServiceImpl, and their collaborators by:
- Injecting private fields via reflection, and
- Mutating multiple static fields (
clientCryptoService,secureRandom,LocalClientCryptoServiceImpl.cryptoCore).This gives good coverage with the current design, but it’s brittle:
- Renaming private fields or changing constructor signatures will silently break tests.
- Static state makes these tests unsafe for parallel execution with other suites touching the same classes.
If feasible, consider longer‑term refactors like:
- Adding package‑visible or test‑only constructors that accept dependencies directly, or
- Reducing static state in the production classes so tests can rely on normal DI wiring instead of reflection.
166-185: Minor: payload construction could more faithfully mirror the real envelope formatIn
testClientCryptoFacadeDecryptUsesClientSecurity, the first segment ofpayloadis initialized withnew byte[secret.length]rather than thesecretarray you later expect fromclientCryptoService.asymmetricDecrypt(...). Because the mock ignores the input and always returnssecret, the test still passes, but it no longer validates that the envelope layout matches production expectations.Consider using
secretinstead of a zeroed array for the first copy, mirroring the structure you already use in the fallback test.
503-511: EnsurecleanKeysDirectoryonly touches test‑scoped key material
cleanKeysDirectory()recursively deletes everything underClientCryptoManagerConstant.KEY_PATH/KEYS_DIR. That’s fine if those constants resolve to a test‑only directory (e.g., undertarget), but risky if they can ever point at a shared or production‑like location on a developer machine or CI.If there’s any chance of overlap, consider:
- Overriding KEY_PATH/KEYS_DIR via test configuration to a temp directory, or
- Using JUnit/temporary directories explicitly rather than shared constants.
547-555: Key‑size assumption inrsaModulusBytesis implicitly 2048‑bit
rsaModulusBytes()normalizes the modulus to 256 bytes by either truncating or left‑padding. That’s consistent with a 2048‑bit RSA modulus (as used by the TPM helpers), but would silently misbehave if the key size ever changed.Given this is test‑only code, it’s acceptable, but you might want to:
- Assert the modulus bit length is 2048 before normalization, or
- Derive the target length from the actual modulus size instead of hard‑coding 256.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/clientcrypto/test/service/ClientCryptoManagerServiceTest.java(1 hunks)kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/crypto/jce/test/CryptoCoreTest.java(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/clientcrypto/test/service/ClientCryptoManagerServiceTest.java (2)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/clientcrypto/exception/ClientCryptoException.java (1)
ClientCryptoException(10-35)kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/clientcrypto/service/impl/AndroidClientCryptoServiceImpl.java (1)
AndroidClientCryptoServiceImpl(20-105)
⏰ 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). (1)
- GitHub Check: build-kernel / maven-build
🔇 Additional comments (2)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/crypto/jce/test/CryptoCoreTest.java (1)
53-73: LGTM: Certificate wiring for certificate-based sign/verify testsInline PEM plus
KeymanagerUtil.convertToCertificateand the dedicated test boot app give you a realistic certificate context for the new sign/verify tests without touching production wiring. This is a good pattern for exercising cert-based flows in isolation.kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/clientcrypto/test/service/ClientCryptoManagerServiceTest.java (1)
220-313: LGTM: TPM client crypto tests give strong coverage of caching, error paths, and random generationThe TPM tests here exercise:
- Primary response caching for signing/encryption public parts,
- Happy‑path sign/decrypt/random operations via a mocked
Tpm,- Behavior when
tpmisnulland whenSignreturnsnull,closeSecurityInstanceboth with and without a real TPM instance.This is a solid set of tests for a tricky, stateful integration point.
2 participants
Summary by CodeRabbit