[MOSIP-42804] - Sonar coverage added for admin-services#1345
Merged
dhanendra06 merged 19 commits intomosip:developfrom Mar 3, 2026
Merged
[MOSIP-42804] - Sonar coverage added for admin-services#1345dhanendra06 merged 19 commits intomosip:developfrom
dhanendra06 merged 19 commits intomosip:developfrom
Conversation
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.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 many new and expanded unit tests across syncdata and masterdata modules and a small import-only change in SyncMasterDataServiceHelper.java; no production APIs or method signatures were modified. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 3
🧹 Nitpick comments (2)
admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncMasterDataServiceImplTest.java (1)
242-303: Clean up temp files created in tests.
createTempFile(...)files aren’t deleted, which can leak files on CI runners. UsedeleteOnExit()or a try/finally cleanup.🧹 Example cleanup
- Path tempFile = java.nio.file.Files.createTempFile("test", ".json"); + Path tempFile = java.nio.file.Files.createTempFile("test", ".json"); + tempFile.toFile().deleteOnExit();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncMasterDataServiceImplTest.java` around lines 242 - 303, The tests getClientSettingsJsonFile_notEncrypted and getClientSettingsJsonFile_encrypted create temp files with java.nio.file.Files.createTempFile(...) but never delete them; update these tests to ensure temp files are cleaned up (call tempFile.toFile().deleteOnExit() or wrap creation/use in try/finally and explicitly delete the file in finally) so the temporary file referenced by tempFile is removed after the test; apply the same cleanup approach for both test methods (references: method names getClientSettingsJsonFile_notEncrypted, getClientSettingsJsonFile_encrypted and local variable tempFile).admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/SyncMasterDataServiceHelperTest.java (1)
2118-3227: Consider fixed timestamps to avoid time-boundary flakiness.Many tests rely on
LocalDateTime.now()/LocalDate.now()for delta logic. This can become flaky around date boundaries or CI clock skew. Using fixed timestamps (or a testClock) will keep these tests deterministic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/SyncMasterDataServiceHelperTest.java` around lines 2118 - 3227, Tests use LocalDateTime.now() / LocalDate.now() across many test methods (e.g. testGetMachines_WithData_ShouldMapFields, testGetLocationHierarchyList_Success, testGetHolidays_WithChanges_ShouldReturnList, testGetTemplates_WithChanges_ShouldReturnTemplateList, etc.) causing time-bound flakiness; make timestamps deterministic by replacing calls to LocalDateTime.now()/LocalDate.now() with fixed constants or a test Clock: define a single fixed LocalDateTime (and LocalDate) at the top of SyncMasterDataServiceHelperTest (e.g. FIXED_NOW, FIXED_DATE) or inject a Clock.fixed into the code under test, then use those fixed values in each test (adjust lastUpdated/currentTime calculations relative to FIXED_NOW) so all methods (getMachines, getLocationHierarchyList, getHolidays, getTemplates, getDocumentTypes, etc.) use deterministic times.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncAuthTokenServiceImplTest.java`:
- Around line 72-74: The test sets an inverted timestamp window by assigning
ReflectionTestUtils.setField(service, "maxMinutes", -5), which can hide real
validation behavior; change the maxMinutes to a non-negative value greater than
or equal to minMinutes (e.g., set "maxMinutes" to 10) so the window is valid and
the SyncAuthTokenServiceImplTest properly exercises timestamp validation for the
service instance referenced as service (also ensure "minMinutes" and "clientId"
remain correctly set).
- Around line 111-153: The test getAuthToken_success_verifiedTrue in
SyncAuthTokenServiceImplTest currently asserts that service.getAuthToken(token)
throws RequestException despite its name implying a successful path; fix by
either renaming the test to reflect the expected exception (e.g.,
getAuthToken_throwsRequestException_when... ) or change the assertion to verify
a successful response: call service.getAuthToken(token) and assert the returned
TokenResponseDto (or ResponseWrapper) contents instead of assertThrows; update
mocks if necessary so getAuthToken returns normally (e.g., ensure restTemplate
and objectMapper.readValue return the expected wrapper and service.parse/convert
logic succeeds) and replace assertThrows(RequestException.class, ...) with
assertions on the returned value.
In
`@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncUserDetailsServiceImplTest.java`:
- Around line 226-236: Stub the mapper and crypto manager calls used by
getAllUserDetailsBasedOnKeyIndex so the test is deterministic: mock
mapper.getObjectAsJsonString(...) to return a fixed JSON string and mock
clientCryptoManagerService.csEncrypt(...) to return the prepared
TpmCryptoResponseDto (with value "encryptedData"); ensure these mocks are
injected/assigned to the SyncUserDetailsServiceImpl instance (or spy) before
calling spy.getAllUserDetailsBasedOnKeyIndex(KEY_INDEX) and keep the existing
doReturn(userDetailResponseDto).when(spy).getUserDetailsFromAuthServer(any())
stub.
---
Nitpick comments:
In
`@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncMasterDataServiceImplTest.java`:
- Around line 242-303: The tests getClientSettingsJsonFile_notEncrypted and
getClientSettingsJsonFile_encrypted create temp files with
java.nio.file.Files.createTempFile(...) but never delete them; update these
tests to ensure temp files are cleaned up (call tempFile.toFile().deleteOnExit()
or wrap creation/use in try/finally and explicitly delete the file in finally)
so the temporary file referenced by tempFile is removed after the test; apply
the same cleanup approach for both test methods (references: method names
getClientSettingsJsonFile_notEncrypted, getClientSettingsJsonFile_encrypted and
local variable tempFile).
In
`@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/SyncMasterDataServiceHelperTest.java`:
- Around line 2118-3227: Tests use LocalDateTime.now() / LocalDate.now() across
many test methods (e.g. testGetMachines_WithData_ShouldMapFields,
testGetLocationHierarchyList_Success,
testGetHolidays_WithChanges_ShouldReturnList,
testGetTemplates_WithChanges_ShouldReturnTemplateList, etc.) causing time-bound
flakiness; make timestamps deterministic by replacing calls to
LocalDateTime.now()/LocalDate.now() with fixed constants or a test Clock: define
a single fixed LocalDateTime (and LocalDate) at the top of
SyncMasterDataServiceHelperTest (e.g. FIXED_NOW, FIXED_DATE) or inject a
Clock.fixed into the code under test, then use those fixed values in each test
(adjust lastUpdated/currentTime calculations relative to FIXED_NOW) so all
methods (getMachines, getLocationHierarchyList, getHolidays, getTemplates,
getDocumentTypes, etc.) use deterministic times.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
admin/kernel-syncdata-service/src/main/java/io/mosip/kernel/syncdata/utils/SyncMasterDataServiceHelper.javaadmin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/SyncMasterDataServiceHelperTest.javaadmin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncAuthTokenServiceImplTest.javaadmin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncMasterDataServiceImplTest.javaadmin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncRolesServiceImplTest.javaadmin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncUserDetailsServiceImplTest.java
...e/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncAuthTokenServiceImplTest.java
Show resolved
Hide resolved
...e/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncAuthTokenServiceImplTest.java
Outdated
Show resolved
Hide resolved
...src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncUserDetailsServiceImplTest.java
Show resolved
Hide resolved
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
admin/kernel-masterdata-service/src/test/java/io/mosip/kernel/masterdata/test/service/impl/CacheManagementServiceImplTest.java (1)
60-65:nullCache_ignoredtest validity depends on cache-name matching — see above concern.If "CACHE1" doesn't match
CacheName.BLOCK_LISTED_WORDSin the implementation,cacheManager.getCache("CACHE1")is never called and the null-return stub is never reached. The null-handling path being tested would not actually be exercised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/kernel-masterdata-service/src/test/java/io/mosip/kernel/masterdata/test/service/impl/CacheManagementServiceImplTest.java` around lines 60 - 65, The test clears a null-cache path but stubs a cache name "CACHE1" which may not match CacheName.BLOCK_LISTED_WORDS, so the getCache(...) null stub isn't exercised; update the test to stub cacheManager.getCacheNames() to return the actual cache-name string used by the implementation for CacheName.BLOCK_LISTED_WORDS (or use the enum's name/value) and then stub cacheManager.getCache(<that matching name>) to return null so clearCacheByCacheName(CacheName.BLOCK_LISTED_WORDS) exercises the null-handling branch in clearCacheByCacheName.admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncAuthTokenServiceImplTest.java (1)
69-70:⚠️ Potential issue | 🟡 Minor
maxMinutesis still set to-5, inverting the timestamp window.This was flagged in a previous review and remains unaddressed. Setting
maxMinutesto a negative value inverts the allowed timestamp window: any real timestamp (including the current one) falls outside the valid range, makingvalidateRequestTimestamp_invalidpass trivially (a 2-hour-old timestamp would fail even if the window were sane), and potentially causinggetAuthToken_emptyTokenResponse_throwsRequestExceptionto throw at the timestamp check rather than the intended "empty response" code path.🔧 Suggested fix
- ReflectionTestUtils.setField(service, "maxMinutes", -5); + ReflectionTestUtils.setField(service, "maxMinutes", 5);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncAuthTokenServiceImplTest.java` around lines 69 - 70, The test sets an inverted timestamp window by setting maxMinutes to -5; update the ReflectionTestUtils.setField calls in SyncAuthTokenServiceImplTest so that maxMinutes is a positive value greater than minMinutes (e.g., set minMinutes to 5 and maxMinutes to 10) so the request timestamp validation uses a valid window and the test exercises the intended code path (validateRequestTimestamp_invalid should not pass trivially and getAuthToken_emptyTokenResponse_throwsRequestException will reach the empty-response logic).
🧹 Nitpick comments (3)
admin/kernel-masterdata-service/src/test/java/io/mosip/kernel/masterdata/test/service/impl/CacheManagementServiceImplTest.java (1)
33-36:openMocksreturn value is never closed — store and close it in@AfterEach.
openMocksreturns anAutoCloseableinstance that should be closed after test execution. Not closing it can cause issues with static mocks and custom MockMakers, and will trigger compiler/IDE warnings.Consider replacing the manual
openMocksapproach with@ExtendWith(MockitoExtension.class)(idiomatic JUnit 5 + Mockito), or store and close the handle explicitly:♻️ Proposed fix
+import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.junit.jupiter.MockitoExtension; +@ExtendWith(MockitoExtension.class) class CacheManagementServiceImplTest { `@InjectMocks` private CacheManagementServiceImpl cacheService; `@Mock` private CacheManager cacheManager; `@Mock` private Cache mockCache1; `@Mock` private Cache mockCache2; - `@BeforeEach` - void setup() { - MockitoAnnotations.openMocks(this); - }Alternatively, if you prefer keeping
openMocksexplicitly:+ private AutoCloseable closeable; + `@BeforeEach` void setup() { - MockitoAnnotations.openMocks(this); + closeable = MockitoAnnotations.openMocks(this); } + + `@AfterEach` + void tearDown() throws Exception { + closeable.close(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/kernel-masterdata-service/src/test/java/io/mosip/kernel/masterdata/test/service/impl/CacheManagementServiceImplTest.java` around lines 33 - 36, The setup method calls MockitoAnnotations.openMocks(this) but never closes its AutoCloseable handle; change this by storing the returned AutoCloseable (e.g., a class field named mocks) when calling MockitoAnnotations.openMocks in the `@BeforeEach` setup and add an `@AfterEach` method that calls mocks.close() (safely handling null/exception), or replace the manual approach by annotating the test class with `@ExtendWith`(MockitoExtension.class) and removing the setup/teardown methods; look for the setup() method and any references to MockitoAnnotations.openMocks to implement the chosen fix.admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncAuthTokenServiceImplTest.java (2)
83-91:createToken()helper is dead code.The method is defined but never called; all tests construct tokens inline. Remove it or start using it in the inline constructions to reduce duplication.
♻️ Suggested clean-up
- private String createToken(String kid, String machineName, LocalDateTime timestamp) throws Exception { - String headerJson = "{\"kid\":\"" + kid + "\"}"; - String payloadJson = "{\"machineName\":\"" + machineName + "\",\"timestamp\":\"" + timestamp + "\",\"authType\":\"NEW\"}"; - String signature = "sig"; - - return Base64.getUrlEncoder().encodeToString(headerJson.getBytes()) + "." + - Base64.getUrlEncoder().encodeToString(payloadJson.getBytes()) + "." + - Base64.getUrlEncoder().encodeToString(signature.getBytes()); - }Alternatively, generalise it and use it in
getAuthToken_emptyTokenResponse_throwsRequestExceptionandsendOTP_successto eliminate the repetitive inline token construction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncAuthTokenServiceImplTest.java` around lines 83 - 91, The helper createToken(String kid, String machineName, LocalDateTime timestamp) is unused; either remove it or (preferred) reuse it to eliminate duplicated inline token building: update the tests getAuthToken_emptyTokenResponse_throwsRequestException and sendOTP_success to call createToken(kid, machineName, timestamp) instead of manually Base64-encoding header/payload/signature, ensuring the timestamp passed matches the tests' timestamp representation (use the same LocalDateTime value used inline), and if you choose not to reuse it then delete the createToken method to remove dead code.
46-48: Remove unnecessary@Spyannotation.The test does not use partial mocking (
doReturn(...).when(service)...) on the service, so@Spyprovides no value. Only@InjectMocksis needed to inject the mocked dependencies.Proposed fix
- `@InjectMocks` - `@Spy` - private SyncAuthTokenServiceImpl service; + `@InjectMocks` + private SyncAuthTokenServiceImpl service;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncAuthTokenServiceImplTest.java` around lines 46 - 48, Remove the unnecessary `@Spy` on the test field: in SyncAuthTokenServiceImplTest remove the `@Spy` annotation from the private SyncAuthTokenServiceImpl service field so only `@InjectMocks` remains; this avoids creating a partial mock when the test never uses doReturn/when on service and keeps Mockito injection semantics clean (also remove any unused Mockito spy import if present).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@admin/kernel-masterdata-service/src/test/java/io/mosip/kernel/masterdata/test/service/impl/CacheManagementServiceImplTest.java`:
- Around line 39-47: The test
clearCacheByCacheName_existingCache_clearsOnlyMatched only asserts that
mockCache2 is not cleared; add a positive assertion to verify that the intended
cache is actually cleared by calling verify(mockCache1).clear() (or
verify(mockCache1, times(1)).clear()) after invoking
cacheService.clearCacheByCacheName; locate this in
CacheManagementServiceImplTest around the setup using
cacheManager.getCache("CACHE1") and mockCache1 to ensure the target cache clear
is asserted.
- Around line 40-46: The test CacheManagementServiceImplTest stubs use
"CACHE1"/"CACHE2" so they never match CacheName.BLOCK_LISTED_WORDS.getName();
update the cache name stubs returned by cacheManager.getCacheNames() and the
keys passed to cacheManager.getCache(...) to include "blocklisted-words" (the
value of CacheName.BLOCK_LISTED_WORDS.getName()) so that calling
cacheService.clearCacheByCacheName(CacheName.BLOCK_LISTED_WORDS) exercises the
matching branch; adjust mockCache1/mockCache2 placement accordingly so
verify(mockCache2, never()).clear() still asserts the non-matching cache isn't
cleared while the matching mock is cleared.
In
`@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncAuthTokenServiceImplTest.java`:
- Around line 158-162: Remove the redundant test method
getAuthToken_invalidFormat() from SyncAuthTokenServiceImplTest because it
duplicates getAuthToken_invalidRequest_throwsRequestException(); keep the
stronger test (getAuthToken_invalidRequest_throwsRequestException) which also
asserts SyncAuthErrorCode.INVALID_REQUEST and delete the
getAuthToken_invalidFormat method and its usage of
service.getAuthToken("invalid.token") to avoid duplicate assertions.
---
Duplicate comments:
In
`@admin/kernel-masterdata-service/src/test/java/io/mosip/kernel/masterdata/test/service/impl/CacheManagementServiceImplTest.java`:
- Around line 60-65: The test clears a null-cache path but stubs a cache name
"CACHE1" which may not match CacheName.BLOCK_LISTED_WORDS, so the getCache(...)
null stub isn't exercised; update the test to stub cacheManager.getCacheNames()
to return the actual cache-name string used by the implementation for
CacheName.BLOCK_LISTED_WORDS (or use the enum's name/value) and then stub
cacheManager.getCache(<that matching name>) to return null so
clearCacheByCacheName(CacheName.BLOCK_LISTED_WORDS) exercises the null-handling
branch in clearCacheByCacheName.
In
`@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncAuthTokenServiceImplTest.java`:
- Around line 69-70: The test sets an inverted timestamp window by setting
maxMinutes to -5; update the ReflectionTestUtils.setField calls in
SyncAuthTokenServiceImplTest so that maxMinutes is a positive value greater than
minMinutes (e.g., set minMinutes to 5 and maxMinutes to 10) so the request
timestamp validation uses a valid window and the test exercises the intended
code path (validateRequestTimestamp_invalid should not pass trivially and
getAuthToken_emptyTokenResponse_throwsRequestException will reach the
empty-response logic).
---
Nitpick comments:
In
`@admin/kernel-masterdata-service/src/test/java/io/mosip/kernel/masterdata/test/service/impl/CacheManagementServiceImplTest.java`:
- Around line 33-36: The setup method calls MockitoAnnotations.openMocks(this)
but never closes its AutoCloseable handle; change this by storing the returned
AutoCloseable (e.g., a class field named mocks) when calling
MockitoAnnotations.openMocks in the `@BeforeEach` setup and add an `@AfterEach`
method that calls mocks.close() (safely handling null/exception), or replace the
manual approach by annotating the test class with
`@ExtendWith`(MockitoExtension.class) and removing the setup/teardown methods;
look for the setup() method and any references to MockitoAnnotations.openMocks
to implement the chosen fix.
In
`@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncAuthTokenServiceImplTest.java`:
- Around line 83-91: The helper createToken(String kid, String machineName,
LocalDateTime timestamp) is unused; either remove it or (preferred) reuse it to
eliminate duplicated inline token building: update the tests
getAuthToken_emptyTokenResponse_throwsRequestException and sendOTP_success to
call createToken(kid, machineName, timestamp) instead of manually
Base64-encoding header/payload/signature, ensuring the timestamp passed matches
the tests' timestamp representation (use the same LocalDateTime value used
inline), and if you choose not to reuse it then delete the createToken method to
remove dead code.
- Around line 46-48: Remove the unnecessary `@Spy` on the test field: in
SyncAuthTokenServiceImplTest remove the `@Spy` annotation from the private
SyncAuthTokenServiceImpl service field so only `@InjectMocks` remains; this avoids
creating a partial mock when the test never uses doReturn/when on service and
keeps Mockito injection semantics clean (also remove any unused Mockito spy
import if present).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
admin/kernel-masterdata-service/src/test/java/io/mosip/kernel/masterdata/test/service/impl/CacheManagementServiceImplTest.javaadmin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncAuthTokenServiceImplTest.java
...c/test/java/io/mosip/kernel/masterdata/test/service/impl/CacheManagementServiceImplTest.java
Show resolved
Hide resolved
...c/test/java/io/mosip/kernel/masterdata/test/service/impl/CacheManagementServiceImplTest.java
Outdated
Show resolved
Hide resolved
...e/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncAuthTokenServiceImplTest.java
Show resolved
Hide resolved
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/SyncMasterDataServiceHelperTest.java (1)
2125-2999: Consider extracting helpers/parameterized tests to reduce repetition.Many new tests follow the same “no changes / changes / empty / data access exception” pattern. A small helper (or JUnit parameterized tests) could cut down duplication and keep the suite easier to maintain.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/SyncMasterDataServiceHelperTest.java` around lines 2125 - 2999, Many tests repeat the same "no changes / changes / empty / data access exception" pattern; extract reusable helpers and/or convert groups to parameterized tests to remove duplication. Create small helper methods in SyncMasterDataServiceHelperTest such as prepareEntityDtimesAndStub(repository, EntityDtimes), assertNoChangesReturnsNull(callableGetMethod, repositoryVerifier), and stubRepositoryToThrow(repository, exception) and then replace repeated setups that call repository.getMaxCreatedDateTimeMaxUpdatedDateTime(), repository.find...(...) and verify(...) (examples: templateRepository.getMaxCreatedDateTimeMaxUpdatedDateTime() + syncMasterData.getTemplates(...), holidayRepository.findAllLatestCreatedUpdateDeletedByMachineId(...) with testGetTemplates_NoChangesFound_ShouldReturnNull, testGetTemplates_WithChanges_ShouldReturnTemplateList, testGetHolidays_WithChanges_ShouldReturnList, testGetHolidays_EmptyList_ShouldReturnNull); alternatively group similar cases into parameterized tests (JUnit parameterized or JUnit5 `@ParameterizedTest`) driving repository, input args and expected outcome to keep assertions concise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/SyncMasterDataServiceHelperTest.java`:
- Around line 173-175: There are two mocked ApplicantValidDocumentRespository
fields causing one to be uninjected by `@InjectMocks` on syncMasterData; remove
one duplicate and consolidate usages so the injected mock is the one used in
stubs: either delete the misspelled field applicantValidDocumentRespository and
update all test stubs/uses to the correctly-named
applicantValidDocumentRepository, or delete applicantValidDocumentRepository and
rename all stubs/uses to applicantValidDocumentRespository; ensure only one
`@Mock` of ApplicantValidDocumentRespository remains and that all when(...) /
verify(...) calls reference that single field so syncMasterData receives the
stubbed mock.
- Around line 2604-2623: The test calls syncMasterData.getTemplates(...) which
returns a CompletableFuture but does not await it, so async exceptions are never
observed; update the test to call and await the returned CompletableFuture
(e.g., call .get() or .join() on the CompletableFuture returned by getTemplates)
and assert that it throws/wraps the expected exception (ExecutionException or
the underlying cause) so the DataAccessResourceFailureException from
templateRepository.findAllLatestCreatedUpdateDeletedByModule is verified.
---
Nitpick comments:
In
`@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/SyncMasterDataServiceHelperTest.java`:
- Around line 2125-2999: Many tests repeat the same "no changes / changes /
empty / data access exception" pattern; extract reusable helpers and/or convert
groups to parameterized tests to remove duplication. Create small helper methods
in SyncMasterDataServiceHelperTest such as
prepareEntityDtimesAndStub(repository, EntityDtimes),
assertNoChangesReturnsNull(callableGetMethod, repositoryVerifier), and
stubRepositoryToThrow(repository, exception) and then replace repeated setups
that call repository.getMaxCreatedDateTimeMaxUpdatedDateTime(),
repository.find...(...) and verify(...) (examples:
templateRepository.getMaxCreatedDateTimeMaxUpdatedDateTime() +
syncMasterData.getTemplates(...),
holidayRepository.findAllLatestCreatedUpdateDeletedByMachineId(...) with
testGetTemplates_NoChangesFound_ShouldReturnNull,
testGetTemplates_WithChanges_ShouldReturnTemplateList,
testGetHolidays_WithChanges_ShouldReturnList,
testGetHolidays_EmptyList_ShouldReturnNull); alternatively group similar cases
into parameterized tests (JUnit parameterized or JUnit5 `@ParameterizedTest`)
driving repository, input args and expected outcome to keep assertions concise.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/SyncMasterDataServiceHelperTest.java
...ice/src/test/java/io/mosip/kernel/syncdata/test/service/SyncMasterDataServiceHelperTest.java
Outdated
Show resolved
Hide resolved
...rvice/src/test/java/io/mosip/kernel/syncdata/test/utils/SyncMasterDataServiceHelperTest.java
Show resolved
Hide resolved
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
admin/kernel-masterdata-service/src/test/java/io/mosip/kernel/masterdata/test/service/impl/CacheManagementServiceImplTest.java (1)
38-47:⚠️ Potential issue | 🟠 MajorAdd a positive assertion that the matched cache is cleared.
Line 44 invokes the target, but the test only asserts the non‑matched cache is untouched. Add
verify(mockCache1).clear()to ensure the matching cache is actually cleared.🐛 Proposed fix
cacheService.clearCacheByCacheName(CacheName.BLOCK_LISTED_WORDS); + verify(mockCache1).clear(); verify(mockCache2, never()).clear();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/kernel-masterdata-service/src/test/java/io/mosip/kernel/masterdata/test/service/impl/CacheManagementServiceImplTest.java` around lines 38 - 47, The test clearCacheByCacheName_existingCache_clearsOnlyMatched is missing a positive assertion that the matched cache was cleared; after invoking cacheService.clearCacheByCacheName(CacheName.BLOCK_LISTED_WORDS) add a verification that mockCache1.clear() was called (e.g., verify(mockCache1).clear()) so the test asserts both that the matched cache is cleared and mockCache2 is not (verify(mockCache2, never()).clear()) when using the mocked cacheManager/getCache behavior.admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncAuthTokenServiceImplTest.java (1)
69-71:maxMinutes = -5still creates an inverted timestamp window — unresolved.This was flagged in a previous review. With
minMinutes=5andmaxMinutes=-5, the valid timestamp range becomes inverted, causing any currentnowtimestamp to failvalidateRequestTimestamp. This silently alters which code paths are exercised in tests that supply a current timestamp (see the downstream note on lines 94–139).🔧 Proposed fix
- ReflectionTestUtils.setField(service, "maxMinutes", -5); + ReflectionTestUtils.setField(service, "maxMinutes", 5);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncAuthTokenServiceImplTest.java` around lines 69 - 71, The test sets an inverted timestamp window by assigning minMinutes=5 and maxMinutes=-5 on the tested service (in SyncAuthTokenServiceImplTest), which causes validateRequestTimestamp to reject current timestamps; fix by setting maxMinutes to a non-negative value >= minMinutes (e.g., make maxMinutes 5 or 10) when calling ReflectionTestUtils.setField(service, "maxMinutes", ...), or adjust the test to enforce minMinutes <= maxMinutes before running validateRequestTimestamp so the window is not inverted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncAuthTokenServiceImplTest.java`:
- Around line 94-139: The test is failing to exercise the REST/token-response
logic because the timestamp window is inverted; in the test setup change the
negative maxMinutes used in the `@BeforeEach` initialization to a positive value
(e.g., 5) so validateRequestTimestamp accepts the MachineAuthDto timestamp and
execution reaches service.getAuthToken's REST call and token-response handling
(references: getAuthToken, validateRequestTimestamp, MachineAuthDto and the
`@BeforeEach` maxMinutes setup).
---
Duplicate comments:
In
`@admin/kernel-masterdata-service/src/test/java/io/mosip/kernel/masterdata/test/service/impl/CacheManagementServiceImplTest.java`:
- Around line 38-47: The test
clearCacheByCacheName_existingCache_clearsOnlyMatched is missing a positive
assertion that the matched cache was cleared; after invoking
cacheService.clearCacheByCacheName(CacheName.BLOCK_LISTED_WORDS) add a
verification that mockCache1.clear() was called (e.g.,
verify(mockCache1).clear()) so the test asserts both that the matched cache is
cleared and mockCache2 is not (verify(mockCache2, never()).clear()) when using
the mocked cacheManager/getCache behavior.
In
`@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncAuthTokenServiceImplTest.java`:
- Around line 69-71: The test sets an inverted timestamp window by assigning
minMinutes=5 and maxMinutes=-5 on the tested service (in
SyncAuthTokenServiceImplTest), which causes validateRequestTimestamp to reject
current timestamps; fix by setting maxMinutes to a non-negative value >=
minMinutes (e.g., make maxMinutes 5 or 10) when calling
ReflectionTestUtils.setField(service, "maxMinutes", ...), or adjust the test to
enforce minMinutes <= maxMinutes before running validateRequestTimestamp so the
window is not inverted.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
admin/kernel-masterdata-service/src/test/java/io/mosip/kernel/masterdata/test/service/impl/CacheManagementServiceImplTest.javaadmin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/SyncMasterDataServiceHelperTest.javaadmin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncAuthTokenServiceImplTest.javaadmin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncMasterDataServiceImplTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/SyncMasterDataServiceHelperTest.java
- admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncMasterDataServiceImplTest.java
...e/src/test/java/io/mosip/kernel/syncdata/test/service/impl/SyncAuthTokenServiceImplTest.java
Show resolved
Hide resolved
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/controller/WebsubCallbackControllerTest.java (2)
100-183:⚠️ Potential issue | 🔴 CriticalOld tests never invoke the method under test — they are no-ops.
Tests
testHandleCACertificate_shouldUploadCertificate(line 102),testHandleCACertificate_shouldHandleError(line 131), andtestHandleCACertificate_shouldUploadCACertificate(line 163) all construct anEventModeland set up stubs, but none of them callcontroller.handleCACertificate(eventModel). They pass without actually testing any behavior.Additionally, these tests use
@WithUserDetails(inert underMockitoJUnitRunner), referencetopic(which isnullsince@Valueis not resolved), and useMockRestServiceServerbound to a Mockito mock (which doesn't work as intended).These tests inflate coverage metrics without validating any behavior. They should be either fixed to actually invoke the controller or removed entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/controller/WebsubCallbackControllerTest.java` around lines 100 - 183, The three tests (testHandleCACertificate_shouldUploadCertificate, testHandleCACertificate_shouldHandleError, testHandleCACertificate_shouldUploadCACertificate) never call the method under test; update each to actually invoke controller.handleCACertificate(eventModel) after constructing the EventModel, then assert/verify expected outcomes (e.g., response body, partnerCertificateManagerService.uploadCACertificate interactions, or logged errors) and verify the MockRestServiceServer exchanges; remove or replace the inert `@WithUserDetails` annotation (or run with a Spring test runner that resolves it) and ensure the injected topic value is set (assign a test value to the topic field or mock the `@Value`) and bind MockRestServiceServer to a real RestTemplate used by the controller instead of a Mockito mock so the stubbed HTTP responses are effective; alternatively if you prefer deletion, remove these no-op tests.
54-57:⚠️ Potential issue | 🟠 Major
@RunWith(MockitoJUnitRunner.class)makes all Spring annotations inert.
MockitoJUnitRunnerdoes not bootstrap a SpringApplicationContext. This means@SpringBootTest,@AutoConfigureMockMvc,@Autowired(lines 68, 78),@Value(lines 81, 84), and@WithUserDetails(lines 101, 130, 162) are silently ignored. Fields likemockMvc,authenticatedContentVerifier,secret, andtopicwill benullat runtime.If the intent is purely Mockito-based unit testing (which the new tests do correctly), remove the Spring annotations. If Spring integration testing is needed for the older tests, they should be in a separate class using
SpringRunner.Proposed fix: remove dead Spring annotations for a pure unit test class
`@RunWith`(MockitoJUnitRunner.class) -@SpringBootTest(classes = TestBootApplication.class) -@AutoConfigureMockMvc public class WebsubCallbackControllerTest {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/controller/WebsubCallbackControllerTest.java` around lines 54 - 57, The test class WebsubCallbackControllerTest is using MockitoJUnitRunner alongside Spring annotations which are inert; for a pure unit test keep `@RunWith`(MockitoJUnitRunner.class) and remove Spring-specific annotations (`@SpringBootTest`, `@AutoConfigureMockMvc`) and injection annotations (`@Autowired`, `@Value`, `@WithUserDetails`). Replace autowired beans like authenticatedContentVerifier with `@Mock` and the controller under test with `@InjectMocks`, initialize mockMvc via MockMvcBuilders.standaloneSetup(...) in a `@Before` method, and replace secret and topic `@Value` fields with test constants or mocks so mockMvc, authenticatedContentVerifier, secret, and topic are not null at runtime.
♻️ Duplicate comments (1)
admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/SyncMasterDataServiceHelperTest.java (1)
2584-2604: Async exception test does not await theCompletableFuture— silently passes when it should fail.
getTemplatesreturns aCompletableFuture. Without.get(), any exception thrown inside the future's completion is swallowed and the test always passes vacuously. Several newly-added tests forgetTemplates,getTemplateFileFormats,getReasonCategory,getReasonList,getHolidays,getBlackListedWords,getDocumentTypes, and thegetLocationHierarchyListvariants (lines 2284–2401) share this same Pattern-A shape. In contrast, other new tests in this PR (testGetMachines_DataAccessException,testGetRegistrationCenterMachines_DataAccessException,testGetRegistrationCenterUsers_DataAccessException, etc.) correctly use Pattern B.Apply Pattern B consistently for all
DataAccessExceptiontests:- syncMasterData.getTemplates("REG", lastUpdated, currentTime); + try { + syncMasterData.getTemplates("REG", lastUpdated, currentTime).get(); + } catch (ExecutionException e) { + throw e.getCause(); + }The method signature should also declare
throws Throwable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/SyncMasterDataServiceHelperTest.java` around lines 2584 - 2604, The async tests call methods that return CompletableFuture (e.g., syncMasterData.getTemplates, getTemplateFileFormats, getReasonCategory, getReasonList, getHolidays, getBlackListedWords, getDocumentTypes, and the getLocationHierarchyList variants) but never await them, so exceptions inside the future are swallowed; update each failing test method to declare "throws Throwable" and call .get() on the CompletableFuture returned by the service invocation (e.g., syncMasterData.getTemplates(...).get()) so the DataAccessResourceFailureException is propagated to the test; apply this Pattern B change consistently across all the listed test methods.
🧹 Nitpick comments (6)
admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/controller/WebsubCallbackControllerTest.java (3)
263-264: Unusedwrappervariable.The
ResponseWrapperat lines 263-264 is constructed and populated but never passed to any mock or assertion. Remove it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/controller/WebsubCallbackControllerTest.java` around lines 263 - 264, Remove the unused ResponseWrapper instance in WebsubCallbackControllerTest: delete the lines that create "ResponseWrapper<JsonNode> wrapper = new ResponseWrapper<>();" and "wrapper.setErrors(Collections.singletonList(new ServiceError(\"ERR-001\", \"Error\")));", since "wrapper" is never used in mocks or assertions; ensure no other code references "wrapper" remains.
62-63: Dead fields that will never be populated.With
MockitoJUnitRunner, the following fields are alwaysnull:
mockMvc(line 69) —@Autowired, unusedauthenticatedContentVerifier(line 79) —@Autowired, unusedsecret(line 82) —@Value, used only ingetHubSignature()which is never calledtopic(line 85) —@Value, used in old tests that are no-opsThese should be removed along with the old tests to avoid confusion.
Also applies to: 68-69, 78-82, 84-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/controller/WebsubCallbackControllerTest.java` around lines 62 - 63, Remove the dead/unpopulated fields and related unused tests: delete the fields mockMvc, authenticatedContentVerifier, secret, and topic (and their `@Autowired/`@Value annotations) from WebsubCallbackControllerTest; remove any obsolete test methods that reference topic or call no-ops and the unused helper getHubSignature(), and clean up any now-unused imports/fields in the test class; run the test suite to ensure nothing else depends on those symbols.
92-96:MockRestServiceServer.bindTo()on a Mockito@MockRestTemplate is incorrect.
MockRestServiceServeris designed to intercept requests on a realRestTemplateby replacing its request factory. Binding it to a Mockito mock has no meaningful effect — the mock's behavior is controlled by Mockito stubs, not by theMockRestServiceServerexpectations. This line creates a misleading setup.The new tests correctly use
when(restTemplate.getForObject(...))Mockito stubs instead, makingmockRestServiceServerunnecessary. Consider removing it.Proposed cleanup
`@Before` public void setUp() throws Exception { - mockRestServiceServer = MockRestServiceServer.bindTo(restTemplate) - .ignoreExpectOrder(true) - .build(); setField("partnerAllowedDomains", Arrays.asList("FTM", "DEVICE")); }Also remove the field:
-private MockRestServiceServer mockRestServiceServer;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/controller/WebsubCallbackControllerTest.java` around lines 92 - 96, In setUp() the test creates mockRestServiceServer via MockRestServiceServer.bindTo(restTemplate) which is invalid because restTemplate is a Mockito `@Mock`; remove the mockRestServiceServer construction and the mockRestServiceServer field entirely, and keep using Mockito stubs like when(restTemplate.getForObject(...)) in your tests; update setUp() to only configure test state (e.g., setField("partnerAllowedDomains", ...)) and delete any expectations or references to mockRestServiceServer elsewhere in WebsubCallbackControllerTest.admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/websub/PartnerCACertEventSubscriberTest.java (1)
47-51:getDeclaredFielddoes not traverse the class hierarchy.If any of the five fields (
topicName,callbackUrl,callbackSecret,hubUrl,publisherUrl) are declared in a superclass ofPartnerCACertEventSubscriber, this helper will throwNoSuchFieldExceptionat test setup time. Consider walking the hierarchy or usingReflectionTestUtils.setField, which is already idiomatic in this codebase and handles inherited fields automatically.♻️ Replace custom reflection helper with `ReflectionTestUtils`
Remove the
setFieldhelper entirely and updatesetUp:+import org.springframework.test.util.ReflectionTestUtils; `@BeforeEach` void setUp() { - setField("topicName", "test-topic"); - setField("callbackUrl", "http://callback-url"); - setField("callbackSecret", "secret"); - setField("hubUrl", "http://hub-url"); - setField("publisherUrl", "http://publisher-url"); + ReflectionTestUtils.setField(subscriber, "topicName", "test-topic"); + ReflectionTestUtils.setField(subscriber, "callbackUrl", "http://callback-url"); + ReflectionTestUtils.setField(subscriber, "callbackSecret", "secret"); + ReflectionTestUtils.setField(subscriber, "hubUrl", "http://hub-url"); + ReflectionTestUtils.setField(subscriber, "publisherUrl", "http://publisher-url"); } - -private void setField(String fieldName, Object value) throws Exception { - Field field = PartnerCACertEventSubscriber.class.getDeclaredField(fieldName); - field.setAccessible(true); - field.set(subscriber, value); -}Also remove the unused
java.lang.reflect.Fieldimport.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/websub/PartnerCACertEventSubscriberTest.java` around lines 47 - 51, The custom setField helper on PartnerCACertEventSubscriber uses getDeclaredField which fails to find inherited fields and can throw NoSuchFieldException; remove the setField method and the unused java.lang.reflect.Field import, and in setUp replace uses with ReflectionTestUtils.setField(subscriber, "topicName", value) (and similarly for "callbackUrl", "callbackSecret", "hubUrl", "publisherUrl") so inherited fields are handled automatically.admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/SyncMasterDataServiceHelperTest.java (2)
3367-3388: Dead variableentityDtimesintestGetRegistrationCenterMachines_LastUpdatedNull.
EntityDtimes entityDtimesis constructed on line 3371 but is never passed to anywhen(...)stub. It has no effect on the test and should be removed.🧹 Remove unused variable
- EntityDtimes entityDtimes = new EntityDtimes(now, null, null); - when(machineRepository.findMachineLatestCreatedUpdatedDeleted(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/SyncMasterDataServiceHelperTest.java` around lines 3367 - 3388, The test method testGetRegistrationCenterMachines_LastUpdatedNull contains an unused local variable EntityDtimes entityDtimes that is constructed but never used; remove the declaration and construction of entityDtimes from the test to eliminate the dead variable and keep the test focused (also run a quick cleanup to remove any now-unused imports if needed).
76-76:realHelperis created and wired but never used — dead code.
realHelperis instantiated and has 12 fields injected viaReflectionTestUtilsinsetUp(), but it is never referenced in any@Testmethod. Remove this dead scaffolding to reduce confusion about whether it affects the actualsyncMasterDatainstance.🧹 Remove unused realHelper
- private SyncMasterDataServiceHelper realHelper; -- realHelper = new SyncMasterDataServiceHelper(); - ReflectionTestUtils.setField(realHelper, "documentTypeRepository", documentTypeRepository); - ObjectMapper realObjectMapper = new ObjectMapper(); - ReflectionTestUtils.setField(realHelper, "objectMapper", realObjectMapper); - ReflectionTestUtils.setField(realHelper, "locationHirerarchyUrl", "http://localhost/location-hierarchy"); - ReflectionTestUtils.setField(realHelper, "machineRepository", machineRepository); - ReflectionTestUtils.setField(realHelper, "registrationCenterRepository", registrationCenterRepository); - ReflectionTestUtils.setField(realHelper, "templateRepository", templateRepository); - ReflectionTestUtils.setField(realHelper, "templateFileFormatRepository", templateFileFormatRepository); - ReflectionTestUtils.setField(realHelper, "reasonCategoryRepository", reasonCategoryRepository); - ReflectionTestUtils.setField(realHelper, "reasonListRepository", reasonListRepository); - ReflectionTestUtils.setField(realHelper, "holidayRepository", holidayRepository); - ReflectionTestUtils.setField(realHelper, "blocklistedWordsRepository", blocklistedWordsRepository); - ReflectionTestUtils.setField(realHelper, "locationRepository", locationRepository);Also applies to: 231-244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/SyncMasterDataServiceHelperTest.java` at line 76, Remove the dead test scaffolding: delete the private field realHelper and any setup/injection code that constructs and populates it (the ReflectionTestUtils.setField calls in setUp()), since tests exercise syncMasterData and never reference realHelper; also remove any imports or helper methods that only served realHelper so the test class only initializes and uses syncMasterData and its real collaborators.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/controller/WebsubCallbackControllerTest.java`:
- Around line 329-347: The test handleCACertificate_objectMapperThrowsException
currently doesn't stub objectMapper to throw, so it doesn't exercise the
exception path; update the test to explicitly stub objectMapper.readValue(...)
to throw a JsonProcessingException (targeting ResponseWrapper.class) before
calling controller.handleCACertificate(eventModel) and then assert/verify the
expected behavior (e.g., that
partnerCertificateManagerService.uploadCACertificate(...) is/aren't invoked as
intended), or if you intended to test the null-response path, rename the test to
reflect "null response wrapper" and add an explicit
when(objectMapper.readValue(...)).thenReturn(null) stub to make the intent
clear.
- Around line 249-270: The test method
handleCACertificate_whenServiceError_shouldNotUpload asserts the opposite of its
name; update the verification for
partnerCertificateManagerService.uploadCACertificate in this test to assert it
is NOT called (use verify(partnerCertificateManagerService,
never()).uploadCACertificate(any()) or times(0)) so the assertion matches the
test name and expected behavior of controller.handleCACertificate; locate the
verify call currently using times(1) and change it accordingly.
In
`@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/websub/PartnerCACertEventSubscriberTest.java`:
- Around line 114-125: The test
subscribeTopics_withNullValues_shouldStillCallSubscribe currently nulls
topicName, callbackUrl, callbackSecret, and hubUrl but leaves publisherUrl
non-null, making the test intent inconsistent; update the test by nulling
publisherUrl as well via setField("publisherUrl", null) before calling
subscriber.subscribeTopics() so all required fields are null, or alternatively
rename the test to reflect that only a subset of fields are nulled—apply the
former change in the PartnerCACertEventSubscriberTest test method to preserve
the original test title and intent.
- Around line 27-33: Remove the unused mock or add tests: either delete the
unused `@Mock` field publisherClient from PartnerCACertEventSubscriberTest to
avoid dead test setup, or re-enable/add a test exercising the publisherClient by
invoking the production method tryRegisterTopicPartnerCertEvents (or the public
entry that triggers it) and asserting behavior with
Mockito.verify(publisherClient, ...) as appropriate; keep the existing subscribe
mock as-is since its name matches the production field.
---
Outside diff comments:
In
`@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/controller/WebsubCallbackControllerTest.java`:
- Around line 100-183: The three tests
(testHandleCACertificate_shouldUploadCertificate,
testHandleCACertificate_shouldHandleError,
testHandleCACertificate_shouldUploadCACertificate) never call the method under
test; update each to actually invoke controller.handleCACertificate(eventModel)
after constructing the EventModel, then assert/verify expected outcomes (e.g.,
response body, partnerCertificateManagerService.uploadCACertificate
interactions, or logged errors) and verify the MockRestServiceServer exchanges;
remove or replace the inert `@WithUserDetails` annotation (or run with a Spring
test runner that resolves it) and ensure the injected topic value is set (assign
a test value to the topic field or mock the `@Value`) and bind
MockRestServiceServer to a real RestTemplate used by the controller instead of a
Mockito mock so the stubbed HTTP responses are effective; alternatively if you
prefer deletion, remove these no-op tests.
- Around line 54-57: The test class WebsubCallbackControllerTest is using
MockitoJUnitRunner alongside Spring annotations which are inert; for a pure unit
test keep `@RunWith`(MockitoJUnitRunner.class) and remove Spring-specific
annotations (`@SpringBootTest`, `@AutoConfigureMockMvc`) and injection annotations
(`@Autowired`, `@Value`, `@WithUserDetails`). Replace autowired beans like
authenticatedContentVerifier with `@Mock` and the controller under test with
`@InjectMocks`, initialize mockMvc via MockMvcBuilders.standaloneSetup(...) in a
`@Before` method, and replace secret and topic `@Value` fields with test constants
or mocks so mockMvc, authenticatedContentVerifier, secret, and topic are not
null at runtime.
---
Duplicate comments:
In
`@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/SyncMasterDataServiceHelperTest.java`:
- Around line 2584-2604: The async tests call methods that return
CompletableFuture (e.g., syncMasterData.getTemplates, getTemplateFileFormats,
getReasonCategory, getReasonList, getHolidays, getBlackListedWords,
getDocumentTypes, and the getLocationHierarchyList variants) but never await
them, so exceptions inside the future are swallowed; update each failing test
method to declare "throws Throwable" and call .get() on the CompletableFuture
returned by the service invocation (e.g.,
syncMasterData.getTemplates(...).get()) so the
DataAccessResourceFailureException is propagated to the test; apply this Pattern
B change consistently across all the listed test methods.
---
Nitpick comments:
In
`@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/controller/WebsubCallbackControllerTest.java`:
- Around line 263-264: Remove the unused ResponseWrapper instance in
WebsubCallbackControllerTest: delete the lines that create
"ResponseWrapper<JsonNode> wrapper = new ResponseWrapper<>();" and
"wrapper.setErrors(Collections.singletonList(new ServiceError(\"ERR-001\",
\"Error\")));", since "wrapper" is never used in mocks or assertions; ensure no
other code references "wrapper" remains.
- Around line 62-63: Remove the dead/unpopulated fields and related unused
tests: delete the fields mockMvc, authenticatedContentVerifier, secret, and
topic (and their `@Autowired/`@Value annotations) from
WebsubCallbackControllerTest; remove any obsolete test methods that reference
topic or call no-ops and the unused helper getHubSignature(), and clean up any
now-unused imports/fields in the test class; run the test suite to ensure
nothing else depends on those symbols.
- Around line 92-96: In setUp() the test creates mockRestServiceServer via
MockRestServiceServer.bindTo(restTemplate) which is invalid because restTemplate
is a Mockito `@Mock`; remove the mockRestServiceServer construction and the
mockRestServiceServer field entirely, and keep using Mockito stubs like
when(restTemplate.getForObject(...)) in your tests; update setUp() to only
configure test state (e.g., setField("partnerAllowedDomains", ...)) and delete
any expectations or references to mockRestServiceServer elsewhere in
WebsubCallbackControllerTest.
In
`@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/SyncMasterDataServiceHelperTest.java`:
- Around line 3367-3388: The test method
testGetRegistrationCenterMachines_LastUpdatedNull contains an unused local
variable EntityDtimes entityDtimes that is constructed but never used; remove
the declaration and construction of entityDtimes from the test to eliminate the
dead variable and keep the test focused (also run a quick cleanup to remove any
now-unused imports if needed).
- Line 76: Remove the dead test scaffolding: delete the private field realHelper
and any setup/injection code that constructs and populates it (the
ReflectionTestUtils.setField calls in setUp()), since tests exercise
syncMasterData and never reference realHelper; also remove any imports or helper
methods that only served realHelper so the test class only initializes and uses
syncMasterData and its real collaborators.
In
`@admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/websub/PartnerCACertEventSubscriberTest.java`:
- Around line 47-51: The custom setField helper on PartnerCACertEventSubscriber
uses getDeclaredField which fails to find inherited fields and can throw
NoSuchFieldException; remove the setField method and the unused
java.lang.reflect.Field import, and in setUp replace uses with
ReflectionTestUtils.setField(subscriber, "topicName", value) (and similarly for
"callbackUrl", "callbackSecret", "hubUrl", "publisherUrl") so inherited fields
are handled automatically.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/controller/WebsubCallbackControllerTest.javaadmin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/SyncMasterDataServiceHelperTest.javaadmin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/websub/PartnerCACertEventSubscriberTest.java
...ice/src/test/java/io/mosip/kernel/syncdata/test/controller/WebsubCallbackControllerTest.java
Show resolved
Hide resolved
...ice/src/test/java/io/mosip/kernel/syncdata/test/controller/WebsubCallbackControllerTest.java
Show resolved
Hide resolved
...ice/src/test/java/io/mosip/kernel/syncdata/test/websub/PartnerCACertEventSubscriberTest.java
Outdated
Show resolved
Hide resolved
...ice/src/test/java/io/mosip/kernel/syncdata/test/websub/PartnerCACertEventSubscriberTest.java
Show resolved
Hide resolved
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
NidhiKumari0201
changed the title
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
dhanendra06
requested changes
Mar 3, 2026
...ncdata-service/src/main/java/io/mosip/kernel/syncdata/utils/SyncMasterDataServiceHelper.java
Show resolved
Hide resolved
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
dhanendra06
approved these changes
Mar 3, 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