MOSIP-43923 #279
MOSIP-43923 #279
Conversation
Signed-off-by: kaledOu <kaled.ouali@gmail.com>
Signed-off-by: kaledOu <kaled.ouali@gmail.com>
Signed-off-by: kaledOu <kaled.ouali@gmail.com>
Signed-off-by: kaledOu <kaled.ouali@gmail.com>
Signed-off-by: kaledOu <kaled.ouali@gmail.com>
WalkthroughAdds SonarQube and JaCoCo integrations to the Maven build, introduces mockito-inline, migrates many tests from PowerMock to Mockito, standardizes numerous test method names to camelCase, and adds several new comprehensive unit test classes plus a test XSD resource. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (4)
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/config/TestConfig.java (3)
1-48: Critical security vulnerability: SSL certificate validation is disabled.The
TrustStrategyat line 27 returnstruefor all certificates, which completely disables SSL/TLS verification. This makes the application vulnerable to man-in-the-middle attacks. While this may be acceptable in a test environment, it requires explicit documentation and should never be used in production.Add a clear warning comment and ensure this configuration is only accessible during testing.
🔎 Proposed fix: Add security warning comment
@Bean public RestTemplate restTemplate() throws KeyManagementException, NoSuchAlgorithmException, KeyStoreException { + // WARNING: This configuration accepts all SSL certificates without verification. + // This is only suitable for testing and should NEVER be used in production. TrustStrategy acceptingTrustStrategy = (X509Certificate[] chain, String authType) -> true;
36-36: Remove commented-out code and fix formatting.Line 36 contains commented-out code that should be deleted. Additionally, line 37 has inconsistent spacing.
🔎 Proposed fix: Remove commented code and fix formatting
SSLConnectionSocketFactory csf = new SSLConnectionSocketFactory(sslContext); HttpClientConnectionManager cm = PoolingHttpClientConnectionManagerBuilder.create() .setSSLSocketFactory(csf) .build(); - //CloseableHttpClient httpClient = HttpClients.custom().setSSLSocketFactory(csf).build(); - CloseableHttpClient httpClient =HttpClientBuilder - .create() - .setConnectionManager(cm) - .build(); + HttpClientConnectionManager cm = PoolingHttpClientConnectionManagerBuilder.create() + .setSSLSocketFactory(csf) + .build(); + CloseableHttpClient httpClient = HttpClientBuilder.create() + .setConnectionManager(cm) + .build();Also applies to: 37-37
3-3: Fix HttpClient version mismatch: code cannot compile with the declared dependency.The code imports from both HttpClient 4.x (
org.apache.http.ssl.*) and 5.x (org.apache.hc.client5.*) APIs. Spring Boot 3.2.3 (via the parent pom) provides only HttpClient 5.x (5.2.3), which does not include theorg.apache.http.ssl.*classes. Lines 8 (TrustStrategy) and 29 (SSLContexts) reference classes that do not exist in the classpath, causing compilation failure.Migrate all imports and usage to HttpClient 5.x APIs:
- Replace
org.apache.http.ssl.TrustStrategywithorg.apache.hc.core5.ssl.TrustStrategy- Replace
org.apache.http.ssl.SSLContextswithorg.apache.hc.core5.ssl.SSLContextsAlso applies to: commons-packet-service/src/test/java/io/mosip/commons/packetmanager/test/config/TestConfig.java (identical issue)
🧹 Nitpick comments (18)
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/keeper/PacketKeeperTest.java (1)
359-361: Complete or remove these ineffective tests.These tests only set the adapter name field but don't verify that the configured adapter is actually used. Without assertions or method invocations to verify behavior, they always pass regardless of correctness.
Consider either:
- Completing the tests by calling a method (e.g.,
putPacketorgetPacket) and verifying the correct adapter is invoked- Removing these tests if adapter selection is already covered elsewhere
🔎 Example of completed test
@Test public void testAdapterSelectionWhenPosixAdapterConfiguredUsesPosixAdapter() { ReflectionTestUtils.setField(packetKeeper, "adapterName", "PosixAdapter"); + + Mockito.when(posixAdapter.putObject(any(), any(), any(), any(), any(), any())).thenReturn(true); + + PacketInfo result = packetKeeper.putPacket(packet); + + Mockito.verify(posixAdapter, Mockito.times(1)).putObject(any(), any(), any(), any(), any(), any()); + assertNotNull(result); }Also applies to: 367-369
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/IdSchemaUtilsTest.java (3)
9-9: Unused import:@Mockannotation.The
@Mockannotation is imported but never used in this class. All mocks are created inline usingmock(). Remove the unused import.🔎 Proposed fix
-import org.mockito.Mock;
114-116: Misleading comment contradicts the assertion.The comment states "returns null for source" but line 103-104 mentions "getSource returns defaultSource". This is confusing. Either clarify the expected behavior or update the assertion if the actual behavior should be non-null.
168-170: Fix formatting:@Testannotation should be adjacent to its method.The blank line between the
@Testannotation and the method declaration is unconventional and reduces readability.🔎 Proposed fix
assertEquals("1.0", version); } + @Test - void testGetMappingJsonSuccess() throws Exception {commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketManagerHelperTest.java (1)
36-36: Optional: Remove redundant assertion.The
assertNotNull(helper)check is redundant sincehelperis initialized in@BeforeEach setup(), which must succeed for this test to run.commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.java (5)
200-205: Remove commented-out code.These commented-out test method and option comments are dead code that should be removed. The replacement test (
testLoadDemographicIdentityWithSimpleStringValues) is already implemented below.🔎 Proposed fix
- // Remove or fix the problematic test - Option 1: Remove it - // @Test - // public void testLoadDemographicIdentity_SimpleValues() throws Exception { - // // This test is causing issues with unnecessary stubbing - // } - - // Option 2: Fix it by properly testing the method @Test public void testLoadDemographicIdentityWithSimpleStringValues() throws Exception {
219-219: Use parameterized types instead of raw types.Multiple places in this test class use raw types (
LinkedHashMap,List) which suppress generic type checking. Consider using parameterized types for better type safety.Examples:
- Line 219, 235, 260:
LinkedHashMap<String, Object> result- Line 267:
List<?> arr- Lines 331, 370, 446, 557, 607:
List<Map<String, Object>> hashseqList
331-332: Consider avoiding double-brace initialization.Double-brace initialization (e.g.,
new HashMap<>() {{ put(...); }}) creates anonymous inner classes that hold references to the enclosing instance. While acceptable in test code, usingMap.of()or explicitput()calls is cleaner.🔎 Example alternative
- List hashseq1List = Arrays.asList(new HashMap<String, Object>() {{ put("value", Arrays.asList(file1)); }}); - List hashseq2List = Arrays.asList(new HashMap<String, Object>() {{ put("value", Arrays.asList(file1)); }}); + Map<String, Object> seqMap = new HashMap<>(); + seqMap.put("value", Arrays.asList(file1)); + List<Map<String, Object>> hashseq1List = Arrays.asList(seqMap); + List<Map<String, Object>> hashseq2List = Arrays.asList(seqMap);
154-159: Consider usingassertInstanceOffor cleaner exception type checking.The exception type check could be simplified using JUnit 5's
assertInstanceOfor by catching the specific exception type if the production code is expected to throwIOExceptiondirectly.🔎 Alternative approach
// Execute & Verify - Exception exception = assertThrows(Exception.class, () -> + IOException exception = assertThrows(IOException.class, () -> packetValidator.validate(testId, testSource, testProcess)); - - // Check it's an IOException or wrapped IOException - assertTrue(exception instanceof IOException || - (exception.getCause() != null && exception.getCause() instanceof IOException)); + assertNotNull(exception);If the method wraps the exception, keep the current approach but consider using
assertInstanceOf:assertTrue(exception instanceof IOException || exception.getCause() instanceof IOException);
618-631: LGTM - Consider making helper method static.The
createZipByteshelper is well-implemented with proper resource management. Since it doesn't use any instance state, it could be declaredstaticfor clarity.commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/IdObjectsSchemaValidationOperationMapperTest.java (1)
10-15: Consider removing the constructor test unless required for coverage.Testing a default constructor that contains no logic typically adds little value. This test might be retained for coverage metrics, but otherwise could be removed to reduce test maintenance overhead.
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketHelperTest.java (2)
16-23: Test isolation approach is sound for static utility class.Using
ReflectionTestUtilsto reset static state between tests is appropriate here, given thatPacketHelperuses static caching. This prevents test pollution and ensures each test runs with a clean slate.However, this pattern highlights a broader design concern: static mutable state in
PacketHelpermakes testing difficult and could cause issues in production (e.g., thread-safety, testability). Consider refactoringPacketHelperto use instance-based state or dependency injection in future work.
44-68: Solid test coverage for source/process matching logic.The tests correctly validate:
- Provider name substring matching (line 51)
- Process mismatch detection (line 55)
- Iteration suffix stripping (line 66: "NEW-1" → "NEW")
Consider adding equivalent tests for
Provider.WRITERto ensure both code paths are covered. Currently, onlyProvider.READERis tested for theisSourceAndProcessPresentmethod.commons-packet/commons-packet-manager/src/test/resources/cbeff.xsd (1)
1-5: Consider whether the simplified schema meets test requirements.The schema defines
BIRas a simple string type. If this is intended for testing XML validation of Biometric Information Records, consider whether a more representative schema structure would provide better test coverage (e.g., nested elements for biometric data, metadata, quality scores).commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/impl/OfflinePacketCryptoServiceTest.java (3)
68-85: Consider handling null response more gracefully.The test
signWhenCsSignReturnsNullThrowsNullPointerExceptionexpects a NullPointerException whencsSignreturns null. Consider whether the service should handle this case more gracefully by throwing a more specific exception (e.g.,SignatureException) rather than allowing an NPE to propagate.
116-120: Consider handling null response more gracefully.Similar to the sign method, the encrypt method throws NullPointerException when cryptomanagerService returns null. Consider throwing a more descriptive exception to improve debugging and error handling.
135-184: LGTM! Good test coverage for edge cases.The new test cases provide good coverage for null handling and edge cases in decrypt and verify operations. The pattern of expecting NullPointerException consistently appears throughout - consider a broader refactoring to handle nulls more gracefully across all methods.
commons-packet/commons-packet-manager/pom.xml (1)
46-46: Consider updating the SonarQube Maven plugin version.The sonar-maven-plugin version 3.7.0.1746 (October 2019) is significantly outdated. The latest stable version is 5.5.0.6356 (December 2025). Review compatibility with your SonarQube server instance and consider upgrading unless this older version is intentionally locked for compatibility reasons.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
commons-packet/commons-packet-manager/pom.xmlcommons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/config/TestConfig.javacommons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/dto/packet/RegistrationPacketTest.javacommons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/impl/OfflinePacketCryptoServiceTest.javacommons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/impl/OnlinePacketCryptoServiceTest.javacommons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/impl/PacketReaderImplTest.javacommons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/impl/PacketWriterImplTest.javacommons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/keeper/PacketKeeperTest.javacommons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/IdObjectsSchemaValidationOperationMapperTest.javacommons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/IdSchemaUtilsTest.javacommons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketHelperTest.javacommons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketManagerHelperTest.javacommons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.javacommons-packet/commons-packet-manager/src/test/resources/cbeff.xsd
🧰 Additional context used
🧬 Code graph analysis (5)
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.java (1)
commons-packet/commons-packet-manager/src/main/java/io/mosip/commons/packet/dto/packet/FieldValueArray.java (1)
FieldValueArray(12-47)
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/IdSchemaUtilsTest.java (1)
commons-packet/commons-packet-manager/src/main/java/io/mosip/commons/packet/exception/ApiNotAccessibleException.java (1)
ApiNotAccessibleException(6-28)
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketHelperTest.java (1)
commons-packet/commons-packet-manager/src/main/java/io/mosip/commons/packet/util/PacketHelper.java (1)
PacketHelper(14-148)
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/impl/OfflinePacketCryptoServiceTest.java (1)
commons-packet/commons-packet-manager/src/main/java/io/mosip/commons/packet/constants/CryptomanagerConstant.java (1)
CryptomanagerConstant(8-28)
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/impl/OnlinePacketCryptoServiceTest.java (1)
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/impl/OfflinePacketCryptoServiceTest.java (1)
RunWith(29-227)
🔇 Additional comments (21)
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/keeper/PacketKeeperTest.java (3)
243-247: LGTM! Naming follows Java conventions.The method rename from snake_case to camelCase is appropriate and aligns with standard Java naming conventions.
253-262: LGTM! Consistent naming improvements.The method renames consistently apply camelCase convention across the deletePacket and pack test methods. All tests properly verify expected behavior.
Also applies to: 268-277, 283-293, 299-309
315-319: LGTM! Well-named error scenario tests.The renamed getPacket test methods clearly describe their scenarios and follow Java conventions. All tests properly verify exception handling and edge cases.
Also applies to: 325-331, 337-342, 348-353
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/IdSchemaUtilsTest.java (5)
30-69: LGTM!Good coverage of static methods with null handling and various type scenarios. Clear and well-structured test cases.
73-92: LGTM with note on reflection usage.The reflection-based testing of private methods works correctly here. Consider whether these behaviors could be tested through public API instead, as reflection tests couple to implementation details and may break if internals are refactored.
227-229: Consider whetherClassCastExceptionindicates a production bug.Expecting
ClassCastExceptionwhen the response is null suggests the production code (IdSchemaUtils.getIdSchema) may not handle null responses gracefully. If this is intentional behavior, add a comment explaining why. Otherwise, consider opening an issue to improve null handling in the production code.
284-297: LGTM!Good practice to test constant values to catch accidental changes. The assertions are correct and comprehensive.
299-302: LGTM!Basic instantiation test provides coverage for the default constructor.
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.java (2)
42-78: LGTM!The test class setup is well-structured with appropriate use of JUnit 5's
@ExtendWith(MockitoExtension.class)and proper mock injection. The@BeforeEachsetup correctly initializes private fields needed for the tests.
80-108: Good test coverage for validation failure scenarios.The test properly verifies that schema validation failure results in
falsereturn value and appropriate audit logging. The mock setup is comprehensive and the assertions are meaningful.commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketHelperTest.java (2)
25-42: Well-structured test for configuration parsing.The test clearly validates that both reader and writer configurations are parsed correctly, extracting class names into the expected sets. The test data format matches the production configuration structure.
81-100: Excellent comprehensive test for iteration removal logic.This test thoroughly validates
getProcessWithoutIterationacross all relevant scenarios:
- Null and empty inputs
- Processes without iteration suffixes
- Single and multi-segment processes with numeric suffixes
- Non-numeric suffixes that should be preserved
The test cases provide strong confidence in the utility method's correctness.
commons-packet/commons-packet-manager/pom.xml (3)
179-179: LGTM!The argLine correctly references
${jacocoArgLine}which is configured by the Jacoco plugin, enabling code coverage instrumentation during test execution.
270-295: LGTM!The Jacoco configuration is well-structured with appropriate exclusions for non-testable code (DTOs, entities, config classes, etc.) and properly sets the
jacocoArgLineproperty for use in the surefire plugin.
352-383: LGTM! Verify SonarQube configuration in CI.The SonarQube profile is properly configured with appropriate inclusions and exclusions. Ensure that the CI pipeline activates this profile with
-P sonarand that the necessary SonarQube authentication tokens are configured.commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/impl/OnlinePacketCryptoServiceTest.java (3)
39-62: LGTM! Clean migration to Mockito.The migration from PowerMock to MockitoJUnitRunner is well-executed. The setup method properly configures all necessary fields using ReflectionTestUtils, and the field naming is clear and concise.
64-107: LGTM! Clever approach to testing Jackson exception handling.The sign tests provide good coverage for success and error paths. The pattern of temporarily swapping the mocked ObjectMapper with a real instance (lines 99-106) to trigger IOException is an effective way to test exception handling without complex PowerMock setups.
109-276: LGTM! Comprehensive test coverage.The test suite provides excellent coverage for all crypto operations (encrypt, decrypt, verify) including success paths, error responses from the API, and various exception scenarios. The tests properly verify that exceptions are wrapped in appropriate domain-specific exceptions.
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/impl/PacketWriterImplTest.java (1)
148-316: LGTM! Consistent test naming convention.All test method names have been properly renamed from snake_case to camelCase, aligning with Java naming conventions and improving code consistency across the test suite.
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/impl/PacketReaderImplTest.java (2)
462-1002: LGTM! Consistent test naming standardization.All test method names have been properly standardized from snake_case to camelCase, improving code consistency across the test suite and aligning with Java naming conventions.
1003-1042: LGTM! Valuable tests for type handling.The new tests provide important coverage for how the
getAllmethod handles different value types:
getAllNumberValueShouldPutAsIsensures numeric values are preserved correctlygetAllObjectValueShouldConvertToJsonStringverifies complex objects are properly serializedThese tests help prevent type-related bugs and ensure consistent behavior.
...et-manager/src/test/java/io/mosip/commons/packet/test/dto/packet/RegistrationPacketTest.java
Show resolved
Hide resolved
...-manager/src/test/java/io/mosip/commons/packet/test/impl/OfflinePacketCryptoServiceTest.java
Outdated
Show resolved
Hide resolved
...est/java/io/mosip/commons/packet/test/util/IdObjectsSchemaValidationOperationMapperTest.java
Show resolved
Hide resolved
...ommons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/IdSchemaUtilsTest.java
Outdated
Show resolved
Hide resolved
...commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketHelperTest.java
Show resolved
Hide resolved
...-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketManagerHelperTest.java
Show resolved
Hide resolved
...mons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.java
Outdated
Show resolved
Hide resolved
...mons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.java
Show resolved
Hide resolved
Signed-off-by: kouali <kouali@talys.digital>
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.java (4)
358-379: Stream consumption issue persists - apply same fix as line 310.Similar to the issue flagged in past reviews, the same
InputStreamat line 359 is consumed bygenerateHashon line 361, but then the samechecksumMapis passed tochecksumValidationon line 379. While the test at lines 310-316 correctly uses fresh streams for each hash computation, this test hasn't been updated with the same pattern.🔎 Proposed fix - create fresh stream for checksumValidation
+ // Create fresh checksumMap for validation + Map<String, InputStream> checksumMapForValidation = new HashMap<>(); + checksumMapForValidation.put(file1, new ByteArrayInputStream(content1)); + - boolean result = (boolean) ReflectionTestUtils.invokeMethod(packetValidator, "checksumValidation", hashseq1List, hashseq2List, checksumMap, packet); + boolean result = (boolean) ReflectionTestUtils.invokeMethod(packetValidator, "checksumValidation", hashseq1List, hashseq2List, checksumMapForValidation, packet);
392-411: Stream consumption issue - same pattern as lines 358-379.The
InputStreamat line 393 is consumed bygenerateHashon line 395, then reused inchecksumValidationon line 411. Apply the same fix as suggested for the previous test method.🔎 Proposed fix
+ // Create fresh checksumMap for validation + Map<String, InputStream> checksumMapForValidation = new HashMap<>(); + checksumMapForValidation.put(file1, new ByteArrayInputStream(content1)); + - boolean result = (boolean) ReflectionTestUtils.invokeMethod(packetValidator, "checksumValidation", hashseq1List, hashseq2List, checksumMap, packet); + boolean result = (boolean) ReflectionTestUtils.invokeMethod(packetValidator, "checksumValidation", hashseq1List, hashseq2List, checksumMapForValidation, packet);
535-540: Stream consumed before second use - same issue from past review.The past review comment specifically flagged this pattern:
checksumMapForHashcontains anInputStreamthat gets consumed by the firstgenerateHashcall on line 539. WhengenerateHashis called again on line 540 with the same map, the stream is already exhausted. This is the exact issue mentioned in the past review comments.🔎 Proposed fix - use separate streams as done in lines 310-316
- // Prepare a checksumMap to compute the hashes (simulate file bytes) - Map<String, InputStream> checksumMapForHash = new HashMap<>(); - checksumMapForHash.put(file1, new ByteArrayInputStream(content)); - - // Compute hashes using private generateHash - byte[] dataHash = (byte[]) ReflectionTestUtils.invokeMethod(packetValidator, "generateHash", hashSeq, checksumMapForHash); - byte[] opsHash = (byte[]) ReflectionTestUtils.invokeMethod(packetValidator, "generateHash", hashSeq, checksumMapForHash); + // Compute dataHash using fresh stream + Map<String, InputStream> checksumMapForDataHash = new HashMap<>(); + checksumMapForDataHash.put(file1, new ByteArrayInputStream(content)); + byte[] dataHash = (byte[]) ReflectionTestUtils.invokeMethod(packetValidator, "generateHash", hashSeq, checksumMapForDataHash); + + // Compute opsHash using fresh stream + Map<String, InputStream> checksumMapForOpsHash = new HashMap<>(); + checksumMapForOpsHash.put(file1, new ByteArrayInputStream(content)); + byte[] opsHash = (byte[]) ReflectionTestUtils.invokeMethod(packetValidator, "generateHash", hashSeq, checksumMapForOpsHash);
597-599: Stream reuse issue in end-to-end test.Same stream consumption pattern as lines 535-540. The
checksumMapForHashis used for two consecutivegenerateHashcalls, consuming the stream on the first call.🔎 Proposed fix
- Map<String, InputStream> checksumMapForHash = new HashMap<>(); checksumMapForHash.put(file1, new ByteArrayInputStream(content)); - byte[] dataHash = (byte[]) ReflectionTestUtils.invokeMethod(packetValidator, "generateHash", hashSeq, checksumMapForHash); - byte[] opsHash = (byte[]) ReflectionTestUtils.invokeMethod(packetValidator, "generateHash", hashSeq, checksumMapForHash); + // Compute dataHash using fresh stream + Map<String, InputStream> checksumMapForDataHash = new HashMap<>(); + checksumMapForDataHash.put(file1, new ByteArrayInputStream(content)); + byte[] dataHash = (byte[]) ReflectionTestUtils.invokeMethod(packetValidator, "generateHash", hashSeq, checksumMapForDataHash); + + // Compute opsHash using fresh stream + Map<String, InputStream> checksumMapForOpsHash = new HashMap<>(); + checksumMapForOpsHash.put(file1, new ByteArrayInputStream(content)); + byte[] opsHash = (byte[]) ReflectionTestUtils.invokeMethod(packetValidator, "generateHash", hashSeq, checksumMapForOpsHash);
🧹 Nitpick comments (17)
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/dto/packet/RegistrationPacketTest.java (3)
16-16: Remove ineffective@Autowiredannotation.The
@Autowiredannotation has no effect here because:
- This test class doesn't use a Spring test runner (no
@RunWith(SpringRunner.class)or@SpringBootTest)- The field is immediately reassigned in the
@Beforemethod (line 21)This annotation is misleading and should be removed.
🔎 Proposed fix
- @Autowired private RegistrationPacket registrationPacket;
38-38: Inconsistent camelCase in method name.The method name uses
Objectofbut should beObjectOf(with capital "O") to properly follow camelCase conventions. The past review suggestedsetFieldsWithObjectOfMapThenPass().🔎 Proposed fix
- public void setFieldsWithObjectofMapThenPass() + public void setFieldsWithObjectOfMapThenPass()
79-79: Fix typo in variable name.The variable
responceStringappears to be misspelled. It should beresponseString.🔎 Proposed fix
- String responceString= (String) mapObj.get("StringExp"); - assertEquals("ExpString",responceString); + String responseString= (String) mapObj.get("StringExp"); + assertEquals("ExpString",responseString);commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/impl/OfflinePacketCryptoServiceTest.java (2)
68-72: Consider more descriptive exception handling in production code.These tests expose that the production code throws
NullPointerExceptionwhen external services return null responses. While the test correctly documents this behavior, the production code should ideally throw more descriptive custom exceptions (e.g.,CryptoServiceExceptionorInvalidResponseException) with meaningful messages rather than allowingNullPointerExceptionto propagate.
111-111: Clarify or remove misleading comment.The comment suggests calling a "real mergeEncryptedData from khazana," but
cryptomanagerServiceis mocked (line 109). The test verifies thatencryptreturns a non-null result when the mocked service returns valid data. Consider removing or clarifying this comment to accurately reflect what the test validates.commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketManagerHelperTest.java (2)
25-33: Consider extracting magic numbers to constants.The hardcoded values
5and6forcenterIdLengthandmachineIdLengthcould be extracted as named constants for better maintainability and to make the relationship with test data more explicit.Optional refactor
+ private static final int CENTER_ID_LENGTH = 5; + private static final int MACHINE_ID_LENGTH = 6; + @BeforeEach public void setup() { helper = new PacketManagerHelper(); ReflectionTestUtils.setField(helper, "configServerFileStorageURL", "http://localhost:1000/"); ReflectionTestUtils.setField(helper, "schemaName", "mosip-cbeff.xsd"); - ReflectionTestUtils.setField(helper, "centerIdLength", 5); - ReflectionTestUtils.setField(helper, "machineIdLength", 6); + ReflectionTestUtils.setField(helper, "centerIdLength", CENTER_ID_LENGTH); + ReflectionTestUtils.setField(helper, "machineIdLength", MACHINE_ID_LENGTH); }
44-50: Consider adding edge case tests forgetRefId.The current test only covers the happy path. Consider adding tests for edge cases such as:
- ID shorter than
centerIdLength + machineIdLength- Null or empty ID
- ID exactly equal to
centerIdLength + machineIdLengthThese tests would help ensure robust error handling.
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.java (2)
200-204: Remove commented-out code.Dead code should be removed rather than commented out. Version control preserves the history if needed.
🔎 Proposed fix
- // Remove or fix the problematic test - Option 1: Remove it - // @Test - // public void testLoadDemographicIdentity_SimpleValues() throws Exception { - // // This test is causing issues with unnecessary stubbing - // } - - // Option 2: Fix it by properly testing the method
258-258: Consider more precise matcher for JSON parsing.The
contains("\"x\"")matcher is fragile and may match unintended strings. If the exact JSON structure is known, consider usingeq()with the exact string or a more specific pattern.commons-packet/commons-packet-manager/pom.xml (2)
270-285: Consider aligning JaCoCo excludes with SonarQube exclusions.The JaCoCo
excludeslist differs significantly fromsonar.coverage.exclusions(line 44). JaCoCo will measure coverage for many patterns that SonarQube excludes (e.g.,util/**,repository/**,security/**,*Utils.java,*Helper.java). This inconsistency may cause confusion when comparing coverage reports.Consider either:
- Aligning both exclusion lists for consistency
- Documenting why they intentionally differ
- Extracting common exclusions into a shared property
350-381: Consider refiningsonar.sourcesproperty.Setting
sonar.sourcesto.(root directory) is unconventional, thoughsonar.inclusionsdoes limit the scope to source files. For clarity and to avoid potential issues, consider settingsonar.sourcestosrc/main/javainstead, or omitting it to use Maven defaults.🔎 Suggested refinement
- <sonar.sources>.</sonar.sources> + <sonar.sources>src/main/java</sonar.sources> <sonar.inclusions>src/main/java/**,src/main/resources/**</sonar.inclusions>commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/IdSchemaUtilsTest.java (6)
9-9: Remove LENIENT strictness mode and unused@Mockimport.The class uses
Strictness.LENIENTmode, which suppresses warnings about unused stubs and reduces test reliability. This should only be used when absolutely necessary, and in this case, all mocks are created explicitly withmock()calls. Additionally, the@Mockannotation is imported but never used (no@Mockfields are declared).🔎 Proposed fix
-import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; -import org.mockito.junit.jupiter.MockitoSettings; -import org.mockito.quality.Strictness;@ExtendWith(MockitoExtension.class) -@MockitoSettings(strictness = Strictness.LENIENT) class IdSchemaUtilsTest {If removing
LENIENTmode causes test failures, address the root cause (unused stubs) rather than suppressing the warnings.Also applies to: 25-25
118-139: Consider adding mock verification.While the test validates the result, it doesn't verify that the
RestTemplatewas called with the expected URI and parameters. Addingverify(mockRestTemplate).getForObject(...)would make the test more robust and document the expected interaction.Example verification
verify(mockRestTemplate).getForObject(argThat(uri -> uri.toString().contains("idschema") && uri.toString().contains("schemaVersion=1.0") ), eq(String.class));
237-239: Avoidlenient()wrapper for mock setup.The
lenient()wrapper suppresses warnings about unused stubs. This is a code smell that suggests:
- The mock may not be used in all test scenarios
- There are strictness issues that should be addressed
Remove
lenient()and address the root cause. If the stub is truly unused, either remove it or fix the test setup.Also applies to: 251-253
282-295: Testing string constants provides minimal value.Testing public constant values is generally unnecessary for simple string literals. If these constants change, any code using them will break anyway. Consider removing this test to reduce maintenance burden, or keep it only if:
- Constants are computed or derived
- There's a specific requirement to document expected values
- The constants are part of a public API contract
298-300: Instantiation test provides minimal value.Testing basic instantiation with
assertDoesNotThrow(() -> new IdSchemaUtils())is unnecessary unless:
- The constructor has complex logic that could throw
- There's a specific bug history related to instantiation
- This is documenting an expected behavior for a no-arg constructor
Consider removing this test to reduce noise.
104-107: Consider using JSON test resources or builders for readability.The JSON response strings with escaped quotes and concatenation are hard to read and maintain. Consider:
- Using external JSON files in
src/test/resources- Using a JSON builder library for test data
- Using text blocks (Java 13+) for multi-line strings
Example with text block
String schemaResponse = """ { "response": { "schemaJson": "{\\"properties\\":{\\"identity\\":{\\"properties\\":{}}}}" } } """;Also applies to: 125-128, 199-204
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
commons-packet/commons-packet-manager/pom.xmlcommons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/dto/packet/RegistrationPacketTest.javacommons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/impl/OfflinePacketCryptoServiceTest.javacommons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/IdSchemaUtilsTest.javacommons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketManagerHelperTest.javacommons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.java
🧰 Additional context used
🧬 Code graph analysis (1)
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.java (1)
commons-packet/commons-packet-manager/src/main/java/io/mosip/commons/packet/dto/packet/FieldValueArray.java (1)
FieldValueArray(12-47)
🔇 Additional comments (14)
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/impl/OfflinePacketCryptoServiceTest.java (5)
19-19: LGTM! Test runner migration to Mockito.The migration from PowerMock to
MockitoJUnitRunner.Silentmodernizes the test infrastructure and aligns with the broader migration strategy across this PR.Also applies to: 26-26, 29-29
65-66: Good use of assertArrayEquals for byte array comparison.Using
assertArrayEqualsinstead of direct comparison provides better failure messages and is the idiomatic way to compare byte arrays in JUnit.
100-120: Good edge-case coverage for encrypt method.The new tests provide comprehensive coverage of the encrypt method's behavior with valid data and null responses from the cryptomanager service.
135-155: Excellent boundary condition testing.The
decryptWithShortPacketCallsDecryptWithEmptyEncryptedDatatest effectively validates the edge case where a packet contains only the nonce and AAD with no encrypted data. This is valuable coverage for potential malformed input scenarios.
170-184: LGTM! Comprehensive verify test coverage and consistent naming.The new verify tests cover both negative verification results and null response handling. The method name updates to camelCase align with Java naming conventions and the PR's standardization goals.
Also applies to: 190-190, 204-204, 218-218
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketManagerHelperTest.java (4)
1-19: LGTM!The imports are clean and necessary. The inclusion of
assumeTruefrom JUnit Assumptions aligns with the recommended approach from the previous review.
21-23: LGTM!Clean test class structure with appropriate field declaration.
35-42: LGTM!Straightforward test that verifies the helper returns the provided reference unchanged.
51-76: Previous review feedback successfully addressed.The
@Disabledannotation andassumeTrue(false, ...)in the catch block both implement the recommended fixes from the previous review. While having both is technically redundant, this belt-and-suspenders approach is reasonable—it allows the test to be easily enabled for local verification while preventing CI failures.commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.java (1)
310-316: Stream consumption issue correctly fixed.Good job addressing the previous review feedback! This test now creates fresh streams for each hash computation, avoiding the stream exhaustion issue.
commons-packet/commons-packet-manager/pom.xml (2)
179-179: LGTM! Proper JaCoCo integration.The
argLinecorrectly includes${jacocoArgLine}before the JVM flags, ensuring JaCoCo test coverage is collected during test execution.
288-289: LGTM! Correct phase for JaCoCo agent.Setting the
prepare-agentexecution to theinitializephase is the recommended approach, ensuring the JaCoCo agent is attached before tests run.commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/IdSchemaUtilsTest.java (2)
28-69: LGTM! Comprehensive static method tests.The static method tests appropriately cover edge cases (null, empty JSON) and typical scenarios for both
getJSONObjectandgetJSONValue. The test assertions are clear and verify the expected behavior.
192-213: Test logic appears sound.The test properly validates default field extraction behavior. The assertion and comment align with the expected implementation.
...ommons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/IdSchemaUtilsTest.java
Show resolved
Hide resolved
...ommons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/IdSchemaUtilsTest.java
Show resolved
Hide resolved
...ommons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/IdSchemaUtilsTest.java
Show resolved
Hide resolved
...ommons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/IdSchemaUtilsTest.java
Show resolved
Hide resolved
Signed-off-by: kouali <kouali@talys.digital>
2 participants
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.