Added unit test cases #668
Conversation
Signed-off-by: sachin.sp <[email protected]>
WalkthroughAdds and updates unit tests across clientmanager and packetmanager modules, introduces new test coverage for a RegistrationDeletionJob, updates constructors/provider signatures to accept LocalConfigService and PacketService, adjusts several test mocks/imports, and relocates a DTO import path. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.40.5)android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/RegistrationServiceImplTest.java[] 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/PacketServiceImplTest.java (1)
3-6: Fix mixed JUnit4/JUnit5 static imports (likely compile ambiguity onassertEquals).You have
import static org.junit.jupiter.api.Assertions.assertEquals;(Line 3) andimport static org.junit.Assert.*;(Line 5), which both provideassertEqualsoverloads and commonly causes ambiguous resolution at compile time.Proposed fix (pick one framework and stick to it)
If this class is JUnit4 (it uses
@RunWith(MockitoJUnitRunner.class)andorg.junit.Test), drop the JUnit5 static import:-import static org.junit.jupiter.api.Assertions.assertEquals; import static org.mockito.Mockito.*; import static org.junit.Assert.*;(Alternatively, migrate fully to JUnit5:
@ExtendWith(MockitoExtension.class)+org.junit.jupiter.api.Test+ remove@RunWith.)android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/MasterDataServiceImplTest.java (2)
64-112: JUnit4 runner + JUnit5@BeforeEachmeanssetUp()likely never runs.This class uses
@RunWith(MockitoJUnitRunner.class)(Line 64) andorg.junit.Test, but the setup usesorg.junit.jupiter.api.BeforeEach(Line 106). Under JUnit4 execution,@BeforeEachwon’t be invoked, so any stubbing insidesetUp()may not apply.Proposed fix (stay JUnit4)
-import org.junit.jupiter.api.BeforeEach; +import org.junit.Before; ... - @BeforeEach + @Before public void setUp() { MockitoAnnotations.openMocks(this); when(mockContext.getString(anyInt())).thenReturn(TEST_APP_NAME); when(mockContext.getSharedPreferences(eq(TEST_APP_NAME), eq(Context.MODE_PRIVATE))) .thenReturn(mockSharedPreferences); }(Or migrate fully to JUnit5 with
@ExtendWith(MockitoExtension.class)andorg.junit.jupiter.api.Test.)
1335-1343:saveCACertificatetest still mocks keymanager DTO; clarify intent after DTO import move.Even after importing the HTTP
CACertificateResponseDto, this test explicitly mocksio.mosip.registration.keymanager.dto.CACertificateResponseDto(Line 1335-1336). That may be correct ifCertificateManagerService.uploadCACertificate(...)returns the keymanager DTO, but it’s confusing alongside the new import—especially given the identical class name.Suggestion: either fully-qualify all
CACertificateResponseDtousages in this file, or refactor imports to make the distinction obvious.
🤖 Fix all issues with AI agents
In
@android/clientmanager/src/test/java/io/mosip/registration/clientmanager/jobs/RegistrationDeletionJobTest.java:
- Around line 36-46: The timestamp verification in
testTriggerJob_SuccessfulDeletion_ReturnsTrue is brittle because
currentTimeMillis is captured before triggerJob and the production code computes
elapsed time differently; update the test to avoid exact-match timing: replace
the strict verification of logJobTransaction(jobId,
TimeUnit.MILLISECONDS.toSeconds(currentTimeMillis)) with either
verify(registrationDeletionJob).logJobTransaction(eq(jobId), anyLong()) or use
an ArgumentCaptor<Long> to capture the timestamp and assert it falls within an
expected range, or alternatively mock the time source used by
registrationDeletionJob so you can assert an exact value; ensure
packetService.deleteRegistrationPackets() is still verified and keep references
to registrationDeletionJob.triggerJob and logJobTransaction in your changes.
In
@android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/MasterDataServiceImplTest.java:
- Around line 23-24: In MasterDataServiceImplTest, remove the ambiguous wildcard
import io.mosip.registration.keymanager.dto.* and replace it with explicit
imports for only the types you actually use (or alternatively fully-qualify one
of the CACertificateResponseDto references); ensure you keep a single,
unambiguous import for CACertificateResponseDto (either
io.mosip.registration.clientmanager.dto.http.CACertificateResponseDto or
io.mosip.registration.keymanager.dto.CACertificateResponseDto) and update any
references in the test accordingly so there is no collision between the two
classes.
- Around line 51-53: Remove the unused okhttp3 imports Headers, Protocol, and
Request from MasterDataServiceImplTest.java; keep only the used
okhttp3.ResponseBody import so tests compile with strict lint—locate the import
block containing okhttp3.Headers, okhttp3.Protocol, and okhttp3.Request and
delete those three entries.
🧹 Nitpick comments (3)
android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/PacketServiceImplTest.java (2)
404-445: Good update to matchupdateServerStatusWithTimestamp, but assert the timestamp semantics.Switching the verification to
updateServerStatusWithTimestamp(regId, "UPLOADED", anyLong())(Line 439-443) aligns with the new repository API. Consider capturing thelongand asserting it matches what the production code should be using (e.g., parsed server response time vs “now”), so the test verifies behavior rather than just “a timestamp was provided”.
950-1275: Nice coverage expansion; reduce time/file-system flakiness.
- The new time-based tests (Line 950-1206) use
System.currentTimeMillis(); these can get flaky if production logic uses different time sources/timezones. Prefer a fixednow(local variable) and derive all offsets from it, or inject aClockin prod and use a fixed clock in tests.- The deletion tests (Line 1243-1260) create real temp files but don’t assert the file is actually deleted. If production deletes the file by path, add
assertFalse(new File(filePath).exists())after the call (and ensure cleanup on failure).android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/MasterDataServiceImplTest.java (1)
2424-4021: Good breadth of new tests; consider trimming private-method reflection and “expects NPE” cases.The new block adds a lot of coverage (Line 2424-4021), but:
- Many tests invoke private methods via
ReflectionTestUtils—these tend to be brittle to refactors. Prefer exercising behavior through public APIs where feasible.- Several tests assert
NullPointerExceptionfor error paths; that can lock in undesirable behavior (vs returning defaults or throwing domain exceptions). If NPE is intentional, consider asserting on message/type from a controlled exception instead.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
android/clientmanager/src/test/java/io/mosip/registration/clientmanager/config/AppModuleTest.javaandroid/clientmanager/src/test/java/io/mosip/registration/clientmanager/jobs/RegistrationDeletionJobTest.javaandroid/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/Biometrics095ServiceTest.javaandroid/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/JobManagerServiceImplTest.javaandroid/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/LocalConfigServiceImplTest.javaandroid/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/MasterDataServiceImplTest.javaandroid/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/PacketServiceImplTest.java
🔇 Additional comments (13)
android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/Biometrics095ServiceTest.java (1)
74-76: LGTM! Explicit environment mocking improves test clarity.The added mocks for
SERVER_ACTIVE_PROFILEmake the environment value explicit ("Developer") for capture request tests, improving test predictability and readability.Also applies to: 175-176
android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/LocalConfigServiceImplTest.java (5)
67-78: LGTM! Proper delegation testing.The test correctly verifies that
getValuedelegates to the DAO layer and returns the expected result.
80-90: LGTM! Comprehensive permission retrieval test.The test validates that
getPermittedJobscorrectly delegates to the DAO with the appropriate constant (PERMITTED_JOB_TYPE).
92-153: LGTM! Thorough input validation coverage.Excellent coverage of edge cases for
modifyJobinput validation:
- Null, empty, and blank checks for both name and value parameters
- Invalid cron expression validation
- Proper verification that DAO is never called when validation fails
155-166: LGTM! Permission enforcement test.The test correctly validates that unauthorized job modifications are rejected, ensuring security boundaries are enforced.
168-192: LGTM! Success path validation.Both tests verify successful delegation to the DAO for valid inputs, including proper handling of whitespace in cron expressions.
android/clientmanager/src/test/java/io/mosip/registration/clientmanager/jobs/RegistrationDeletionJobTest.java (2)
48-58: LGTM! Exception handling test.The test correctly verifies that when
deleteRegistrationPacketsthrows an exception,triggerJobreturnsfalseand the exception is handled gracefully.
60-67: LGTM! Logging verification test.The test validates that the deletion method is invoked during job execution.
android/clientmanager/src/test/java/io/mosip/registration/clientmanager/config/AppModuleTest.java (3)
109-113: LGTM! Mock additions for new dependencies.The mock declarations for
LocalConfigServiceandPacketServiceproperly support the updated dependency injection wiring in the AppModule.
208-208: LGTM! Updated parameter list.The test correctly passes the new
packetServicedependency toprovideRegistrationService, aligning with the updated method signature.
251-251: LGTM! Updated parameter list.The test correctly passes the new
localConfigServicedependency toprovideJobManagerService, aligning with the updated method signature.android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/JobManagerServiceImplTest.java (2)
20-20: LGTM! Proper mock setup for new dependency.The import and mock declaration for
LocalConfigServiceare correctly added to support the updatedJobManagerServiceImplconstructor signature.Also applies to: 46-47
63-63: LGTM! Consistent constructor updates across all tests.All
JobManagerServiceImplinstantiations have been systematically updated to include the newmockLocalConfigServiceparameter, ensuring test compatibility with the updated service signature.Also applies to: 127-129, 146-146, 161-161, 176-176, 206-206, 236-236, 251-251, 257-257, 279-279, 294-294, 306-306, 323-323, 342-342, 356-356, 368-368, 380-380, 390-390, 398-398, 406-406, 415-415, 438-438, 461-461, 476-476, 491-491, 506-506, 517-517, 527-527, 536-536, 546-546, 557-557, 578-578, 591-591
| @Test | ||
| public void testTriggerJob_SuccessfulDeletion_ReturnsTrue() { | ||
| int jobId = 1; | ||
| long currentTimeMillis = System.currentTimeMillis(); | ||
|
|
||
| boolean result = registrationDeletionJob.triggerJob(jobId); | ||
|
|
||
| assertTrue(result); | ||
| verify(packetService).deleteRegistrationPackets(); | ||
| verify(registrationDeletionJob).logJobTransaction(jobId, TimeUnit.MILLISECONDS.toSeconds(currentTimeMillis)); | ||
| } |
There was a problem hiding this comment.
Fix the timestamp verification logic.
The test captures currentTimeMillis at line 39 but then verifies logJobTransaction is called with TimeUnit.MILLISECONDS.toSeconds(currentTimeMillis) at line 45. However, the actual elapsed time calculation in the production code will differ from this pre-captured value, causing the verification to fail.
Consider either:
- Using
any()matcher for the timestamp parameter if exact timing isn't critical - Using argument captor to verify the timestamp is reasonable (e.g., within expected range)
- Mocking the time source if precise timing verification is required
🔧 Proposed fix
@Test
public void testTriggerJob_SuccessfulDeletion_ReturnsTrue() {
int jobId = 1;
- long currentTimeMillis = System.currentTimeMillis();
boolean result = registrationDeletionJob.triggerJob(jobId);
assertTrue(result);
verify(packetService).deleteRegistrationPackets();
- verify(registrationDeletionJob).logJobTransaction(jobId, TimeUnit.MILLISECONDS.toSeconds(currentTimeMillis));
+ verify(registrationDeletionJob).logJobTransaction(eq(jobId), anyLong());
}📝 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.
| @Test | |
| public void testTriggerJob_SuccessfulDeletion_ReturnsTrue() { | |
| int jobId = 1; | |
| long currentTimeMillis = System.currentTimeMillis(); | |
| boolean result = registrationDeletionJob.triggerJob(jobId); | |
| assertTrue(result); | |
| verify(packetService).deleteRegistrationPackets(); | |
| verify(registrationDeletionJob).logJobTransaction(jobId, TimeUnit.MILLISECONDS.toSeconds(currentTimeMillis)); | |
| } | |
| @Test | |
| public void testTriggerJob_SuccessfulDeletion_ReturnsTrue() { | |
| int jobId = 1; | |
| boolean result = registrationDeletionJob.triggerJob(jobId); | |
| assertTrue(result); | |
| verify(packetService).deleteRegistrationPackets(); | |
| verify(registrationDeletionJob).logJobTransaction(eq(jobId), anyLong()); | |
| } |
🤖 Prompt for AI Agents
In
@android/clientmanager/src/test/java/io/mosip/registration/clientmanager/jobs/RegistrationDeletionJobTest.java
around lines 36 - 46, The timestamp verification in
testTriggerJob_SuccessfulDeletion_ReturnsTrue is brittle because
currentTimeMillis is captured before triggerJob and the production code computes
elapsed time differently; update the test to avoid exact-match timing: replace
the strict verification of logJobTransaction(jobId,
TimeUnit.MILLISECONDS.toSeconds(currentTimeMillis)) with either
verify(registrationDeletionJob).logJobTransaction(eq(jobId), anyLong()) or use
an ArgumentCaptor<Long> to capture the timestamp and assert it falls within an
expected range, or alternatively mock the time source used by
registrationDeletionJob so you can assert an exact value; ensure
packetService.deleteRegistrationPackets() is still verified and keep references
to registrationDeletionJob.triggerJob and logJobTransaction in your changes.
| import io.mosip.registration.clientmanager.dto.http.CACertificateResponseDto; | ||
| import io.mosip.registration.keymanager.exception.KeymanagerServiceException; |
There was a problem hiding this comment.
Avoid ambiguous CACertificateResponseDto imports (two classes with same name).
You import io.mosip.registration.keymanager.dto.* (Line 22) and also io.mosip.registration.clientmanager.dto.http.CACertificateResponseDto (Line 23-24). Since both packages contain a CACertificateResponseDto, this is easy to misuse and hard to read.
Recommendation: remove the wildcard import and explicitly import only what you use, or fully-qualify one of the types consistently.
🤖 Prompt for AI Agents
In
@android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/MasterDataServiceImplTest.java
around lines 23 - 24, In MasterDataServiceImplTest, remove the ambiguous
wildcard import io.mosip.registration.keymanager.dto.* and replace it with
explicit imports for only the types you actually use (or alternatively
fully-qualify one of the CACertificateResponseDto references); ensure you keep a
single, unambiguous import for CACertificateResponseDto (either
io.mosip.registration.clientmanager.dto.http.CACertificateResponseDto or
io.mosip.registration.keymanager.dto.CACertificateResponseDto) and update any
references in the test accordingly so there is no collision between the two
classes.
| import okhttp3.Headers; | ||
| import okhttp3.Protocol; | ||
| import okhttp3.Request; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if file exists and read it to verify imports and usage
file_path="android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/MasterDataServiceImplTest.java"
if [ ! -f "$file_path" ]; then
echo "File not found: $file_path"
exit 1
fi
echo "=== Checking for okhttp3 imports ==="
rg "import okhttp3\.(Headers|Protocol|Request)" "$file_path" -n
echo ""
echo "=== Checking for usage of Headers, Protocol, Request classes ==="
# Search for actual usage (excluding import statements)
rg "\b(Headers|Protocol|Request)\b" "$file_path" -n | grep -v "^[0-9]*:\s*import"
echo ""
echo "=== Full file line count and structure ==="
wc -l "$file_path"
echo ""
echo "=== Lines around 51-53 ==="
sed -n '45,60p' "$file_path"Repository: mosip/android-registration-client
Length of output: 875
Remove unused okhttp3 imports.
The imports okhttp3.Headers, okhttp3.Protocol, and okhttp3.Request (lines 51-53) are not used anywhere in this test file. These unused imports can cause build failures with strict lint settings. Only okhttp3.ResponseBody is used and should be retained.
🤖 Prompt for AI Agents
In
@android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/MasterDataServiceImplTest.java
around lines 51 - 53, Remove the unused okhttp3 imports Headers, Protocol, and
Request from MasterDataServiceImplTest.java; keep only the used
okhttp3.ResponseBody import so tests compile with strict lint—locate the import
block containing okhttp3.Headers, okhttp3.Protocol, and okhttp3.Request and
delete those three entries.
Signed-off-by: sachin.sp <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/PreCheckValidatorServiceImplTest.java (1)
456-472:⚠️ Potential issue | 🟡 MinorTest name and comment are misleading; this test duplicates
testValidateSyncStatus_JobWithNullApiName_Skipped.The test is named
testValidateSyncStatus_JobWithNullId_Skippedand the comment says "Create job with null ID", but the implementation creates aSyncJobDefwith a validJOB_ID_1and then setsapiNametonull. This makes the test identical totestValidateSyncStatus_JobWithNullApiName_Skippedat lines 474-490.If the intent was to test a job with a null ID, the new constructor
SyncJobDef(String id)would need to acceptnull. Otherwise, consider removing this duplicate test or renaming/refactoring to test a distinct scenario.android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/PreRegZipHandlingServiceImplTest.java (1)
401-419:⚠️ Potential issue | 🟡 MinorTest lacks assertion for the expected exception.
The test name indicates it should verify an exception is thrown when entry count exceeds threshold, but there's no assertion or
expectedannotation. The test will pass regardless of whether the exception is thrown. Additionally,new byte[5]creates an invalid zip, not a zip with entries exceeding the threshold.🐛 Proposed fix
`@Test` public void test_throws_exception_when_entry_count_exceeds_threshold() throws Exception { ReflectionTestUtils.setField(service, "registrationService", mockRegistrationService); ReflectionTestUtils.setField(service, "applicantValidDocumentDao", mockApplicantValidDocumentDao); ReflectionTestUtils.setField(service, "THRESHOLD_ENTRIES", 2); ReflectionTestUtils.setField(service, "THRESHOLD_SIZE", 200000L); ReflectionTestUtils.setField(service, "THRESHOLD_RATIO", 10); Map<String, DocumentDto> documentsMap = new HashMap<>(); Map<String, Object> demographicsMap = new HashMap<>(); when(mockRegistrationService.getRegistrationDto()).thenReturn(regDto); when(regDto.getDocuments()).thenReturn(documentsMap); when(regDto.getDemographics()).thenReturn(demographicsMap); - byte[] zipFile = new byte[5]; - - service.extractPreRegZipFile(zipFile); + // Create a zip file with more entries than the threshold + byte[] zipFile = createZipFileWithEntries(5, 100); // 5 entries > threshold of 2 + + assertThrows(RegBaseUncheckedException.class, () -> { + service.extractPreRegZipFile(zipFile); + }); }android/packetmanager/src/test/java/io/mosip/registration/packetmanager/service/PosixAdapterServiceImplTest.java (1)
3-40:⚠️ Potential issue | 🟡 MinorFix duplicate mockito-inline version declarations in android/packetmanager/build.gradle.
The test file properly uses
MockedStatic, andmockito-inlineis present as a test dependency. However,mockito-inlineis declared twice with conflicting versions (4.11.0 at line 63 and 5.0.0 at line 92). Remove the older 4.11.0 declaration to avoid build ambiguity. Additionally, verify thatmockito-core:5.1.1andmockito-inline:5.0.0are compatible (minor version mismatch between core and inline).
🤖 Fix all issues with AI agents
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java`:
- Around line 317-321: In RegistrationServiceImpl, the null/empty check for
containerPath is wrong and can NPE; change the condition in the block that sets
registrationDto.packetId to ensure containerPath is non-null AND non-empty (use
&& instead of ||), or preferably use a safe utility like
StringUtils.isNotBlank(containerPath) (or containerPath != null &&
!containerPath.trim().isEmpty()) before calling substring/trim; then compute
packetId from containerPath, strip ".zip", and call
this.registrationDto.setPacketId(packetId).
In
`@android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/PreRegZipHandlingServiceImplTest.java`:
- Around line 367-374: The test contains duplicated verify calls for
mockRegistrationDto.addWithoutDocument(...),
mockRegistrationDto.getDemographics(), and
mockIdentitySchemaRepository.getAllFieldSpec(...); remove the repeated block so
each verification appears only once in PreRegZipHandlingServiceImplTest (look
for the verify calls referencing mockRegistrationDto and
mockIdentitySchemaRepository and remove the second identical group), leaving a
single set of verifies for addWithoutDocument, getDemographics, and
getAllFieldSpec.
In
`@android/packetmanager/src/test/java/io/mosip/registration/packetmanager/service/PosixAdapterServiceImplTest.java`:
- Around line 1623-1626: The tests set global Robolectric shadow state via
ShadowEnvironment.setExternalStorageState(...) but never restore it, causing
cross-test leakage; update the two tests (the ones that call
ShadowEnvironment.setExternalStorageState() at the shown diffs) to capture the
prior state(s) (call ShadowEnvironment.getExternalStorageState() and
ShadowEnvironment.getExternalStorageState(baseDir) as applicable) before
changing them and restore those values in a finally block (or restore them in
the existing tearDown() method) by calling
ShadowEnvironment.setExternalStorageState(...) with the saved values so the
global shadow state is always reset after each test.
🧹 Nitpick comments (10)
android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/PreCheckValidatorServiceImplTest.java (1)
42-42: Consider documenting the rationale for usingMockitoJUnitRunner.Silent.The
Silentrunner suppresses warnings about unnecessary stubbings. While this reduces test noise, it can also mask tests that have stale or unused mock configurations. If specific tests require unused stubs for readability/setup consistency, consider using@MockitoSettings(strictness = Strictness.LENIENT)on individual tests instead for finer control.android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java (1)
252-253: Address commented-out try-finally block or remove the comments.The commented-out
tryandfinallyblocks suggest incomplete refactoring. If the try-finally is no longer needed, remove the comments entirely to avoid confusion. If it's intentionally deferred, consider adding a TODO with context.Also applies to: 372-374
android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/RegistrationServiceImplTest.java (2)
126-129: Consider cleaning up the temporary directory after tests.The temporary
packetsdirectory is created but never cleaned up. While this may not cause immediate issues, it could leave test artifacts on the filesystem. Consider adding cleanup intearDown().♻️ Proposed cleanup in tearDown
`@After` public void tearDown() { if (storageUtilsMock != null) { storageUtilsMock.close(); } + // Clean up temp directory + File packetDir = new File(System.getProperty("java.io.tmpdir"), "packets"); + if (packetDir.exists()) { + packetDir.delete(); + } }
75-75: Note: MockitoJUnitRunner.Silent suppresses unnecessary stubbing warnings.Using
Silent.classsuppresses warnings for unused stubs. While this is acceptable for reducing noise in tests with lenient setup, be aware it may also hide legitimate stubbing issues. Ensure stubs are intentionally lenient where needed.android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/PreRegZipHandlingServiceImplTest.java (5)
308-317: Use try-with-resources forMockedStaticto prevent resource leaks.If an assertion or other statement throws before
.close()is called, the static mock will leak and potentially affect subsequent tests. This pattern is repeated intest_initPreRegAdapter_externalStorageNotMountedandtest_initPreRegAdapter_externalStorageMounted.♻️ Suggested refactor using try-with-resources
`@Test` public void test_init_prereg_adapter_when_external_storage_not_mounted() { - MockedStatic<Environment> mockedEnvironment = Mockito.mockStatic(Environment.class); - mockedEnvironment.when(Environment::getExternalStorageState).thenReturn("unmounted"); - - ReflectionTestUtils.invokeMethod(service, "initPreRegAdapter", mockContext); - - assertEquals(mockContext, ReflectionTestUtils.getField(service, "appContext")); - - mockedEnvironment.close(); + try (MockedStatic<Environment> mockedEnvironment = Mockito.mockStatic(Environment.class)) { + mockedEnvironment.when(Environment::getExternalStorageState).thenReturn("unmounted"); + + ReflectionTestUtils.invokeMethod(service, "initPreRegAdapter", mockContext); + + assertEquals(mockContext, ReflectionTestUtils.getField(service, "appContext")); + } }
780-785: Clarify test intent and add proper assertion.The empty catch block with comment "Should not throw, just print error" doesn't verify the expected behavior. If the method is expected to handle the error gracefully, explicitly assert what should happen (e.g., verify error logging, return value, or state).
♻️ Suggested improvement
try { service.storePreRegPacketToDisk("id", new byte[] { 1, 2, 3 }, new CenterMachineDto()); + // If we reach here, verify expected fallback behavior + // e.g., verify logging occurred or return value is as expected } catch (Exception e) { - // Should not throw, just print error + fail("Expected method to handle error gracefully without throwing: " + e.getMessage()); }
981-1018: Consider addressing non-deterministic test behavior.The comments acknowledge that test behavior depends on
HashMapiteration order, which is non-deterministic. This could lead to flaky tests. Consider using aLinkedHashMapin the production code (if appropriate) or restructuring the test to be deterministic.
717-723: Apply try-with-resources pattern here as well.Same issue as noted earlier -
MockedStaticshould use try-with-resources to ensure cleanup.♻️ Suggested refactor
`@Test` public void test_initPreRegAdapter_externalStorageNotMounted() { - MockedStatic<Environment> mockedEnv = Mockito.mockStatic(Environment.class); - mockedEnv.when(Environment::getExternalStorageState).thenReturn("unmounted"); - ReflectionTestUtils.invokeMethod(service, "initPreRegAdapter", mockContext); - assertEquals(mockContext, ReflectionTestUtils.getField(service, "appContext")); - mockedEnv.close(); + try (MockedStatic<Environment> mockedEnv = Mockito.mockStatic(Environment.class)) { + mockedEnv.when(Environment::getExternalStorageState).thenReturn("unmounted"); + ReflectionTestUtils.invokeMethod(service, "initPreRegAdapter", mockContext); + assertEquals(mockContext, ReflectionTestUtils.getField(service, "appContext")); + } }
962-968: Apply try-with-resources pattern here as well.Same issue as noted earlier -
MockedStaticshould use try-with-resources to ensure cleanup.♻️ Suggested refactor
`@Test` public void test_initPreRegAdapter_externalStorageMounted() { - MockedStatic<Environment> mockedEnv = Mockito.mockStatic(Environment.class); - mockedEnv.when(Environment::getExternalStorageState).thenReturn(Environment.MEDIA_MOUNTED); - ReflectionTestUtils.invokeMethod(service, "initPreRegAdapter", mockContext); - assertEquals(mockContext, ReflectionTestUtils.getField(service, "appContext")); - mockedEnv.close(); + try (MockedStatic<Environment> mockedEnv = Mockito.mockStatic(Environment.class)) { + mockedEnv.when(Environment::getExternalStorageState).thenReturn(Environment.MEDIA_MOUNTED); + ReflectionTestUtils.invokeMethod(service, "initPreRegAdapter", mockContext); + assertEquals(mockContext, ReflectionTestUtils.getField(service, "appContext")); + } }android/clientmanager/src/test/java/io/mosip/registration/clientmanager/util/UserInterfaceHelperServiceTest.java (1)
28-28: Consider switching toMockitoJUnitRunner.StrictStubs.The
Silentmode suppresses stubbing warnings and doesn't detect unused stubs (visible in this file as the unused@MockfieldsmockMissingImageandmockResultBitmap).StrictStubswill help keep tests clean by catching unused stubbings and argument mismatches, which improves debugging.Suggested change
-@RunWith(MockitoJUnitRunner.Silent.class) +@RunWith(MockitoJUnitRunner.StrictStubs.class)
| if (containerPath != null || !containerPath.trim().isEmpty()) { | ||
| String packetId = containerPath.substring(containerPath.lastIndexOf("/") + 1); | ||
| packetId = packetId.replace(".zip", ""); | ||
| this.registrationDto.setPacketId(packetId); | ||
| packetId = packetId.replace(".zip", ""); | ||
| this.registrationDto.setPacketId(packetId); | ||
| } |
There was a problem hiding this comment.
Critical bug: NPE risk and incorrect condition logic.
The condition containerPath != null || !containerPath.trim().isEmpty() is logically flawed:
- If
containerPathisnull, the first part isfalse, so the second part is evaluated, causing aNullPointerExceptionon.trim(). - The
||operator means the block executes if either condition is true, but ifcontainerPathis not null, it will always enter regardless of being empty.
The intended logic should use && to ensure containerPath is both non-null AND non-empty.
🐛 Proposed fix
- if (containerPath != null || !containerPath.trim().isEmpty()) {
+ if (containerPath != null && !containerPath.trim().isEmpty()) {📝 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 (containerPath != null || !containerPath.trim().isEmpty()) { | |
| String packetId = containerPath.substring(containerPath.lastIndexOf("/") + 1); | |
| packetId = packetId.replace(".zip", ""); | |
| this.registrationDto.setPacketId(packetId); | |
| packetId = packetId.replace(".zip", ""); | |
| this.registrationDto.setPacketId(packetId); | |
| } | |
| if (containerPath != null && !containerPath.trim().isEmpty()) { | |
| String packetId = containerPath.substring(containerPath.lastIndexOf("/") + 1); | |
| packetId = packetId.replace(".zip", ""); | |
| this.registrationDto.setPacketId(packetId); | |
| } |
🤖 Prompt for AI Agents
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java`
around lines 317 - 321, In RegistrationServiceImpl, the null/empty check for
containerPath is wrong and can NPE; change the condition in the block that sets
registrationDto.packetId to ensure containerPath is non-null AND non-empty (use
&& instead of ||), or preferably use a safe utility like
StringUtils.isNotBlank(containerPath) (or containerPath != null &&
!containerPath.trim().isEmpty()) before calling substring/trim; then compute
packetId from containerPath, strip ".zip", and call
this.registrationDto.setPacketId(packetId).
| verify(mockRegistrationDto, never()).addWithoutDocument(anyString(), anyString(), anyString(), anyString(), | ||
| anyString()); | ||
| verify(mockRegistrationDto, never()).getDemographics(); | ||
| verify(mockIdentitySchemaRepository, never()).getAllFieldSpec(any(Context.class), anyDouble()); | ||
| verify(mockRegistrationDto, never()).addWithoutDocument(anyString(), anyString(), anyString(), anyString(), anyString()); | ||
| verify(mockRegistrationDto, never()).addWithoutDocument(anyString(), anyString(), anyString(), anyString(), | ||
| anyString()); | ||
| verify(mockRegistrationDto, never()).getDemographics(); | ||
| verify(mockIdentitySchemaRepository, never()).getAllFieldSpec(any(Context.class), anyDouble()); |
There was a problem hiding this comment.
Remove duplicate verification calls.
Lines 371-374 are exact duplicates of lines 367-370. This appears to be a copy-paste error and should be removed.
🐛 Proposed fix
verify(mockRegistrationDto, never()).addWithoutDocument(anyString(), anyString(), anyString(), anyString(),
anyString());
verify(mockRegistrationDto, never()).getDemographics();
verify(mockIdentitySchemaRepository, never()).getAllFieldSpec(any(Context.class), anyDouble());
- verify(mockRegistrationDto, never()).addWithoutDocument(anyString(), anyString(), anyString(), anyString(),
- anyString());
- verify(mockRegistrationDto, never()).getDemographics();
- verify(mockIdentitySchemaRepository, never()).getAllFieldSpec(any(Context.class), anyDouble());
}📝 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.
| verify(mockRegistrationDto, never()).addWithoutDocument(anyString(), anyString(), anyString(), anyString(), | |
| anyString()); | |
| verify(mockRegistrationDto, never()).getDemographics(); | |
| verify(mockIdentitySchemaRepository, never()).getAllFieldSpec(any(Context.class), anyDouble()); | |
| verify(mockRegistrationDto, never()).addWithoutDocument(anyString(), anyString(), anyString(), anyString(), anyString()); | |
| verify(mockRegistrationDto, never()).addWithoutDocument(anyString(), anyString(), anyString(), anyString(), | |
| anyString()); | |
| verify(mockRegistrationDto, never()).getDemographics(); | |
| verify(mockIdentitySchemaRepository, never()).getAllFieldSpec(any(Context.class), anyDouble()); | |
| verify(mockRegistrationDto, never()).addWithoutDocument(anyString(), anyString(), anyString(), anyString(), | |
| anyString()); | |
| verify(mockRegistrationDto, never()).getDemographics(); | |
| verify(mockIdentitySchemaRepository, never()).getAllFieldSpec(any(Context.class), anyDouble()); |
🤖 Prompt for AI Agents
In
`@android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/PreRegZipHandlingServiceImplTest.java`
around lines 367 - 374, The test contains duplicated verify calls for
mockRegistrationDto.addWithoutDocument(...),
mockRegistrationDto.getDemographics(), and
mockIdentitySchemaRepository.getAllFieldSpec(...); remove the repeated block so
each verification appears only once in PreRegZipHandlingServiceImplTest (look
for the verify calls referencing mockRegistrationDto and
mockIdentitySchemaRepository and remove the second identical group), leaving a
single set of verifies for addWithoutDocument, getDemographics, and
getAllFieldSpec.
| // Set external storage to mounted for baseDir so getExternalStorageState(baseDir) returns MEDIA_MOUNTED | ||
| ShadowEnvironment.setExternalStorageState(Environment.MEDIA_MOUNTED); | ||
| ShadowEnvironment.setExternalStorageState(baseDir, Environment.MEDIA_MOUNTED); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the test file
find . -type f -name "PosixAdapterServiceImplTest.java" 2>/dev/nullRepository: mosip/android-registration-client
Length of output: 191
🏁 Script executed:
# Read the test file around the specified lines with context
if [ -f "android/packetmanager/src/test/java/io/mosip/registration/packetmanager/service/PosixAdapterServiceImplTest.java" ]; then
echo "=== Lines 1610-1660 (context around 1623-1626 and 1648-1651) ==="
sed -n '1610,1660p' "android/packetmanager/src/test/java/io/mosip/registration/packetmanager/service/PosixAdapterServiceImplTest.java" | cat -n
fiRepository: mosip/android-registration-client
Length of output: 3142
🏁 Script executed:
# Get full test method bodies and check for cleanup
sed -n '1620,1670p' "android/packetmanager/src/test/java/io/mosip/registration/packetmanager/service/PosixAdapterServiceImplTest.java" | cat -nRepository: mosip/android-registration-client
Length of output: 3254
🏁 Script executed:
# Search for `@After`, tearDown, and cleanup methods in the test class
grep -n "@After\|tearDown\|ShadowEnvironment" "android/packetmanager/src/test/java/io/mosip/registration/packetmanager/service/PosixAdapterServiceImplTest.java" | head -30Repository: mosip/android-registration-client
Length of output: 525
🏁 Script executed:
# Read the tearDown method
sed -n '87,110p' "android/packetmanager/src/test/java/io/mosip/registration/packetmanager/service/PosixAdapterServiceImplTest.java" | cat -nRepository: mosip/android-registration-client
Length of output: 915
🏁 Script executed:
# Also search for any other ShadowEnvironment resets in the tearDown or elsewhere
sed -n '87,150p' "android/packetmanager/src/test/java/io/mosip/registration/packetmanager/service/PosixAdapterServiceImplTest.java" | cat -nRepository: mosip/android-registration-client
Length of output: 3051
🏁 Script executed:
# Search for other tests that use getExternalStorageState or depend on storage state
grep -n "getExternalStorageState\|Environment\\.MEDIA" "android/packetmanager/src/test/java/io/mosip/registration/packetmanager/service/PosixAdapterServiceImplTest.java"Repository: mosip/android-registration-client
Length of output: 744
🏁 Script executed:
# Get a count of total test methods to understand scope
grep -c "@Test" "android/packetmanager/src/test/java/io/mosip/registration/packetmanager/service/PosixAdapterServiceImplTest.java"Repository: mosip/android-registration-client
Length of output: 79
🏁 Script executed:
# Check the test around line 1450 that patches getExternalStorageState
sed -n '1440,1470p' "android/packetmanager/src/test/java/io/mosip/registration/packetmanager/service/PosixAdapterServiceImplTest.java" | cat -nRepository: mosip/android-registration-client
Length of output: 1731
🏁 Script executed:
# Check if there are tests that run AFTER the two problematic tests and might be affected
# Get the line numbers and test method names for context
grep -n "@Test" "android/packetmanager/src/test/java/io/mosip/registration/packetmanager/service/PosixAdapterServiceImplTest.java" | tail -10Repository: mosip/android-registration-client
Length of output: 226
🏁 Script executed:
# Check if there are any ShadowEnvironment reset/cleanup patterns elsewhere
grep -rn "reset\|Reset\|clearProperty" "android/packetmanager/src/test/java/io/mosip/registration/packetmanager/service/PosixAdapterServiceImplTest.java" | head -10Repository: mosip/android-registration-client
Length of output: 205
Add cleanup for ShadowEnvironment state to prevent cross-test leakage.
ShadowEnvironment.setExternalStorageState() maintains global Robolectric shadow state that persists across tests. The current tearDown() method only cleans up file resources but does not restore the external storage state, causing tests running after these methods to inherit the MEDIA_MOUNTED state. Use a try-finally block to restore the original state in both test methods:
Suggested pattern (apply to both tests)
- ShadowEnvironment.setExternalStorageState(Environment.MEDIA_MOUNTED);
- ShadowEnvironment.setExternalStorageState(baseDir, Environment.MEDIA_MOUNTED);
+ String originalState = Environment.getExternalStorageState();
+ try {
+ ShadowEnvironment.setExternalStorageState(Environment.MEDIA_MOUNTED);
+ ShadowEnvironment.setExternalStorageState(baseDir, Environment.MEDIA_MOUNTED);
+ // ...
+ } finally {
+ ShadowEnvironment.setExternalStorageState(originalState);
+ ShadowEnvironment.setExternalStorageState(baseDir, originalState);
+ }Also applies to: 1648-1651
🤖 Prompt for AI Agents
In
`@android/packetmanager/src/test/java/io/mosip/registration/packetmanager/service/PosixAdapterServiceImplTest.java`
around lines 1623 - 1626, The tests set global Robolectric shadow state via
ShadowEnvironment.setExternalStorageState(...) but never restore it, causing
cross-test leakage; update the two tests (the ones that call
ShadowEnvironment.setExternalStorageState() at the shown diffs) to capture the
prior state(s) (call ShadowEnvironment.getExternalStorageState() and
ShadowEnvironment.getExternalStorageState(baseDir) as applicable) before
changing them and restore those values in a finally block (or restore them in
the existing tearDown() method) by calling
ShadowEnvironment.setExternalStorageState(...) with the saved values so the
global shadow state is always reset after each test.
1 participant
Summary by CodeRabbit
Tests
Refactor
Bug Fixes