Added test cases for the coverage [MOSIP-25973]#761
Added test cases for the coverage [MOSIP-25973]#761GOKULRAJ136 wants to merge 6 commits intomosip:developfrom
Conversation
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds multiple new and expanded unit tests across registration-services, targeting device specification factories/providers, SBI provider, software update utilities/handler, mapper utilities, response signature advice, and REST client utilities. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In
`@registration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecification_092_ProviderImplTest.java`:
- Around line 163-188: The test should capture and assert the boolean returned
by provider.isDeviceAvailable so it actually verifies expected behavior; update
the test method shouldReturnTrueWhenDeviceAvailableMatches to store the result
(e.g., boolean available = provider.isDeviceAvailable(dev)) and add an assertion
that available is true (use the test framework's assertTrue/assertThat) while
keeping existing Mockito stubs for helper.getMapper, helper.buildUrl, and
helper.getHttpClientResponseEntity.
In
`@registration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecification_095_ProviderImplTest.java`:
- Around line 13-17: The test class
MosipDeviceSpecification_095_ProviderImplTest uses `@InjectMocks` on the field
mockObject (MosipDeviceSpecification_095_ProviderImpl) but never initializes
Mockito; add MockitoAnnotations.openMocks(this) in a `@Before` method to
initialize `@InjectMocks` (or remove `@InjectMocks` and instantiate mockObject with
new MosipDeviceSpecification_095_ProviderImpl() if there are no injected mocks)
so the mock injection actually occurs.
In
`@registration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecification_SBI_1_0_ProviderImplTest.java`:
- Around line 169-183: The test is only validating pre-built lists because it
uses a full mock (mockedObject) instead of invoking the real implementation;
replace the full mock with the `@InjectMocks` instance (mockObject /
MosipDeviceSpecification_SBI_1_0_ProviderImpl) so only dependencies (e.g.,
mosipDeviceSpecificationHelper, mapper) are mocked, configure those dependency
stubs (getMapper, getDeviceInfoDecoded, etc.), call
mockObject.getMdmDevices(inputDeviceInfo, port) to obtain the real result, and
assert that result matches the expected excpectedMdmBioDevices; remove the
when(mockedObject.getMdmDevices(...)) stub and ensure actualBioDevice setup and
port injection remain identical before assertion.
In
`@registration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecificationFactoryTest.java`:
- Around line 84-98: The helper invokePrivate currently calls
ReflectionUtils.findMethod without checking its return, which can cause a null
pointer; modify invokePrivate (the method that builds paramTypes, calls
ReflectionUtils.findMethod, ReflectionUtils.makeAccessible and
ReflectionUtils.invokeMethod) to check if the returned Method is null and throw
a clear exception (e.g., IllegalStateException) that includes the
target.getClass().getName(), methodName and the parameter type list, and only
call ReflectionUtils.makeAccessible/invokeMethod when Method is non-null so
failures produce a descriptive error instead of a NullPointerException.
In
`@registration/registration-services/src/test/java/io/mosip/registration/test/update/SoftwareUpdateUtilTest.java`:
- Around line 16-23: Remove the unused `@InjectMocks` field and stop calling
static utility methods on an uninitialized instance: delete the private
SoftwareUpdateUtil softwareUpdateUtil field (and its `@InjectMocks` annotation) in
SoftwareUpdateUtilTest, then update all calls like
softwareUpdateUtil.deleteUnknownJars(...) to call the static methods directly
via the class or inherited context (SoftwareUpdateUtil.deleteUnknownJars(...) or
this.deleteUnknownJars(...)); alternatively, if you intended to use Mockito
injection, initialize mocks in a `@Before` method with
MockitoAnnotations.openMocks(this) and keep the field, but prefer removing the
field since SoftwareUpdateUtil only exposes protected static methods.
- Around line 37-44: The cleanup helper deleteDir(File dir) can throw a
NullPointerException when dir.listFiles() returns null; update the
deleteDir(File dir) method to check that dir.listFiles() != null (or that
dir.isDirectory()) before iterating over files and attempting f.delete(), and
only call dir.delete() afterwards — specifically modify the deleteDir(File dir)
implementation to guard the for-loop with a null check on dir.listFiles().
In
`@registration/registration-services/src/test/java/io/mosip/registration/test/util/mastersync/MapperTest.java`:
- Around line 229-234: The test testGetLocalDateTimeValue_nullInput currently
has an unreachable assertion because it declares expected = NullPointerException
while also asserting result is null; locate the test method
testGetLocalDateTimeValue_nullInput and either remove the expected =
NullPointerException clause if the intended behavior is to return null (keep the
assertNull(result) and verify invokeMethod(null) returns null), or remove the
assertNull(result) and keep expected = NullPointerException if the intended
behavior is to throw; update the test accordingly to only assert one expected
outcome for invokeMethod.
- Around line 277-282: The test testGetLocalDateValue_nullInput declares
expected=NullPointerException but then asserts the result is null, making
assertNull unreachable; update the test to match intent by removing the
expected=NullPointerException from the `@Test` annotation so
invokeGetLocalDateValue(null) returns and the assertNull(result) executes, or
alternatively remove the assertNull if the intended behavior is to expect an
exception; locate testGetLocalDateValue_nullInput and adjust the `@Test`
annotation or assertion accordingly to make the test reachable and consistent
with invokeGetLocalDateValue.
In
`@registration/registration-services/src/test/java/io/mosip/registration/test/util/RegistrationSystemPropertiesCheckerTest.java`:
- Around line 24-30: The test getMachineId_success_realCall in
RegistrationSystemPropertiesCheckerTest is flaky because it relies on
InetAddress.getLocalHost(); remove or replace this real-host lookup test and
either consolidate it with the existing testGetMachineId() or make it
deterministic by mocking InetAddress.getLocalHost() (or refactor
RegistrationSystemPropertiesChecker to accept a HostProvider and inject a test
implementation); update assertions to use the mocked/provider result and ensure
all references to getMachineId() in tests use the deterministic provider or
consolidated test.
In
`@registration/registration-services/src/test/java/io/mosip/registration/test/util/restclient/ResponseSignatureAdviceTest.java`:
- Around line 372-388: The test method
shouldSkipValidationWhenSignRequiredIsNull in ResponseSignatureAdviceTest
declares an expected RegBaseCheckedException but then calls
Mockito.verify(signatureService, Mockito.never()).jwtVerify(...) after the call
to responseSignatureAdvice.responseSignatureValidation(joinPointMock,
linkedMap), which is unreachable; fix by either removing the post-exception
verification or restructure the test to catch the RegBaseCheckedException
(try-catch) and perform Mockito.verify(signatureService,
Mockito.never()).jwtVerify(...) inside the catch or after asserting no
exception, ensuring you reference the same joinPointMock.getArgs() setup and the
responseSignatureAdvice.responseSignatureValidation invocation and verify
signatureService.jwtVerify interaction appropriately.
In
`@registration/registration-services/src/test/java/io/mosip/registration/test/util/restclient/RestClientUtilTest.java`:
- Around line 83-102: Test uses deprecated MediaType.APPLICATION_JSON_UTF8 in
invokeForToken_buildsEntityAndReturnsMap; update the HttpHeaders setup in
RestClientUtilTest to use MediaType.APPLICATION_JSON instead (update the
HttpHeaders instance created for RequestHTTPDTO in that test), ensuring the rest
of the test (RequestHTTPDTO, dto.setHttpHeaders(...), and assertions) remain
unchanged.
🧹 Nitpick comments (7)
registration/registration-services/src/test/java/io/mosip/registration/test/update/SoftwareUpdateHandlerTest.java (1)
186-202: Consider adding more specific assertions to validate error handling behavior.The test verifies that
updateDerbyDB()returns a non-nullResponseDTOwhengetSortedVersionMappingsthrows an exception, but it doesn't verify the actual error response content. Consider asserting on the error response details to ensure the error is properly propagated.ResponseDTO response = spyHandler.updateDerbyDB(); Assert.assertNotNull(response); +Assert.assertNotNull(response.getErrorResponseDTOs());registration/registration-services/src/test/java/io/mosip/registration/test/update/SoftwareUpdateUtilTest.java (1)
102-110: Network-dependent tests may be flaky in CI environments.Tests at lines 102-110 attempt HTTP connections to
http://invalid.invalid/file. While they expectRegBaseCheckedException, network conditions (DNS resolution, timeouts) could cause inconsistent behavior or slow test execution. Consider using a mock HTTP server or a file URL pointing to a non-existent local path for more deterministic testing.registration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecification_095_ProviderImplTest.java (1)
20-66: Consider extracting repeated reflection code into a reusable helper method.The reflection pattern for invoking private methods is duplicated across multiple test methods. The helper method
invokeGetDeviceSubId(lines 61-66) is a good pattern - consider applying the same approach togetExceptionsinvocations.+ private String[] invokeGetExceptions(String[] input) throws Exception { + Method method = MosipDeviceSpecification_095_ProviderImpl.class + .getDeclaredMethod("getExceptions", String[].class); + method.setAccessible(true); + return (String[]) method.invoke(mockObject, new Object[]{input}); + } + `@Test` public void testGetExceptions_withNullInput() throws Exception { - Method method = MosipDeviceSpecification_095_ProviderImpl.class - .getDeclaredMethod("getExceptions", String[].class); - method.setAccessible(true); - - String[] result = (String[]) method.invoke(mockObject, new Object[]{null}); + String[] result = invokeGetExceptions(null); Assert.assertNull(result); }registration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecificationFactoryTest.java (1)
66-77: Test modifies static shared state without cleanup, which may cause test pollution.
testModifySelectedDeviceInfoadds entries toMosipDeviceSpecificationFactory.getAvailableDeviceInfo()andgetDeviceRegistryInfo()but doesn't clean up after itself. This could affect other tests if they run in the same JVM. Consider adding cleanup in an@AfterEachmethod.+ `@AfterEach` + public void tearDown() { + MosipDeviceSpecificationFactory.getAvailableDeviceInfo().clear(); + MosipDeviceSpecificationFactory.getDeviceRegistryInfo().clear(); + }registration/registration-services/src/test/java/io/mosip/registration/test/util/restclient/ResponseSignatureAdviceTest.java (1)
324-346: Remove commented-out code and incomplete test setup.The test
fileSignatureValidationTesthas commented-out mock setup (lines 334-338) and the actual test assertion is also commented out (line 344). Either complete the test implementation or remove it to avoid confusion.registration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecification_SBI_1_0_ProviderImplTest.java (1)
553-565: Direct field access bypasses encapsulation - use setter or constructor if available.Accessing
wrapper.deviceInfo = nulldirectly assumes package-private or public field visibility. This creates a fragile test that could break if the field visibility changes. If a setter exists, prefer using it.registration/registration-services/src/test/java/io/mosip/registration/test/util/restclient/RestClientUtilTest.java (1)
111-167: Ensure temp file cleanup even on assertion failures.
If an assertion fails mid-test, the temp file may remain. Atry/finallyensures cleanup in all cases.🧹 Suggested cleanup guard
File tempFile = File.createTempFile("restclient", ".bin"); if (tempFile.exists()) { tempFile.delete(); } + try { RequestHTTPDTO dto = new RequestHTTPDTO(); dto.setUri(new URI("https://example.com/file")); dto.setFilePath(tempFile.toPath()); dto.setHttpHeaders(new HttpHeaders()); dto.setFileEncrypted(true); @@ verify(fileSignatureRepository, atLeastOnce()).save(any(FileSignature.class)); - if (tempFile.exists()) { - tempFile.delete(); - } + } finally { + if (tempFile.exists()) { + tempFile.delete(); + } + }
...va/io/mosip/registration/test/mdm/service/MosipDeviceSpecification_092_ProviderImplTest.java
Show resolved
Hide resolved
...va/io/mosip/registration/test/mdm/service/MosipDeviceSpecification_095_ProviderImplTest.java
Show resolved
Hide resolved
...o/mosip/registration/test/mdm/service/MosipDeviceSpecification_SBI_1_0_ProviderImplTest.java
Outdated
Show resolved
Hide resolved
...rc/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecificationFactoryTest.java
Show resolved
Hide resolved
...tration-services/src/test/java/io/mosip/registration/test/update/SoftwareUpdateUtilTest.java
Show resolved
Hide resolved
...gistration-services/src/test/java/io/mosip/registration/test/util/mastersync/MapperTest.java
Outdated
Show resolved
Hide resolved
...gistration-services/src/test/java/io/mosip/registration/test/util/mastersync/MapperTest.java
Outdated
Show resolved
Hide resolved
...s/src/test/java/io/mosip/registration/test/util/RegistrationSystemPropertiesCheckerTest.java
Show resolved
Hide resolved
...es/src/test/java/io/mosip/registration/test/util/restclient/ResponseSignatureAdviceTest.java
Show resolved
Hide resolved
...on-services/src/test/java/io/mosip/registration/test/util/restclient/RestClientUtilTest.java
Show resolved
Hide resolved
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 (2)
registration/registration-services/src/test/java/io/mosip/registration/test/util/restclient/ResponseSignatureAdviceTest.java (1)
324-346:⚠️ Potential issue | 🟡 MinorIncomplete test: actual test invocation is commented out.
The
fileSignatureValidationTestmethod sets up mocks and test data but the actual method callresponseSignatureAdvice.fileSignatureValidation(joinPointMock)is commented out on line 344. This test provides no coverage and should either be completed or removed.🔧 Suggested fix: Either complete the test or remove it
- `@Test` - public void fileSignatureValidationTest() throws RegBaseCheckedException, URISyntaxException, InvalidKeySpecException, NoSuchAlgorithmException { - - RequestHTTPDTO requestHTTPDTO = new RequestHTTPDTO(); - requestHTTPDTO.setIsSignRequired(true); - requestHTTPDTO.setFileEncrypted(true); - requestHTTPDTO.setFilePath(null); - requestHTTPDTO.setUri(new URI("/v1/mosip/test")); - Object[] args = new Object[1]; - args[0] = requestHTTPDTO; -// Optional<FileSignature> fileSignature = Mockito.mock(fileSignature.getClass()); -// fileSignature - Mockito.when(joinPointMock.getArgs()).thenReturn(args); -// Mockito.when(fileSignatureRepository.findByFileName(Mockito.anyString())).thenReturn(fileSignature); -// PowerMockito.when(clientCryptoFacade.decrypt()).thenReturn(cryptoCore); - JWTSignatureVerifyResponseDto jwtSignatureResponseDto = new JWTSignatureVerifyResponseDto(); - jwtSignatureResponseDto.setSignatureValid(true); - jwtSignatureResponseDto.setMessage(SignatureConstant.VALIDATION_SUCCESSFUL); - Mockito.when(signatureService.jwtVerify(Mockito.any())).thenReturn(jwtSignatureResponseDto); - - //responseSignatureAdvice.fileSignatureValidation(joinPointMock); - - }registration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecification_SBI_1_0_ProviderImplTest.java (1)
125-160:⚠️ Potential issue | 🟠 MajorTest does not exercise the actual
stream()implementation.This test creates a full mock, stubs the
stream()method, and then only verifies the mock exists. It doesn't test any real behavior—it essentially validates Mockito works. Use the@InjectMocksinstance with only dependencies mocked, then callmockObject.stream()and assert the actual result.🐛 Proposed fix approach
`@Test` public void streamTest() throws Exception{ - MosipDeviceSpecification_SBI_1_0_ProviderImpl mockedObject = mock(MosipDeviceSpecification_SBI_1_0_ProviderImpl.class); MdmBioDevice inputBioDevice = new MdmBioDevice(); // ... setup inputBioDevice ... - OngoingStubbing<InputStream> actualStream = when(mockedObject.stream(inputBioDevice, "IRIS_DOUBLE")).thenReturn(expectedStream); - Assert.assertNotNull(actualStream.getMock()); + // Mock the helper dependencies instead + MosipDeviceSpecification_SBI_1_0_ProviderImpl spy = Mockito.spy(mockObject); + Mockito.doReturn(true).when(spy).isDeviceAvailable(inputBioDevice); + when(mosipDeviceSpecificationHelper.buildUrl(anyInt(), anyString())).thenReturn("http://localhost/stream"); + // ... setup HTTP response mock ... + + InputStream result = spy.stream(inputBioDevice, "IRIS_DOUBLE"); + assertNotNull(result);
🤖 Fix all issues with AI agents
In
`@registration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecification_SBI_1_0_ProviderImplTest.java`:
- Around line 223-243: The test testIsDeviceAvailable invokes
mockObject.isDeviceAvailable(bioDevice) but does not assert its return value;
update the test to capture the result (e.g., boolean available =
mockObject.isDeviceAvailable(bioDevice)) and assert the expected outcome (use
assertTrue/Assertions.assertTrue or assertEquals(true, available)) given the
mocked responses; reference the test method testIsDeviceAvailable, the
MdmBioDevice instance bioDevice, the mocked responses list, and the
mockObject.isDeviceAvailable(...) call when adding the assertion.
🧹 Nitpick comments (8)
registration/registration-services/src/test/java/io/mosip/registration/test/util/restclient/ResponseSignatureAdviceTest.java (2)
411-429: Consider adding assertions to verify expected behavior.The test name implies graceful handling of a missing response body, but there's no assertion verifying the expected behavior. Consider adding a verification to clarify what "graceful handling" means in this context (e.g., verifying
jwtVerifyis or isn't called, or checking return values).💡 Suggested assertion
responseSignatureAdvice.responseSignatureValidation(joinPointMock, linkedMap); + + // Verify expected behavior when response body is null + Mockito.verify(signatureService, Mockito.times(1)).jwtVerify(Mockito.any()); }
496-497: Consider moving@Ruledeclaration to the top of the class.The
TemporaryFolderrule is declared at line 496, after the test methods. For consistency with the existingMockitoRuleat line 56-57 and standard test class organization, consider moving this rule declaration near the other fields at the top of the class.registration/registration-services/src/test/java/io/mosip/registration/test/util/restclient/RestClientUtilTest.java (2)
120-176: Consider usingTemporaryFolderrule for cleaner temp file management.The test manually creates and deletes temp files using
File.createTempFile()with cleanup in the test body. If an assertion fails, the temp file may not be cleaned up. Consider using JUnit'sTemporaryFolderrule (already used in the companion test class) for automatic cleanup.Additionally, lines 140 and 166 use raw type
ResponseExtractorinstead of the parameterized form.💡 Suggested improvement
+ `@Rule` + public org.junit.rules.TemporaryFolder tempFolder = new org.junit.rules.TemporaryFolder(); + `@Test` public void downloadFile_savesSignatureAndSupportsResume() throws Exception { - File tempFile = File.createTempFile("restclient", ".bin"); - if (tempFile.exists()) { - tempFile.delete(); - } + File tempFile = tempFolder.newFile("restclient.bin"); + tempFile.delete(); // Start with non-existent file for initial download test // ... rest of test ... - org.springframework.web.client.ResponseExtractor extractor = ... + org.springframework.web.client.ResponseExtractor<?> extractor = ... - if (tempFile.exists()) { - tempFile.delete(); - } + // Cleanup handled automatically by TemporaryFolder rule }
178-182: Test name doesn't match assertions.The test is named
getHttpRequestFactory_appliesTimeoutsbut only verifies that the factory is non-null. It doesn't actually verify that the timeouts configured inApplicationContext.map()were applied to the factory. Consider either renaming togetHttpRequestFactory_returnsNonNullor adding assertions to verify timeout configuration.💡 Option: Verify timeouts via reflection
`@Test` - public void getHttpRequestFactory_appliesTimeouts() { + public void getHttpRequestFactory_returnsConfiguredFactory() { SimpleClientHttpRequestFactory factory = restClientUtil.getHttpRequestFactory(); assertNotNull(factory); + // If timeout verification is needed, use reflection or expose accessors }registration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecificationFactoryTest.java (2)
79-82: Test lacks meaningful assertions.
testAwaitTerminationAfterShutdowncalls the method but doesn't verify any behavior. Consider adding assertions or verifications to confirm expected interactions (e.g., verify the executor service methods were called).💡 Suggested improvement
`@Test` public void testAwaitTerminationAfterShutdown() { - factory.awaitTerminationAfterShutdown(mock(ExecutorService.class)); + ExecutorService mockExecutor = mock(ExecutorService.class); + factory.awaitTerminationAfterShutdown(mockExecutor); + verify(mockExecutor).shutdown(); }
243-269: Consider usingassertThrowsfor cleaner exception testing.The try-catch pattern with
fail()works but is verbose. JUnit 5'sassertThrowsprovides a cleaner approach and returns the exception for additional assertions.💡 Suggested refactor
`@Test` public void testGetSpecVersionByModality_deviceNotFound() throws Exception { String modality = "UNKNOWN"; MosipDeviceSpecificationFactory spyFactory = spy(factory); doThrow(new RegBaseCheckedException("404", "Device not found")) .when(spyFactory).getDeviceInfoByModality(modality); - try { - spyFactory.getSpecVersionByModality(modality); - fail("Expected RegBaseCheckedException"); - } catch (RegBaseCheckedException ex) { - assertEquals("404", ex.getErrorCode()); - } + RegBaseCheckedException ex = assertThrows(RegBaseCheckedException.class, + () -> spyFactory.getSpecVersionByModality(modality)); + assertEquals("404", ex.getErrorCode()); }registration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecification_092_ProviderImplTest.java (2)
63-97: Test validates device info parsing but could have more specific assertions.The test correctly sets up mocks and verifies the result is not null. Consider adding assertions on the properties of the returned
MdmBioDeviceobjects (e.g., device ID, port) for stronger validation.
124-163: Test name may not accurately reflect the scenario being tested.The test is named
shouldThrowExceptionWhenRCaptureReturnsNoBiometrics, but the setup includes biometrics in the response (line 148). If the exception is expected due to data validation failure or another reason, consider renaming the test to reflect the actual error condition being tested.
...o/mosip/registration/test/mdm/service/MosipDeviceSpecification_SBI_1_0_ProviderImplTest.java
Show resolved
Hide resolved
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
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/mdm/service/MosipDeviceSpecification_SBI_1_0_ProviderImplTest.java (1)
125-160:⚠️ Potential issue | 🟠 MajorTest does not exercise the actual implementation - it only mocks the SUT.
Similar to the issue previously flagged in
getMdmDevicesTest, this test creates a full mock ofMosipDeviceSpecification_SBI_1_0_ProviderImpland stubs itsstream()method. This only verifies that Mockito works, not that the actual implementation behaves correctly.Use the
@InjectMocksinstance (mockObject) with only dependencies mocked:🐛 Proposed fix
- MosipDeviceSpecification_SBI_1_0_ProviderImpl mockedObject = mock(MosipDeviceSpecification_SBI_1_0_ProviderImpl.class); MdmBioDevice inputBioDevice = new MdmBioDevice(); inputBioDevice.setCallbackId("http://127.0.0.1:4501"); // ... setup inputBioDevice ... - OngoingStubbing<InputStream> actualStream = when(mockedObject.stream(inputBioDevice, "IRIS_DOUBLE")).thenReturn(expectedStream); + // Mock the helper dependencies instead + when(mosipDeviceSpecificationHelper.buildUrl(anyInt(), anyString())).thenReturn("http://localhost/stream"); + when(mosipDeviceSpecificationHelper.getJPEGByteArray(any(), anyLong())).thenReturn(byteData); - Assert.assertNotNull(actualStream.getMock()); + InputStream result = mockObject.stream(inputBioDevice, "IRIS_DOUBLE"); + assertNotNull(result);
🤖 Fix all issues with AI agents
In
`@registration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecification_SBI_1_0_ProviderImplTest.java`:
- Around line 242-244: The test currently asserts assertNotNull(result) for the
primitive boolean returned by mockObject.isDeviceAvailable(bioDevice) in
MosipDeviceSpecification_SBI_1_0_ProviderImplTest; replace that meaningless
null-check with a boolean assertion that reflects the expected behavior—use
assertTrue(result) if the device should be available or assertFalse(result) if
it should not be available (and ensure the test imports the correct JUnit
assertion methods if missing).
- Around line 173-186: The test creates an SbiDigitalId instance but never
attaches it to the MdmSbiDeviceInfoWrapper (deviceInfo), so the setup is
ineffective; fix by setting the digitalId on deviceInfo using the wrapper's
appropriate setter or adder (e.g., deviceInfo.setDigitalId(...) or
deviceInfo.getDigitalIds().add(...)) before the
when(mosipDeviceSpecificationHelper.getDeviceInfoDecoded(...)) stub so the
returned deviceInfo contains the configured SbiDigitalId.
...o/mosip/registration/test/mdm/service/MosipDeviceSpecification_SBI_1_0_ProviderImplTest.java
Show resolved
Hide resolved
...o/mosip/registration/test/mdm/service/MosipDeviceSpecification_SBI_1_0_ProviderImplTest.java
Outdated
Show resolved
Hide resolved
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/util/restclient/ResponseSignatureAdviceTest.java (1)
324-346:⚠️ Potential issue | 🟡 Minor
fileSignatureValidationTestdoesn't invoke the method under test.The actual call to
responseSignatureAdvice.fileSignatureValidation(joinPointMock)is commented out on line 344, and several mock setups are also commented out (lines 334–338). This test sets up state but never exercises any behavior — it provides zero coverage.Either complete the test or remove it.
🤖 Fix all issues with AI agents
In
`@registration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecification_SBI_1_0_ProviderImplTest.java`:
- Around line 255-267: The test is missing a mock for
mosipDeviceSpecificationHelper.buildUrl which returns null and causes
mapper.readValue to NPE; update the test method
isDeviceAvailable_withNoDevices_returnsFalse to stub
mosipDeviceSpecificationHelper.buildUrl(anyInt(), anyString()) to return a valid
URL string (e.g., "http://localhost/device") before stubbing
getHttpClientResponseEntity, ensuring buildUrl returns non-null so
getHttpClientResponseEntity(...) and subsequent mapper.readValue(...) receive
the mocked response.
🧹 Nitpick comments (8)
registration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecification_095_ProviderImplTest.java (2)
51-66: Weak assertion: only verifies output differs from input, not correctness.
assertNotEquals("LEFT_THUMB", result[0])confirms the value changed but doesn't assert the expected transformed value. If the transformation logic is wrong but produces a different string, this test still passes. Consider asserting the actual expected output.
27-73: Consider extracting the repeated reflection boilerplate into a single generic helper.
invokeGetDeviceSubIdandinvokeGetDefaultCountboth follow the same find-method → setAccessible → invoke pattern. A single generic helper (similar toinvokePrivateinMosipDeviceSpecificationFactoryTest) would reduce duplication.registration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecificationFactoryTest.java (3)
35-54:@MockonmdmBioDeviceis immediately overwritten insetUp().Line 52 reassigns
mdmBioDevice = new MdmBioDevice(), discarding the Mockito mock created by@Mockon line 36. Remove the@Mockannotation since you need a real instance.Suggested fix
- `@Mock` private MdmBioDevice mdmBioDevice;
217-221: Misleading test name:getLatestVersion_differentLength_returnsLongerimplies length, not version comparison.
"1.0.0"vs"1.0"— the result depends on how the implementation treats missing version segments (e.g.,"1.0"treated as"1.0.0"). The test name suggests string length determines the outcome, but it's actually a semantic version comparison. A name like_shorterVersionPadded_returnsCorrectLatestwould be clearer.
66-77: Static shared state mutation without cleanup may leak across tests.
MosipDeviceSpecificationFactory.getAvailableDeviceInfo().put("key", devices)modifies a static map that persists beyond this test. If other test classes in the suite share this JVM, this could cause interference. Consider cleaning up in an@AfterEach.registration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecification_SBI_1_0_ProviderImplTest.java (2)
125-160:stream_whenMocked_returnsMockInputStreamonly tests Mockito, not the production code.This test creates a full mock of
MosipDeviceSpecification_SBI_1_0_ProviderImpl, stubsstream()to return a pre-builtInputStream, then assertsactualStream.getMock()is not null. This verifies Mockito's stubbing works—not any real implementation logic. It contributes no meaningful code coverage.Consider either removing it or rewriting it to use
mockObject(the@InjectMocksinstance) with only dependency mocks stubbed.
486-498: Direct field access (wrapper.deviceInfo = null) is fragile.Accessing
deviceInfodirectly assumes it's a public field. If the field visibility is later changed to private (with a setter), this test breaks. Consider usingwrapper.setDeviceInfo(null)if a setter exists.registration/registration-services/src/test/java/io/mosip/registration/test/update/SoftwareUpdateHandlerTest.java (1)
186-202: Weak assertion — consider verifying the error path was actually exercised.
assertNotNull(response)alone doesn't confirm the exception path was taken —updateDerbyDBmay return a non-nullResponseDTOon both success and error paths. Consider asserting onresponse.getErrorResponseDTOs()being non-null/non-empty (or checking a specific error code/message) to ensure theRuntimeExceptionfromgetSortedVersionMappingswas actually triggered and handled.Suggested stronger assertion
ResponseDTO response = spyHandler.updateDerbyDB(); Assert.assertNotNull(response); + Assert.assertNotNull(response.getErrorResponseDTOs()); + Assert.assertFalse(response.getErrorResponseDTOs().isEmpty());
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
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/mdm/service/MosipDeviceSpecification_SBI_1_0_ProviderImplTest.java (1)
125-160:⚠️ Potential issue | 🟠 MajorTest only validates Mockito stubbing, not actual
stream()behavior.This test creates a full mock of
MosipDeviceSpecification_SBI_1_0_ProviderImpl(line 132) and then stubs itsstream()method to return a canned value. The assertion at line 159 only checks the mock itself is non-null. This verifies Mockito wiring, not any real logic instream().Consider using the
@InjectMocksinstance (mockObject) with the helper dependencies stubbed (e.g.,buildUrl,getJPEGByteArray) to exercise the actual implementation, similar to howstream_withHelper_returnsJpeg_throwsRegBaseCheckedExceptionis structured.
🤖 Fix all issues with AI agents
In
`@registration/registration-services/src/test/java/io/mosip/registration/test/mdm/service/MosipDeviceSpecification_SBI_1_0_ProviderImplTest.java`:
- Around line 196-212: The test rCapture_withEmptyBiometrics_returnsEmptyList is
missing stubs for the injected objectMapper so the JSON returned by
mosipDeviceSpecificationHelper.getHttpClientResponseEntity isn't actually
deserialized; add Mockito stubs on the objectMapper mock to handle readValue for
the rCapture response (the JSON string returned from
getHttpClientResponseEntity) and for any inner biometrics array/type so that
deserialization returns an object representing an empty biometrics list (or an
empty List<BiometricsDto>), ensuring mockObject.rCapture(bioDevice,
mdmRequestDto) exercises the empty-biometrics path; reference the objectMapper
mock, the rCapture method on the class under test (mockObject), and the existing
getHttpClientResponseEntity stub to wire the JSON -> deserialized-object
behavior.
- Around line 255-271: The test is missing a stub for objectMapper.readValue so
the method under test gets null instead of an actual empty list; update the test
isDeviceAvailable_withNoDevices_returnsFalse to mock
objectMapper.readValue(anyString(), any(TypeReference.class)) to return an empty
list (e.g., Collections.emptyList()) so mockObject.isDeviceAvailable(bioDevice)
exercises the "no devices" path; reference the objectMapper.readValue(...) call,
the isDeviceAvailable(...) method on mockObject, and the TypeReference argument
when adding the when(...).thenReturn(...) stub.
...o/mosip/registration/test/mdm/service/MosipDeviceSpecification_SBI_1_0_ProviderImplTest.java
Show resolved
Hide resolved
...o/mosip/registration/test/mdm/service/MosipDeviceSpecification_SBI_1_0_ProviderImplTest.java
Show resolved
Hide resolved
1 participant
Summary by CodeRabbit