MOSIP-25973 : Added testcases to improve coverage#804
Merged
Conversation
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds extensive unit and reflection-based tests across BioService, MosipDeviceSpecificationFactory/Helper/Providers, and SoftwareUpdateHandler, including embedded HttpServer tests, ApplicationContext mutation helpers, and PowerMock/Whitebox-based private-method coverage. ChangesMDM Biometric and Device Tests
Software Update and DB Upgrade Tests
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@registration/registration-services/src/test/java/io/mosip/registration/bio/service/test/BioServiceTest.java`:
- Line 561: Tests in BioServiceTest mutate the global ApplicationContext.map()
(e.g., removal of RegistrationConstants.QUALITY_CHECK_WITH_SDK) which can leak
state across tests; update the test class to reset any keys it changes by adding
an `@After` (or `@AfterEach`) cleanup method that restores or clears those specific
keys, or wrap each mutation in a try-finally that restores the original value,
ensuring ApplicationContext.map() is returned to its prior state after each
test; reference the mutations around ApplicationContext.map() and keys like
RegistrationConstants.QUALITY_CHECK_WITH_SDK when implementing the cleanup.
In
`@registration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecification_SBI_1_0_ProviderImplTest.java`:
- Line 678: The test constructs an MDMRequestDto with a modality typo
"FINGERPRINT_SLAB_LEFT" which should be the slap modality; update the
MDMRequestDto instantiation in MosipDeviceSpecification_SBI_1_0_ProviderImplTest
(the line creating new MDMRequestDto(...)) to use the correct modality string
"FINGERPRINT_SLAP_LEFT" so the rCapture happy-path maps to the intended slap
behavior.
In
`@registration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecificationFactoryTest.java`:
- Around line 56-65: The test initializes the ApplicationContext singleton in
setupApplicationContext() by reflectively setting the private static field
applicationContext but never resets it, risking state leakage between tests; add
an `@AfterAll` teardown method (e.g., tearDownApplicationContext) that
reflectively clears or nulls the ApplicationContext.applicationContext field or
invokes a reset/clear method on ApplicationContext to restore clean state
between test classes so internal maps mutated by tests are cleared.
In
`@registration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecificationHelperTest.java`:
- Around line 223-226: The test
getMDMConnectionTimeout_allNull_returnsDefault10000 is order-dependent because
it assumes ApplicationContext.applicationMap has no timeout keys; modify the
test to make it deterministic by explicitly clearing or resetting
ApplicationContext.applicationMap (or removing the specific timeout keys) before
calling MosipDeviceSpecificationHelper.getMDMConnectionTimeout("MOSIPDINFO"),
and if other tests rely on the map, save and restore the original map state
around the test so the global state is not mutated for other tests.
- Around line 364-379: withApplicationMap currently only removes the inserted
key on success and will skip cleanup if op.apply throws and will also clobber
any pre-existing value; change the contract so the op returns both the inserted
key and the previous value (e.g., have MapOp.apply return a
Map.Entry<String,Object> or a small KeyPrev record instead of String), then in
withApplicationMap always run a finally block that uses the returned previous
value to restore state: if previous==null remove(key) else put(key, previous).
Update the MapOp interface and all callers (tests) accordingly so cleanup always
runs and original values are restored even when op.apply throws.
In
`@registration/registration-services/src/test/java/io/mosip/registration/test/update/SoftwareUpdateHandlerTest.java`:
- Around line 84-88: The tests delete and create files in the repository root
and src/test/resources/sql which makes them order-dependent and unsafe; change
the tests in SoftwareUpdateHandlerTest (the doSoftwareUpgrade test cases) to use
an isolated temporary directory per test (e.g., JUnit TemporaryFolder or
Files.createTempDirectory) for creating MANIFEST.MF and SQL artifacts and pass
that temp path into the code-under-test or mock the file-system access, and
update the cleanup() method to only remove files inside that temp directory (or
rely on TemporaryFolder to auto-clean) instead of deleting "MANIFEST.MF" in the
project root.
- Around line 273-283: The tests currently only assert "no exception" — update
each named test to verify the intended side effects: in
doSoftwareUpgrade_backupFails_doesNotPropagateException, after calling
softwareUpdateHandler.doSoftwareUpgrade() verify interactions such as
Mockito.verify(globalParamService).update(...) and that FileUtils.copyFile (the
static mock) was/was-not invoked as appropriate given the simulated backup
failure; in doSoftwareUpgrade_withManifestEntries_downloadsFiles assert
manifest.getMainAttributes() was used and verify the download/extract behavior
(verify the method that performs downloads or the static FileUtils.copyFile
calls); in updateDerbyDB_withAllSqlFailing_triggersDbRollback call
softwareUpdateHandler.updateDerbyDB(...) and verify a rollback on the DB
interaction (e.g., verify connection.rollback() or the transaction manager
rollback mock was invoked); and in
doSoftwareUpgrade_withSuppressedCopyFile_callsUpdate assert that
globalParamService.update(...) was called with expected args when
FileUtils.copyFile is suppressed. Use the existing mocks (manifest,
globalParamService, static FileUtils) and Mockito.verify calls to make the
asserted side effects explicit.
- Around line 202-217: The test
executeSqlFile_multipleVersions_executesAllInOrder currently only asserts
atLeast(2) calls to globalParamService.update but promises ordering; change the
assertion to verify call order by creating an InOrder for the mock
(Mockito.inOrder(globalParamService)) and then verify() the sequential update
calls against software version values (e.g., verify first update invoked with
RegistrationConstants.SERVICES_VERSION_KEY and a value containing "0.10.0", then
verify the next update with a value containing "0.11.0") so the test asserts the
updates happen in the expected order after calling
softwareUpdateHandler.executeSqlFile.
- Around line 213-214: The test uses Assert.assertSame to compare the SQL
success message constant and the actual message
(RegistrationConstants.SQL_EXECUTION_SUCCESS vs
response.getSuccessResponseDTO().getMessage()), which checks object identity;
change these to Assert.assertEquals for value equality at the three assertions
that compare RegistrationConstants.SQL_EXECUTION_SUCCESS with
response.getSuccessResponseDTO().getMessage() (occurrences around the assertions
currently using Assert.assertSame). Ensure all three instances (the assertions
comparing the constant and response message) are updated to Assert.assertEquals.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cba1fbb9-0040-49a8-a438-9d4190ae7ec1
📒 Files selected for processing (6)
registration/registration-services/src/test/java/io/mosip/registration/bio/service/test/BioServiceTest.javaregistration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecificationFactoryTest.javaregistration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecificationHelperTest.javaregistration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecification_092_ProviderImplTest.javaregistration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecification_SBI_1_0_ProviderImplTest.javaregistration/registration-services/src/test/java/io/mosip/registration/test/update/SoftwareUpdateHandlerTest.java
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
registration/registration-services/src/test/java/io/mosip/registration/test/update/ClientSetupValidatorTest.java (1)
152-153: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider updating existing tests to use the class-level pattern for static fields.
Lines 152–153, 169–170, and 185–186 set static fields (
serverManifest,localManifest) using the instance formReflectionTestUtils.setField(clientSetupValidator, ...). While this works, it is inconsistent with the new pattern introduced at lines 120 and 127–129, which correctly usesReflectionTestUtils.setField(ClientSetupValidator.class, ...)for static fields.For clarity and consistency, consider refactoring these older tests to use the class form.
♻️ Example refactor for consistency
Manifest manifest = getManifest(); -ReflectionTestUtils.setField(clientSetupValidator, "serverManifest", manifest); -ReflectionTestUtils.setField(clientSetupValidator, "localManifest", manifest); +ReflectionTestUtils.setField(ClientSetupValidator.class, "serverManifest", manifest); +ReflectionTestUtils.setField(ClientSetupValidator.class, "localManifest", manifest);Apply similar changes at lines 169–170 and 185–186.
Also applies to: 169-170, 185-186
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@registration/registration-services/src/test/java/io/mosip/registration/test/update/ClientSetupValidatorTest.java` around lines 152 - 153, Update the tests to set static fields via the class-level ReflectionTestUtils API: replace instance-based ReflectionTestUtils.setField(clientSetupValidator, "serverManifest", manifest) and ReflectionTestUtils.setField(clientSetupValidator, "localManifest", manifest) with ReflectionTestUtils.setField(ClientSetupValidator.class, "serverManifest", manifest) and ReflectionTestUtils.setField(ClientSetupValidator.class, "localManifest", manifest) (apply the same change for the occurrences around lines 169–170 and 185–186) so static fields on ClientSetupValidator are consistently modified via the class reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@registration/registration-services/src/test/java/io/mosip/registration/test/update/ClientSetupValidatorTest.java`:
- Around line 87-96: The resetStaticFields() helper currently clears several
ClientSetupValidator static fields but omits the messages Stack<String>; update
resetStaticFields() to also reset the ClientSetupValidator.messages static field
(e.g., set it to a new empty Stack or null) so tests don’t see leftover entries
— locate the method resetStaticFields() in the test class and add a
ReflectionTestUtils.setField(ClientSetupValidator.class, "messages", <empty
Stack or null>) call to clear that static state.
---
Outside diff comments:
In
`@registration/registration-services/src/test/java/io/mosip/registration/test/update/ClientSetupValidatorTest.java`:
- Around line 152-153: Update the tests to set static fields via the class-level
ReflectionTestUtils API: replace instance-based
ReflectionTestUtils.setField(clientSetupValidator, "serverManifest", manifest)
and ReflectionTestUtils.setField(clientSetupValidator, "localManifest",
manifest) with ReflectionTestUtils.setField(ClientSetupValidator.class,
"serverManifest", manifest) and
ReflectionTestUtils.setField(ClientSetupValidator.class, "localManifest",
manifest) (apply the same change for the occurrences around lines 169–170 and
185–186) so static fields on ClientSetupValidator are consistently modified via
the class reference.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2aac2bd3-3f85-4797-b9de-2bd70cc7ff01
📒 Files selected for processing (1)
registration/registration-services/src/test/java/io/mosip/registration/test/update/ClientSetupValidatorTest.java
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
anushasunkada
approved these changes
May 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit