[MOSIP-43903] - Increased Junit code coverage for Registration Processor (develop branch)#2249
Conversation
…sor (develop branch) Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
WalkthroughAdds many unit tests across multiple registration-processor modules (UIN generator, bio-dedupe, verification, notification, packet-manager, message-sender, manual-adjudication). All edits are test-only: new test methods, mocks, imports, and a duplicated test block; no production code or public API changes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.40.0)registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.javaThanks 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-18T09:19:25.334Z
Learnt from: ashok-ksharma
Repo: mosip/registration PR: 2209
File: registration-processor/core-processor/registration-processor-uin-generator-stage/src/main/java/io/mosip/registration/processor/stages/uingenerator/stage/UinGeneratorStage.java:1168-1177
Timestamp: 2025-11-18T09:19:25.334Z
Learning: In the MOSIP registration processor, the `updatePacketCreatedOnInDemographicIdentity` method in `UinGeneratorStage.java` is intentionally limited to `RegistrationType.NEW` and `RegistrationType.UPDATE` only, because these are the registration types that update biometrics. `RES_UPDATE` and Mapped `UPDATE` do not update biometrics, so they should not trigger the `packetCreatedOn` update logic.
Applied to files:
registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java
🧬 Code graph analysis (1)
registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java (1)
registration-processor/registration-processor-core/src/main/java/io/mosip/registration/processor/core/constant/MappingJsonConstants.java (1)
MappingJsonConstants(3-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-maven-registration / maven-build
🔇 Additional comments (2)
registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java (2)
2881-2933: Verify test behavior matches reactivation logic.This test sets up
idResponseDTOwith status "ACTIVATED" (returned byidrepoUpdateDraft) andidResponseDTO1with status "ANY" (returned bygetApi). The expected result isisValid=trueandinternalError=false.Ensure this mock setup accurately reflects the production code's reactivation flow for an already-activated UIN, particularly the interaction between draft service and REST API responses.
2842-2879: Test setup doesn't align with test name—mock for IdRepoDraft null return may be missing.The test name claims to verify behavior "WhenIdRepoDraftReturnsNull", but the visible mock configuration does not set
idrepoDraftService.idrepoUpdateDraft()to returnnull. Verify the complete test setup and confirm whether this mock is configured elsewhere or should be added:when(idrepoDraftService.idrepoUpdateDraft(anyString(), any(), any())).thenReturn(null);
.../src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java
Show resolved
Hide resolved
.../src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java
Show resolved
Hide resolved
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
registration-processor/core-processor/registration-processor-verification-stage/src/test/java/io/mosip/registration/processor/verification/service/VerificationServiceTest.java (1)
538-564: Manual verification approval/rejection tests look good; unused locals are optional cleanupThe two new tests correctly exercise the
updatePacketStatusbranches forreturnValue4 (rejected) and 1 (approved) and fit the existing mocking pattern in this class. If you want to reduce noise, you could drop the unusedresponse,ActiveMQBytesMessage, andByteSequencelocals (they’re not used in the assertions or method under test), or factor the common setup into a small helper—but that’s purely optional given the current objectives.registration-processor/core-processor/registration-processor-bio-dedupe-stage/src/test/java/io/mosip/registration/processor/biodedupe/stage/BioDedupeProcessorTest.java (1)
951-1002: New CBEFF and pre‑ABIS tests improve coverage; consider a couple of small refinementsThe three tests nicely extend coverage for:
- CBEFF missing surfaced via
CbeffNotFoundExceptionongetPacketStatus.- Generic exception in the lost/update UIN path.
- NEW pre‑ABIS, no‑CBEFF, non‑infant flow with success routing and status updates.
Two optional nits you might consider:
- In
testNewPacketPreAbisIdentification_WhenCbeffAbsent_ShouldSetDedupeSuccessStatus, the bus‑address assertion uses the inputmessageDTOinstead of the returnedmessageDto. Today they’re likely the same instance, but asserting on the returned object would better express intent and be more robust if the implementation ever changes:- assertEquals(MessageBusAddress.BIO_DEDUPE_BUS_IN, messageDTO.getMessageBusAddress()); + assertEquals(MessageBusAddress.BIO_DEDUPE_BUS_IN, messageDto.getMessageBusAddress());
- In
testLostAndUpdateUin_WhenExceptionOccurs_ShouldSetValidStatusFalseAndInternalErrorTrue, the name mentions “LostAndUpdateUin” but the setup relies on the default"new"registration type from the sharedregistrationStatusDto. Either renaming the test or explicitly configuring the registration type to LOST/UPDATE would make the intent clearer.registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java (1)
2779-2983: New UIN generator edge‑case tests are solid; only minor cleanup opportunitiesThe added tests at the bottom of this class provide good coverage for several important scenarios:
testProcess_UinGenerationReprocess_ShouldSetReprocessStatusandtestProcess_UinGenerationFailed_ShouldSetReprocessStatusverify that specific IDRepo error codes (e.g.,IDR-IDC-004,IDR-IDC-005) correctly drivePACKET_UIN_GENERATION_REPROCESS/PACKET_UIN_GENERATION_FAILEDmappings and update the transaction status toREPROCESSwhile returning a valid message withinternalError = true.
testLostAndUpdateUin_WhenIdRepoDraftReturnsNull_ShouldSetValidStatusFalseexercises the LOST path where the IDRepo draft update yields no usable response, ensuring the stage marks the message invalid without internal error.
testReactivateUin_WhenStatusActivatedcovers the reactivation path when IDRepo responds with an activated status, asserting the stage treats it as a clean success (valid and no internal error).
testUinUpdate_WhenIdRepoStatusDrafted_ShouldReturnTrueconfirms that an UPDATE flow with IDRepo statusDRAFTEDis treated as a happy path.A few optional polish points you might consider:
- Avoid redundant stubbing / unused locals in the LOST test
In
testLostAndUpdateUin_WhenIdRepoDraftReturnsNull_ShouldSetValidStatusFalsethere are some leftovers that could be trimmed without changing behavior:
stris defined but never used.packetManagerService.getFieldByMappingJsonKey(...)is stubbed twice, with different matcher forms; only the last stub will actually be effective.You could safely drop the unused variable and the first stub for clarity.
- Clarify status constant usage in
testReactivateUin_WhenStatusActivatedHere you set the IDRepo response status via
io.mosip.registration.processor.status.code.RegistrationType.ACTIVATED.toString(). If IDRepo has its own explicit status constants (or if"ACTIVATED"is the canonical value in that domain), using those instead would reduce coupling between registration-status enums and IDRepo payload semantics. Functionally it’s fine as long as the strings match; this is more about readability and domain separation.
- Document the assumption about
registrationStatusDtomutation (optional)Several tests, including the new reprocess/failed ones, assert on
registrationStatusDto.getLatestTransactionStatusCode(). Given the setup stubsregistrationStatusService.getRegistrationStatus(...)to return this exact instance, this is a reasonable way to observe status changes. If you expect this pattern to remain stable, a brief comment near the field declaration (or insetup()) noting that the stage mutates the same DTO instance returned by the mock would make the intent explicit for future readers.None of these are blockers; the new tests themselves look consistent and valuable for coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
registration-processor/core-processor/registration-processor-bio-dedupe-stage/src/test/java/io/mosip/registration/processor/biodedupe/stage/BioDedupeProcessorTest.java(2 hunks)registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java(2 hunks)registration-processor/core-processor/registration-processor-verification-stage/src/test/java/io/mosip/registration/processor/verification/service/VerificationServiceTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-18T09:19:25.334Z
Learnt from: ashok-ksharma
Repo: mosip/registration PR: 2209
File: registration-processor/core-processor/registration-processor-uin-generator-stage/src/main/java/io/mosip/registration/processor/stages/uingenerator/stage/UinGeneratorStage.java:1168-1177
Timestamp: 2025-11-18T09:19:25.334Z
Learning: In the MOSIP registration processor, the `updatePacketCreatedOnInDemographicIdentity` method in `UinGeneratorStage.java` is intentionally limited to `RegistrationType.NEW` and `RegistrationType.UPDATE` only, because these are the registration types that update biometrics. `RES_UPDATE` and Mapped `UPDATE` do not update biometrics, so they should not trigger the `packetCreatedOn` update logic.
Applied to files:
registration-processor/core-processor/registration-processor-verification-stage/src/test/java/io/mosip/registration/processor/verification/service/VerificationServiceTest.javaregistration-processor/core-processor/registration-processor-bio-dedupe-stage/src/test/java/io/mosip/registration/processor/biodedupe/stage/BioDedupeProcessorTest.javaregistration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java
🧬 Code graph analysis (2)
registration-processor/core-processor/registration-processor-bio-dedupe-stage/src/test/java/io/mosip/registration/processor/biodedupe/stage/BioDedupeProcessorTest.java (1)
registration-processor/registration-processor-core/src/main/java/io/mosip/registration/processor/core/abstractverticle/MessageBusAddress.java (1)
MessageBusAddress(18-266)
registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java (1)
registration-processor/registration-processor-core/src/main/java/io/mosip/registration/processor/core/constant/MappingJsonConstants.java (1)
MappingJsonConstants(3-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-maven-registration / maven-build
🔇 Additional comments (2)
registration-processor/core-processor/registration-processor-bio-dedupe-stage/src/test/java/io/mosip/registration/processor/biodedupe/stage/BioDedupeProcessorTest.java (1)
22-22: RegistrationType import is appropriate
RegistrationTypeis used intestNewPacketPreAbisIdentification_WhenCbeffAbsent_ShouldSetDedupeSuccessStatusto setreg_typetoNEW, so this import is correct and necessary.registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java (1)
245-245: UsingIDREPO_STATUSconstant avoids magic stringIntroducing
private static final String IDREPO_STATUS = "DRAFTED";centralizes the IDRepo “drafted” status value and fixes the earlier undefined constant issue, making the related test easier to read and maintain.
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
registration-processor/registration-processor-notification-service/src/test/java/io/mosip/registration/processor/notification/NotificationServiceTest.java (1)
822-856: Consider extracting common test setup into helper methods.The template setup pattern (creating
TemplateDto,TemplateResponseDto,ResponseWrapper, mock configurations) is repeated across nearly all new test methods. Extracting this into private helper methods would improve maintainability.Example refactor:
private ResponseWrapper<TemplateResponseDto> createTemplateResponseWrapper(String smsCode, String emailCode) { List<TemplateDto> templates = new ArrayList<>(); TemplateDto templateSMS = new TemplateDto(); templateSMS.setTemplateTypeCode(smsCode); templates.add(templateSMS); TemplateDto templateEmail = new TemplateDto(); templateEmail.setTemplateTypeCode(emailCode); templates.add(templateEmail); TemplateResponseDto templateResponseDto = new TemplateResponseDto(); templateResponseDto.setTemplates(templates); ResponseWrapper<TemplateResponseDto> responseWrapper = new ResponseWrapper<>(); responseWrapper.setResponse(templateResponseDto); responseWrapper.setErrors(null); return responseWrapper; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
registration-processor/registration-processor-notification-service/src/test/java/io/mosip/registration/processor/notification/NotificationServiceTest.java(3 hunks)registration-processor/registration-processor-packet-manager/src/test/java/io/mosip/registration/processor/packet/manager/service/impl/test/IdRepoServiceImplTest.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-maven-registration / maven-build
🔇 Additional comments (8)
registration-processor/registration-processor-notification-service/src/test/java/io/mosip/registration/processor/notification/NotificationServiceTest.java (4)
96-97: LGTM: Mock for Utilities addedThe
Utilitiesmock is appropriately added and will be used by the new test methods to stubgetInternalProcessbehavior.
902-944: Verify test name alignment with workflow type.The test is named
testProcess_SetTemplateType_UIN_UPDATEbut setsworkflowTypeto"ACTIVATED"(line 940). The mock returns"UPDATE"fromgetInternalProcess, which appears to be the intended internal process. Ensure the test name accurately reflects what is being tested, or clarify that this tests the mapping fromACTIVATED→UPDATE.
1388-1421: LGTM: Edge case test for unsupported notification type.This test verifies the behavior when an unsupported notification type ("FAX") is configured. The test correctly expects a 200 response, indicating graceful handling of this edge case.
1034-1066: LGTM: Notification type NONE scenario.This test correctly verifies behavior when notifications are disabled (set to "NONE"), ensuring the service still returns a 200 response without attempting to send notifications.
registration-processor/registration-processor-packet-manager/src/test/java/io/mosip/registration/processor/packet/manager/service/impl/test/IdRepoServiceImplTest.java (4)
3-3: LGTM: New imports for test coverage.The added imports (
assertEquals,ApiName,ResponseDTO) are properly used by the new test methods.Also applies to: 11-12
136-165: LGTM: Comprehensive test for non-null response scenario.The test properly mocks the REST client and
JsonUtilstatic methods, then verifies that the returnedJSONObjectcontains the expected UIN value.
167-177: LGTM: Null response handling test.This test correctly verifies that
getIdJsonFromIDReporeturnsnullwhen the response's payload is null, ensuring proper null-safety in the implementation.
179-188: LGTM: Null response test for getIdResponseFromIDRepo.This test verifies the null handling path for a different method (
getIdResponseFromIDRepo), which returnsResponseDTOinstead ofJSONObject.
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
registration-processor/registration-processor-message-sender-impl/src/test/java/io/mosip/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java (3)
18-26: New imports and mocks are consistent; consider simplifyingInputStreammockingThe additional imports and mocks (for
RequestWrapper,SyncRegistrationService,Decryptor,InputStream, andFile) line up with the new test cases and look correct.One small clean-up:
isis annotated as a Mockito mock but is later reassigned to a realFileInputStream. To avoid confusion between “mock” and “real” usage, you can drop the@Mockonisand just treat it as a plain field (or a local variable in the test):- @Mock - private InputStream is; + private InputStream is;Also applies to: 133-140, 183-183
519-535: Assert the SMS response in the preferred-languages success testCurrently this test only verifies that no exception is thrown, even though the setup clearly expects a successful
"OK"status. Asserting on the returnedSmsResponseDtowill make the test a stronger regression guard:- messageNotificationServiceImpl.sendSmsNotification("RPR_UIN_GEN_SMS", "12345", "NEW", IdType.RID, attributes, - RegistrationType.NEW.name()); + SmsResponseDto result = messageNotificationServiceImpl.sendSmsNotification( + "RPR_UIN_GEN_SMS", "12345", "NEW", IdType.RID, attributes, RegistrationType.NEW.name()); + assertEquals("Expected OK status when preferred languages are set", "OK", result.getStatus());
556-589: JSON-parse-path test mixes naming, resource handling, and open stream; tighten it upThis test drives the “JSON parse / optional values” path using
SyncRegistrationEntity,Decryptor, and a ZIP file, which is useful for coverage, but there are a few rough edges:
The method name
testSendSmsNotification_ShouldThrowJsonParseExceptionimplies that an exception is expected, yet the test does not declareexpected=...or assert on any exception; it simply runs through the flow and allows a normal return. Consider either:
- Renaming to something like
...ShouldHandleJsonParseExceptionGracefully, or- Explicitly arranging and asserting that a specific parse exception is thrown.
The test uses a hard-coded relative path and leaves the stream open:
file1 = new File("src/test/resources/1001.zip"); is = new FileInputStream(file1); IOUtils.toByteArray(is); Mockito.when(decryptor.decrypt(...)).thenReturn(is);This is a bit brittle (depends on working directory) and leaks the
InputStream. You can make it more robust with a local file + try-with-resources:
file1 = new File("src/test/resources/1001.zip");is = new FileInputStream(file1);IOUtils.toByteArray(is);Mockito.when(decryptor.decrypt(Mockito.any(), Mockito.any(),Mockito.any())).thenReturn(is);
File file = new File("src/test/resources/1001.zip");try (InputStream decryptedStream = new FileInputStream(file)) {IOUtils.toByteArray(decryptedStream);Mockito.when(decryptor.decrypt(Mockito.any(), Mockito.any(), Mockito.any())).thenReturn(decryptedStream);// existing mocking of restClientService and the call to sendSmsNotification// can remain inside this try block}These changes keep the behavior the same while clarifying intent and avoiding resource leaks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
registration-processor/registration-processor-message-sender-impl/src/test/resources/1001.zipis excluded by!**/*.zip
📒 Files selected for processing (1)
registration-processor/registration-processor-message-sender-impl/src/test/java/io/mosip/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-maven-registration / maven-build
🔇 Additional comments (1)
registration-processor/registration-processor-message-sender-impl/src/test/java/io/mosip/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java (1)
509-517: Template-generator ApisResourceAccessException path is exercised correctlyThis test cleanly verifies that when
templateGenerator.getTemplatethrowsApisResourceAccessException,sendSmsNotificationpropagates it (via theexpectedattribute). Stubbing is minimal and focused on the failing dependency, which is good for readability.
...p/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java
Show resolved
Hide resolved
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
registration-processor/registration-processor-packet-manager/src/test/java/io/mosip/registration/processor/packet/manager/service/impl/test/IdRepoServiceImplTest.java (2)
145-174: Good coverage of the happy path scenario.This test properly validates that
getIdJsonFromIDRepocorrectly extracts identity JSON when the response is non-null. The mocking strategy is consistent with existing tests in the file.Consider adding
Mockito.verify()calls to ensure the REST client is invoked with the expected parameters, which would strengthen the test:Mockito.verify(restClientService).getApi( Mockito.eq(ApiName.RETRIEVEIDENTITYFROMRID), anyList(), anyString(), Mockito.eq("123"), Mockito.eq(ResponseWrapper.class) );
199-216: Excellent coverage of metadata search scenarios.These three tests comprehensively cover the
searchIdVidMetadatamethod across success, null response, and error conditions. The assertions appropriately validate the expected behavior in each scenario.Consider adding a test case for exception handling when
restClientService.postApithrowsApisResourceAccessException, since this exception is declared in the method signature (line 200):@Test(expected = ApisResourceAccessException.class) public void testSearchIdVidMetadata_ThrowsException() throws ApisResourceAccessException, IOException { IdVidMetadataRequest request = new IdVidMetadataRequest(); Mockito.when(restClientService.postApi(any(), any(), any(), any(), Mockito.any(RequestWrapper.class), Mockito.eq(ResponseWrapper.class))) .thenThrow(new ApisResourceAccessException("Connection failed")); idRepoService.searchIdVidMetadata(request); }Also applies to: 218-231, 233-245
registration-processor/core-processor/registration-processor-manual-adjudication-stage/src/test/java/io/mosip/registration/processor/verification/service/ManualAdjudicationServiceTest.java (3)
817-856: Datashare URL flow test is well‑targeted; consider tightening expectationsThis test gives good coverage of the biometrics/datashare path and correctly asserts an invalid message without internal error. If you want to further couple it to the datashare path, you could also verify that the expected mapper or REST interactions occur (e.g.,
Mockito.verify(mapper, atLeastOnce()).readValue(...)), but that’s optional.
858-882: Rejected-status success-flow test is reasonable, minor clarity tweak optionalThe new test cleanly covers the
returnValue = 4/ REJECTED branch and asserts a successfulupdatePacketStatusresult. IfmanualVerificationDTO.setStatusCode(ManualVerificationStatus.REJECTED.name())does not influenceManualAdjudicationServiceImplvia injection, it may be dead setup; consider removing or asserting on it for clarity.
884-899: Strengthen invalid-RID test by verifying the mapper interactionThe test correctly asserts that an empty RID leads to
internalError = trueandisValid = false. Since you also stubregistrationExceptionMapperUtil.getStatusCode(MANUAL_VERIFICATION_FAILED), you could make the intent more explicit by verifying that this mapper is actually invoked:MessageDTO response = manualAdjudicationService.process(object, queue); assertTrue(response.getInternalError()); assertFalse(response.getIsValid()); + + Mockito.verify(registrationExceptionMapperUtil) + .getStatusCode(RegistrationExceptionTypeCode.MANUAL_VERIFICATION_FAILED);This keeps behavior unchanged while ensuring the new exception-mapping path is really exercised.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
registration-processor/core-processor/registration-processor-manual-adjudication-stage/src/test/java/io/mosip/registration/processor/verification/service/ManualAdjudicationServiceTest.java(2 hunks)registration-processor/registration-processor-packet-manager/src/test/java/io/mosip/registration/processor/packet/manager/service/impl/test/IdRepoServiceImplTest.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
registration-processor/registration-processor-packet-manager/src/test/java/io/mosip/registration/processor/packet/manager/service/impl/test/IdRepoServiceImplTest.java (1)
registration-processor/registration-processor-core/src/main/java/io/mosip/registration/processor/core/util/JsonUtil.java (1)
JsonUtil(34-317)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-maven-registration / maven-build
🔇 Additional comments (3)
registration-processor/registration-processor-packet-manager/src/test/java/io/mosip/registration/processor/packet/manager/service/impl/test/IdRepoServiceImplTest.java (2)
3-3: LGTM! New imports support expanded test coverage.The newly imported assertions and DTOs are all utilized in the new test methods, supporting the enhanced test scenarios for null responses, error conditions, and metadata operations.
Also applies to: 5-5, 12-18, 43-44
176-186: LGTM! Essential null-safety tests.These tests properly validate that both
getIdJsonFromIDRepoandgetIdResponseFromIDRepohandle null responses gracefully, returning null rather than throwing exceptions. This adds important edge-case coverage.Also applies to: 188-197
registration-processor/core-processor/registration-processor-manual-adjudication-stage/src/test/java/io/mosip/registration/processor/verification/service/ManualAdjudicationServiceTest.java (1)
25-25: Import forRegistrationExceptionTypeCodeis appropriateThe new import is required for the invalid-RID test and is correctly scoped to tests only.
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
registration-processor/registration-processor-message-sender-impl/src/test/java/io/mosip/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java (2)
519-554: Strengthen preferred-language SMS tests with explicit assertionsBoth language‑preference tests only assert indirectly (first test not at all, second only by not failing). To make the behavior clearer and less brittle, consider asserting on the returned
SmsResponseDtoand/or verifying the REST client interaction, for example:- messageNotificationServiceImpl.sendSmsNotification("RPR_UIN_GEN_SMS", "12345", "NEW", IdType.RID, attributes, - RegistrationType.NEW.name()); + SmsResponseDto result = messageNotificationServiceImpl.sendSmsNotification( + "RPR_UIN_GEN_SMS", "12345", "NEW", IdType.RID, attributes, RegistrationType.NEW.name()); + assertEquals("OK", result.getStatus());and similarly for the failure‑then‑OK case, plus an optional:
+ verify(restClientService).postApi(any(), any(), any(), Mockito.any(RequestWrapper.class), + Mockito.eq(ResponseWrapper.class));This keeps the tests validating the actual outcome instead of just the absence of exceptions.
18-26: Align JSON‑parse test name with behavior and tidy InputStream handling
testSendSmsNotification_ShouldThrowJsonParseExceptioncurrently callssendSmsNotificationwithout@Test(expected = …)or an assertion, so the test passes whether or not any JSON parse exception is thrown. The name suggests you expect an exception; either:
- rename to something like
testSendSmsNotification_JsonParseErrorPath_ShouldCompleteWithoutFailure, or- add
expected = JsonProcessingException.class(or the exact type thrown) to the@Testto actually assert that behavior.@Mock private InputStream is;is later overwritten with a realFileInputStream; the@Mockisn’t buying anything here and can be dropped in favor of a plain field or a local variable.- The
FileInputStreamcreated forfile1is never closed; using try‑with‑resources or closing it in afinally/@Afterwould avoid leaving open file handles even in tests.These are cosmetic/robustness issues and don’t block the PR, but tightening them would make the tests’ intent clearer.
Also applies to: 133-141, 183-184, 556-589
registration-processor/registration-processor-notification-service/src/test/java/io/mosip/registration/processor/notification/NotificationServiceTest.java (2)
822-1032: Fallback-to-email LOST/UIN_CREATED/ACTIVATED/DEACTIVATED/RES_UPDATE tests could assert the actual channel behaviorThe new tests (
testProcess_LostWorkflow_UsesEmailWhenSmsFails, and the UIN_CREATED/ACTIVATED/DEACTIVATED/RES_UPDATE variants) correctly set up:
- SMS to throw
PhoneNumberNotFoundException, and- EMAIL to return a successful
ResponseDto,then assert a 200 response. To better lock in the intended behavior, consider also verifying that:
service.sendSmsNotification(..)is invoked once, andservice.sendEmailNotification(..)is invoked as the fallback,for example:
- ResponseEntity<Void> result = notificationService.process(completedEventDTO); - assertEquals(200, result.getStatusCodeValue()); + ResponseEntity<Void> result = notificationService.process(completedEventDTO); + assertEquals(200, result.getStatusCodeValue()); + verify(service).sendSmsNotification(any(), any(), any(), any(), any(), any()); + verify(service).sendEmailNotification(any(), any(), any(), any(), any(), any(), any(), any(), any());This keeps the tests from passing even if the implementation silently skips one of the channels.
1034-1413: notificationTypes‑specific tests are thorough but heavily duplicated; consider small helpersThe SMS‑only, EMAIL‑only, NONE, and FAX/TemplateNotFoundException tests do a good job exercising different
notificationTypesvalues and error paths (status failed vs success, various exceptions). However, the setup for templates, wrappers, env properties, and stubbed SMS/EMAIL responses is almost identical across many methods.You could improve maintainability by extracting small helpers, e.g.:
buildTemplateResponseWrapper(String smsCode, String emailCode)to createTemplateResponseDto+ResponseWrapper.stubLostUinEnvAndTemplates()to centralize the commonenv.getProperty(..)andrestClientService.getApi(..)stubbing.That would reduce boilerplate while preserving the expanded coverage.
registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java (2)
2658-2777: PacketCreatedOn tests: verify you’re asserting the intended timestamp source and registrationTypeThe new
updatePacketCreatedOnInDemographicIdentitytests add useful coverage, but a couple of details are worth double‑checking:
- In
testNewPacketCreatedOn_InsertsWhenMetaInfoPresentandtestProcessUpdate_WithPacketCreatedOnyou:
- populate
metaInfowithMappingJsonConstants.PACKET_CREATED_ON = "2025-...Z", and- assert that
demographicIdentity.get("packetCreatedOn")equals"2019-01-17T06:29:01.940Z", which comes from the globalutility.retrieveCreatedDateFromPacket(..)stub.
This means the tests are validating the fallback value, not the metaInfo value, despite the method names mentioning “WhenMetaInfoPresent” / “WithPacketCreatedOn”.- In the UPDATE test,
packetManagerService.getMetaInfois stubbed with the hardcoded"NEW"registrationType:while bothwhen(packetManagerService.getMetaInfo(rid, "NEW", ProviderStageName.UIN_GENERATOR)).thenReturn(metaInfo);messageDTOandinternalRegistrationStatusDtoare set to"UPDATE". If the production code passes"UPDATE"togetMetaInfo, this stub will never be hit, and your test will again only exercise the fallback path.It would be good to confirm that:
- the method is supposed to prefer
metaInfovs.retrieveCreatedDateFromPacket(and adjust the assertion accordingly), and- the registrationType argument used in the
getMetaInfostub matches whatUinGeneratorStageactually passes for UPDATE flows.The NEW/LOST/no‑meta tests that assert
packetCreatedOnremainsnulllook consistent with the intent that only NEW/UPDATE should get this field populated.Based on learnings, the restriction of
updatePacketCreatedOnInDemographicIdentityto NEW/UPDATE only is correct; this comment is only about which timestamp and stubbed registrationType you validate.
2779-2837: Reprocess/failed IDRepo draft tests now assert on MessageDTO flags instead of mocksThe new tests:
testProcess_UinGenerationReprocess_ShouldSetReprocessStatustestProcess_UinGenerationFailed_ShouldSetReprocessStatusset up specific IDRepo error codes and map them via
registrationStatusMapperUtiltoRegistrationTransactionStatusCode.REPROCESS, then assertresult.getInternalError()andresult.getIsValid()are bothtrue. This is a more robust pattern than asserting on the preconfiguredregistrationStatusDtofixture and aligns with the observable behavior ofprocess().If you want even stronger checks, you could additionally verify that
registrationStatusService.addRegistrationTransaction(..)was called with a DTO whose latest transaction status isREPROCESS, but that’s optional given the current coverage:ArgumentCaptor<InternalRegistrationStatusDto> captor = ArgumentCaptor.forClass(InternalRegistrationStatusDto.class); verify(registrationStatusService).addRegistrationTransaction(any(), captor.capture()); assertEquals(RegistrationTransactionStatusCode.REPROCESS.toString(), captor.getValue().getLatestTransactionStatusCode());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java(2 hunks)registration-processor/registration-processor-message-sender-impl/src/test/java/io/mosip/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java(4 hunks)registration-processor/registration-processor-notification-service/src/test/java/io/mosip/registration/processor/notification/NotificationServiceTest.java(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-18T09:19:25.334Z
Learnt from: ashok-ksharma
Repo: mosip/registration PR: 2209
File: registration-processor/core-processor/registration-processor-uin-generator-stage/src/main/java/io/mosip/registration/processor/stages/uingenerator/stage/UinGeneratorStage.java:1168-1177
Timestamp: 2025-11-18T09:19:25.334Z
Learning: In the MOSIP registration processor, the `updatePacketCreatedOnInDemographicIdentity` method in `UinGeneratorStage.java` is intentionally limited to `RegistrationType.NEW` and `RegistrationType.UPDATE` only, because these are the registration types that update biometrics. `RES_UPDATE` and Mapped `UPDATE` do not update biometrics, so they should not trigger the `packetCreatedOn` update logic.
Applied to files:
registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java
🔇 Additional comments (4)
registration-processor/registration-processor-message-sender-impl/src/test/java/io/mosip/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java (1)
509-517: Template-generator ApisResourceAccessException path is covered correctlyStubbing
templateGenerator.getTemplate(..)to throwApisResourceAccessExceptionand using@Test(expected = ApisResourceAccessException.class)exercises the propagation path cleanly; the setup and assertion look consistent with the method contract.registration-processor/registration-processor-notification-service/src/test/java/io/mosip/registration/processor/notification/NotificationServiceTest.java (1)
5-14: New imports and Utilities mock are consistent with added notification-flow testsThe additional imports (
anyString,HashMap,Map,TemplateNotFoundException,Utilities) and the new@Mock Utilities utilitiesare all exercised in the later tests (e.g.,getInternalProcessand template‑error scenarios). Nothing blocking here.Also applies to: 95-99
registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java (2)
245-245: IDREPO_STATUS constant cleanly fixes the earlier undefined-symbol issueIntroducing
private static final String IDREPO_STATUS = "DRAFTED";and using it intestUinUpdate_WhenIdRepoStatusDrafted_ShouldReturnTrueremoves the magic string from the test and resolves the previous compilation problem aroundIDREPO_STATUS. The test now clearly documents that a drafted status from IDRepo is treated as a successful update by the stage.Also applies to: 2933-2967
2840-2979: Additional LOST/reactivation/drafted-status tests look consistent with stage behaviorThe new tests around:
- LOST + UPDATE UIN with null IDRepo draft (
testLostAndUpdateUin_WhenIdRepoDraftReturnsNull_ShouldSetValidStatusFalse),- reactivation where IDRepo already reports ACTIVATED (
testReactivateUin_WhenStatusActivated), and- UPDATE where IDRepo status is
"DRAFTED"(testUinUpdate_WhenIdRepoStatusDrafted_ShouldReturnTrue),all follow the existing setup patterns (UIN field maps, defaultFields, IDRepo draft stubbing) and assert on
MessageDTO.getIsValid()/getInternalError()in ways that are consistent with the rest of the suite (treating null drafts as invalid-but-not-internal-error, respecting ACTIVATED/DRAFTED as success cases).No blocking issues here from a test-logic perspective.
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
registration-processor/core-processor/registration-processor-verification-stage/src/test/java/io/mosip/registration/processor/verification/service/VerificationServiceTest.java (1)
538-564: Trim unused JMS setup and consider stronger assertionsThe two new tests correctly exercise additional
updatePacketStatusbranches, but there are some easy cleanups:
- The
ActiveMQBytesMessage/ByteSequenceand JSON string are built but never passed toverificationService.updatePacketStatus, so they can be dropped here without changing behavior.- Both tests duplicate the same setup pattern already used above; if you plan to add more status-code tests, consider extracting a small helper to reduce repetition.
- If feasible, assert something more specific than just
result(e.g., updated status on the returnedVerificationEntityor verifying repository interactions), so the tests better reflect the “...ShouldSet...Status” intent.registration-processor/registration-processor-notification-service/src/test/java/io/mosip/registration/processor/notification/NotificationServiceTest.java (2)
825-1035: Fallback workflow tests are correct but could be made less brittleThe new tests:
testProcessLostWorkflowUsesEmailWhenSmsFailstestProcessUinCreatedWorkflowFallbacksToEmailWhenSmsFailstestProcessActivatedWorkflowFallbacksToEmailWhenSmsFailstestProcessDeactivatedWorkflowFallbacksToEmailWhenSmsFailstestProcessResUpdateWorkflowFallbackToEmailWhenSmsFailsnicely cover the SMS→email fallback paths for LOST/UIN_CREATED/ACTIVATED/DEACTIVATED/RES_UPDATE, including the
additionalProcessCategoryForNotificationmap andutilities.getInternalProcess(...). Functionally they look fine and should increase coverage as intended.Two optional robustness tweaks:
- Use argument matchers when stubbing
utilities.getInternalProcessso the tests don’t depend on the exact sameMapinstance being passed:Mockito.when(utilities.getInternalProcess(Mockito.anyMap(), Mockito.eq("UIN_CREATED"))) .thenReturn("NEW");(and similarly for
ACTIVATED,DEACTIVATED,RES_UPDATE).
- If you want to assert the fallback behavior more strongly, you could add a
Mockito.verify(service).sendEmailNotification(...)and/or verify that SMS was attempted once and failed, rather than only asserting HTTP 200.These are quality improvements only; current tests are acceptable as-is.
1037-1416: Notification type and exception-path tests are valid but quite repetitiveThe new tests for:
notificationTypes=NONE,SMS,FAX(e.g.,testProcessWhenNotificationTypeIsNone,testProcessNotificationTypesSendSMSSuccess,testProcessNotificationTypesSendEmailFailed, etc.)- Various exception scenarios (
ApisResourceAccessException,PhoneNumberNotFoundException,EmailIdNotFoundException,TemplateGenerationFailedException,TemplateNotFoundException)are consistent with the existing style and clearly aim to exercise different branches while ensuring
notificationService.process(...)still returns HTTP 200. From a correctness standpoint, they look fine.A few optional improvements to consider (mainly for maintainability and test strength):
- Reduce duplication: Most tests repeat the same setup for
TemplateDto/TemplateResponseDto/ResponseWrapperand SMS/email response DTOs. A couple of small helper methods (e.g.,buildTemplateResponse(String smsCode, String emailCode),buildWrapper(TemplateResponseDto dto)) or parameterized tests would significantly cut boilerplate and make future changes easier.- Interaction-focused assertions: Right now the only assertion is the HTTP 200 status. For some scenarios (e.g.,
notificationTypes = NONE, or SMS-only/EMAIL-only modes), addingMockito.verify/verifyNoMoreInteractionsfor SMS vs email sends would better confirm that the right channels are (or are not) invoked.- Ensure exception stubs are actually exercised: In tests that stub exception-throwing behavior on
sendSmsNotification/sendEmailNotification, consider addingverify(..., times(1))on those calls so the test will fail if the production logic changes and stops hitting those branches.These are non-blocking suggestions; the current tests already serve the PR’s coverage goals.
registration-processor/registration-processor-packet-manager/src/test/java/io/mosip/registration/processor/packet/manager/service/impl/test/IdRepoServiceImplTest.java (1)
145-174: Make the staticJsonUtil.getJSONObjectstub less brittleThe stub currently matches a specific
identityinstance:PowerMockito.when(JsonUtil.getJSONObject(identity, "Identity")) .thenReturn(demo);If
IdRepoServiceImplever passes a different but equivalentJSONObject, this test will start failing. Prefer argument matchers to decouple from the exact instance:- PowerMockito.when(JsonUtil.getJSONObject(identity, "Identity")) - .thenReturn(demo); + PowerMockito.when(JsonUtil.getJSONObject(any(JSONObject.class), Mockito.eq("Identity"))) + .thenReturn(demo);This still validates behavior while making the test more robust to internal implementation changes.
registration-processor/registration-processor-message-sender-impl/src/test/java/io/mosip/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java (2)
133-141: New sync/decryptor mocks are coherent; minor cleanup possibleThe new mocks (
SyncRegistrationService<SyncResponseDto, SyncRegistrationDto>,Decryptor, andInputStream is) andFile file1field line up with the new JSON-parse path you’re exercising.One minor cleanup: since
isis reassigned to a realFileInputStreamin the JsonParse test, the@Mockannotation onInputStream isdoesn’t add value and could be dropped (orismade a local variable in that test) to avoid confusion between mocks and real streams.Also applies to: 183-183
519-535: Add assertions to match “ShouldSendSmsSuccessfully” intentHere you configure
packetManagerService.getFieldto return a preferred language and stubrestClientService.postApito respond with status"OK", but you don’t assert on the returnedSmsResponseDtoor verify thatpostApiis invoked.To make this test actually validate “ShouldSendSmsSuccessfully”, consider capturing the result and/or verifying the interaction, for example:
- messageNotificationServiceImpl.sendSmsNotification("RPR_UIN_GEN_SMS", "12345", "NEW", IdType.RID, attributes, - RegistrationType.NEW.name()); + SmsResponseDto result = messageNotificationServiceImpl.sendSmsNotification( + "RPR_UIN_GEN_SMS", "12345", "NEW", IdType.RID, attributes, RegistrationType.NEW.name()); + assertEquals("OK", result.getStatus()); + verify(restClientService).postApi(any(), any(), any(), Mockito.any(RequestWrapper.class), + Mockito.eq(ResponseWrapper.class));This keeps coverage while ensuring the behavior matches the test name.
registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java (1)
2840-2876: Lost + update flow with null draft response is modeled correctlyRelying on Mockito’s default
nullreturn fromidrepoDraftService.idrepoUpdateDraftmatches the intent in the test name (“…ReturnsNull…”), and the expectations (isValid == false,internalError == false) clearly encode the behavior for this edge case.If you want to make the scenario more explicit for future readers, you could also stub
idrepoDraftService.idrepoUpdateDraft(...)to returnnulldirectly, but that’s optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
registration-processor/core-processor/registration-processor-bio-dedupe-stage/src/test/java/io/mosip/registration/processor/biodedupe/stage/BioDedupeProcessorTest.java(2 hunks)registration-processor/core-processor/registration-processor-manual-adjudication-stage/src/test/java/io/mosip/registration/processor/verification/service/ManualAdjudicationServiceTest.java(2 hunks)registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java(2 hunks)registration-processor/core-processor/registration-processor-verification-stage/src/test/java/io/mosip/registration/processor/verification/service/VerificationServiceTest.java(1 hunks)registration-processor/registration-processor-message-sender-impl/src/test/java/io/mosip/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java(4 hunks)registration-processor/registration-processor-notification-service/src/test/java/io/mosip/registration/processor/notification/NotificationServiceTest.java(3 hunks)registration-processor/registration-processor-packet-manager/src/test/java/io/mosip/registration/processor/packet/manager/service/impl/test/IdRepoServiceImplTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- registration-processor/core-processor/registration-processor-manual-adjudication-stage/src/test/java/io/mosip/registration/processor/verification/service/ManualAdjudicationServiceTest.java
- registration-processor/core-processor/registration-processor-bio-dedupe-stage/src/test/java/io/mosip/registration/processor/biodedupe/stage/BioDedupeProcessorTest.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-18T09:19:25.334Z
Learnt from: ashok-ksharma
Repo: mosip/registration PR: 2209
File: registration-processor/core-processor/registration-processor-uin-generator-stage/src/main/java/io/mosip/registration/processor/stages/uingenerator/stage/UinGeneratorStage.java:1168-1177
Timestamp: 2025-11-18T09:19:25.334Z
Learning: In the MOSIP registration processor, the `updatePacketCreatedOnInDemographicIdentity` method in `UinGeneratorStage.java` is intentionally limited to `RegistrationType.NEW` and `RegistrationType.UPDATE` only, because these are the registration types that update biometrics. `RES_UPDATE` and Mapped `UPDATE` do not update biometrics, so they should not trigger the `packetCreatedOn` update logic.
Applied to files:
registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java
🧬 Code graph analysis (3)
registration-processor/registration-processor-message-sender-impl/src/test/java/io/mosip/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java (1)
registration-processor/registration-processor-registration-status-service-impl/src/main/java/io/mosip/registration/processor/status/dto/SyncRegistrationDto.java (1)
SyncRegistrationDto(21-361)
registration-processor/registration-processor-packet-manager/src/test/java/io/mosip/registration/processor/packet/manager/service/impl/test/IdRepoServiceImplTest.java (1)
registration-processor/registration-processor-core/src/main/java/io/mosip/registration/processor/core/util/JsonUtil.java (1)
JsonUtil(34-317)
registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java (1)
registration-processor/registration-processor-core/src/main/java/io/mosip/registration/processor/core/constant/MappingJsonConstants.java (1)
MappingJsonConstants(3-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-maven-registration / maven-build
🔇 Additional comments (13)
registration-processor/registration-processor-notification-service/src/test/java/io/mosip/registration/processor/notification/NotificationServiceTest.java (1)
5-17: New imports andUtilitiesmock wiring look correctAdding
anyString,HashMap/Map, and the@Mock Utilities utilitiesfield integrates cleanly with the existing Mockito/PowerMock setup and supports the new tests that driveNotificationServiceImplviautilities.getInternalProcess(...). No issues spotted here.Also applies to: 96-101
registration-processor/registration-processor-packet-manager/src/test/java/io/mosip/registration/processor/packet/manager/service/impl/test/IdRepoServiceImplTest.java (5)
3-18: Imports align with new test scenariosAll added imports (assertions, DTOs, ApiName, RequestWrapper, ErrorDTO, IOException, List) are used and consistent with the new tests; no issues here.
Also applies to: 43-45
176-197: Null-response handling tests for ID repo look correctThe tests
testGetIdJsonFromIDRepoWhenResponseNullandtestGetIdResponseFromIDRepoWhenResponseNullcleanly stubrestClientService.getApi(...)to return wrappers withresponse == nulland assert that the service methods also returnnull. This directly exercises the null-guard branches without over-coupling to implementation details; no changes needed.
199-216: Good coverage ofsearchIdVidMetadatasuccess path
testSearchIdVidMetadataSuccesscorrectly sets up a wrapper with a non-nullIdVidMetadataResponse, stubspostApi(...), and verifies both non-null result and that theridis propagated. This is a focused and sufficient positive-path test.
218-231: Null-response variant forsearchIdVidMetadatais appropriate
testSearchIdVidMetadataNullResponsecovers the case where the wrapper’sresponseisnulland asserts that the service returnsnullas well, which neatly validates the defensive behavior for an empty payload.
233-245: Error-response handling forsearchIdVidMetadatais well covered
testSearchIdVidMetadataErrorResponsesetserrorson the wrapper and verifies that the service returnsnull, which is the expected behavior when the downstream call reports errors. This closes the main triad of success/null/error scenarios for this method.registration-processor/registration-processor-message-sender-impl/src/test/java/io/mosip/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java (2)
509-517: Template-generator ApisResourceAccessException test looks correctThis test cleanly verifies that when
templateGenerator.getTemplatethrowsApisResourceAccessException,sendSmsNotificationpropagates it as expected via@Test(expected = ApisResourceAccessException.class). Stubbing and expectations are consistent with the method name and intent.
537-554: Preferred-language failure fallback test is clear and alignedThis test now clearly exercises the fallback when
packetManagerService.getFieldthrowsApisResourceAccessExceptionand asserts that the SMS still succeeds with"OK"status. The behavior, method name, and assertion are all aligned.registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java (5)
245-245: IDREPO_STATUS constant correctly introduced and usedDefining
IDREPO_STATUS = "DRAFTED"here cleanly resolves the previous undefined symbol and documents the expected IDRepo status used later in the new test.
2780-2807: Reprocess-path test setup and assertions look correctThis test cleanly simulates an
IDR-IDC-004error, maps it viaPACKET_UIN_GENERATION_REPROCESStoREPROCESS, and asserts on the resultingMessageDTOflags (internalError == true,isValid == true), which aligns with the existing contract for reprocessable failures.
2810-2837: Failed-generation reprocess scenario is well-coveredThe
IDR-IDC-005path wired throughPACKET_UIN_GENERATION_FAILEDtoREPROCESSis exercised in the same pattern as the previous test, again asserting onMessageDTOrather than any mocked DTO, which avoids the earlier issue and accurately validates observable behavior.
2879-2930: Reactivation scenario with ACTIVATED status is sensibly assertedThis test drives the reactivation path where the draft update returns an
ACTIVATEDstatus and a follow-upgetApi/patchApiinteraction yields a non-error response, with final expectations ofisValid == trueandinternalError == false. The mocks and assertions are coherent with the other activation/deactivation tests in this class.
2933-2979: DRAFTED-status update behavior is validated and uses shared constantThe new test for UPDATE when IDRepo returns a
DRAFTEDstatus correctly expectsisValid == trueandinternalError == false, and it reuses theIDREPO_STATUSconstant instead of a magic string, which improves readability and avoids typos. Setup mirrors the other UPDATE tests, so coverage is consistent.
...p/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java
Outdated
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)
registration-processor/registration-processor-message-sender-impl/src/test/java/io/mosip/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java (1)
140-141: Remove unused mock and field declarations.The
InputStream ismock field (lines 140-141) andFile file1field (line 184) are declared but never referenced in any test method. Line 576 creates a localFileInputStreaminstead of using these fields.Apply this diff to remove the unused declarations:
- @Mock - private InputStream is; - /** The attributes. */ private Map<String, Object> attributes = new HashMap<>();private VidsInfosDTO vidsInfosDTO = new VidsInfosDTO(); - private File file1; - /** * Setup.Also applies to: 184-184
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
registration-processor/registration-processor-message-sender-impl/src/test/java/io/mosip/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-maven-registration / maven-build
🔇 Additional comments (2)
registration-processor/registration-processor-message-sender-impl/src/test/java/io/mosip/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java (2)
510-518: LGTM!The test correctly verifies that
ApisResourceAccessExceptionfrom the template generator propagates to the caller.
538-555: LGTM!The test correctly verifies that SMS notification succeeds even when retrieving preferred languages fails, demonstrating graceful error handling.
...p/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java
Show resolved
Hide resolved
...p/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java
Show resolved
Hide resolved
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
...p/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
registration-processor/registration-processor-message-sender-impl/src/test/java/io/mosip/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java (1)
140-141: Remove unused mock fields.The
is(InputStream) mock field at lines 140-141 and thefile1field at line 184 are declared but never used in any test method. Test methodtestSendSmsNotificationShouldHandleJsonParseExceptionGracefullycreates its own localFileInputStreaminstead of using these fields.Apply this diff to remove the unused fields:
- @Mock - private InputStream is; - /** The attributes. */ private Map<String, Object> attributes = new HashMap<>();private VidsInfosDTO vidsInfosDTO = new VidsInfosDTO(); - private File file1; - /** * Setup.Also applies to: 184-184
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
registration-processor/registration-processor-message-sender-impl/src/test/java/io/mosip/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
registration-processor/registration-processor-message-sender-impl/src/test/java/io/mosip/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java (1)
registration-processor/registration-processor-registration-status-service-impl/src/main/java/io/mosip/registration/processor/status/dto/SyncRegistrationDto.java (1)
SyncRegistrationDto(21-361)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-maven-registration / maven-build
🔇 Additional comments (3)
registration-processor/registration-processor-message-sender-impl/src/test/java/io/mosip/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java (3)
510-518: LGTM!The test correctly verifies that
ApisResourceAccessExceptionpropagates when the template generator fails. The test name accurately reflects the behavior, and the mock setup properly simulates the failure scenario.
520-537: LGTM!The test properly verifies successful SMS notification sending when preferred languages are set. The mock setup is appropriate, and the assertion correctly validates the expected "OK" status.
539-556: LGTM!The test correctly verifies that SMS notification sending succeeds even when preferred language retrieval fails. The test name accurately reflects the graceful exception handling behavior, and the assertions properly validate the expected outcome.
...p/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java
Show resolved
Hide resolved
...ge/src/test/java/io/mosip/registration/processor/biodedupe/stage/BioDedupeProcessorTest.java
Outdated
Show resolved
Hide resolved
...ge/src/test/java/io/mosip/registration/processor/biodedupe/stage/BioDedupeProcessorTest.java
Outdated
Show resolved
Hide resolved
...ge/src/test/java/io/mosip/registration/processor/biodedupe/stage/BioDedupeProcessorTest.java
Outdated
Show resolved
Hide resolved
...ge/src/test/java/io/mosip/registration/processor/biodedupe/stage/BioDedupeProcessorTest.java
Outdated
Show resolved
Hide resolved
...ge/src/test/java/io/mosip/registration/processor/biodedupe/stage/BioDedupeProcessorTest.java
Outdated
Show resolved
Hide resolved
...p/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java
Show resolved
Hide resolved
...vice/src/test/java/io/mosip/registration/processor/notification/NotificationServiceTest.java
Show resolved
Hide resolved
registration-processor/registration-processor-message-sender-impl/src/test/resources/1001.zip
Show resolved
Hide resolved
.../io/mosip/registration/processor/packet/manager/service/impl/test/IdRepoServiceImplTest.java
Show resolved
Hide resolved
.../io/mosip/registration/processor/packet/manager/service/impl/test/IdRepoServiceImplTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
registration-processor/core-processor/registration-processor-bio-dedupe-stage/src/test/java/io/mosip/registration/processor/biodedupe/stage/BioDedupeProcessorTest.java (1)
951-971: Unresolved concern: artificially forcing unrealistic exception.A previous reviewer noted that
getPacketStatus()cannot throwCbeffNotFoundExceptionin production code. Your response indicated you're forcing this exception to cover a catch block. This is a testing anti-pattern:
- Tests should verify realistic behavior, not artificially trigger exception handlers
- If the catch block is genuinely unreachable, consider removing it from production code
- If it is reachable through other paths, create a test that exercises those realistic paths
Either remove this test and the corresponding unreachable catch block, or demonstrate a realistic scenario where
getPacketStatus()would throw this exception.
🧹 Nitpick comments (2)
registration-processor/core-processor/registration-processor-bio-dedupe-stage/src/test/java/io/mosip/registration/processor/biodedupe/stage/BioDedupeProcessorTest.java (2)
973-984: Incomplete test setup reduces test value.This test has minimal setup (only sets RID) and forces a generic
RuntimeExceptionwithout mocking the registration status retrieval. This makes the test shallow and unclear:
- No mock for
registrationStatusService.getRegistrationStatus()could cause different behavior than intended- Generic
RuntimeExceptiondoesn't specify what error scenario is being tested- The test doesn't verify which exception handler path is actually exercised
Consider either:
- Adding proper setup to test a specific, realistic error scenario with meaningful assertions
- Removing this test if the generic exception handler is already covered elsewhere
986-999: Consider improving test isolation to prevent flakiness.This test relies on the class-level
registrationStatusDtoinstance that's configured in the@Beforemethod (line 204 setsregistrationType="new"). This creates test interdependency:
- Lines 997-998 assert on shared
registrationStatusDtostate- If
setUp()changes or another test modifies this instance, this test could break or pass incorrectly- The test doesn't clearly show all its dependencies
Consider creating a fresh
InternalRegistrationStatusDtowithin this test method and mockingregistrationStatusService.getRegistrationStatus()to return it. This makes the test self-contained and easier to understand.@Test public void testNewPacketPreAbisIdentificationWhenCbeffAbsentShouldSetDedupeSuccessStatus() throws ApisResourceAccessException, PacketManagerException, IOException, JsonProcessingException { ReflectionTestUtils.setField(bioDedupeProcessor, "infantDedupe", "N"); + + InternalRegistrationStatusDto testStatusDto = new InternalRegistrationStatusDto(); + testStatusDto.setRegistrationId("TEST_RID"); + testStatusDto.setRegistrationType(RegistrationType.NEW.name()); + MessageDTO messageDTO = new MessageDTO(); + messageDTO.setRid("TEST_RID"); messageDTO.setReg_type(RegistrationType.NEW.name()); + + when(registrationStatusService.getRegistrationStatus(anyString(), anyString(), anyInt(), anyString())) + .thenReturn(testStatusDto); Mockito.when(bioDedupeService.getFileByRegId(anyString(),anyString())).thenReturn(null); Mockito.when(utility.getApplicantAge(any(),any(),any())).thenReturn(3); + MessageDTO messageDto = bioDedupeProcessor.process(messageDTO, stageName); + assertTrue(messageDto.getIsValid()); assertEquals(MessageBusAddress.BIO_DEDUPE_BUS_IN, messageDTO.getMessageBusAddress()); - assertEquals(RegistrationStatusCode.PROCESSING.name(), registrationStatusDto.getStatusCode()); - assertEquals(RegistrationTransactionStatusCode.SUCCESS.name(), registrationStatusDto.getLatestTransactionStatusCode()); + assertEquals(RegistrationStatusCode.PROCESSING.name(), testStatusDto.getStatusCode()); + assertEquals(RegistrationTransactionStatusCode.SUCCESS.name(), testStatusDto.getLatestTransactionStatusCode()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
registration-processor/core-processor/registration-processor-bio-dedupe-stage/src/test/java/io/mosip/registration/processor/biodedupe/stage/BioDedupeProcessorTest.java(2 hunks)registration-processor/registration-processor-packet-manager/src/test/java/io/mosip/registration/processor/packet/manager/service/impl/test/IdRepoServiceImplTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- registration-processor/registration-processor-packet-manager/src/test/java/io/mosip/registration/processor/packet/manager/service/impl/test/IdRepoServiceImplTest.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-18T09:19:25.334Z
Learnt from: ashok-ksharma
Repo: mosip/registration PR: 2209
File: registration-processor/core-processor/registration-processor-uin-generator-stage/src/main/java/io/mosip/registration/processor/stages/uingenerator/stage/UinGeneratorStage.java:1168-1177
Timestamp: 2025-11-18T09:19:25.334Z
Learning: In the MOSIP registration processor, the `updatePacketCreatedOnInDemographicIdentity` method in `UinGeneratorStage.java` is intentionally limited to `RegistrationType.NEW` and `RegistrationType.UPDATE` only, because these are the registration types that update biometrics. `RES_UPDATE` and Mapped `UPDATE` do not update biometrics, so they should not trigger the `packetCreatedOn` update logic.
Applied to files:
registration-processor/core-processor/registration-processor-bio-dedupe-stage/src/test/java/io/mosip/registration/processor/biodedupe/stage/BioDedupeProcessorTest.java
🧬 Code graph analysis (1)
registration-processor/core-processor/registration-processor-bio-dedupe-stage/src/test/java/io/mosip/registration/processor/biodedupe/stage/BioDedupeProcessorTest.java (1)
registration-processor/registration-processor-core/src/main/java/io/mosip/registration/processor/core/abstractverticle/MessageBusAddress.java (1)
MessageBusAddress(18-266)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-maven-registration / maven-build
...ge/src/test/java/io/mosip/registration/processor/biodedupe/stage/BioDedupeProcessorTest.java
Outdated
Show resolved
Hide resolved
...java/io/mosip/registration/processor/verification/service/ManualAdjudicationServiceTest.java
Show resolved
Hide resolved
...java/io/mosip/registration/processor/verification/service/ManualAdjudicationServiceTest.java
Show resolved
Hide resolved
.../src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java
Show resolved
Hide resolved
.../src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java
Outdated
Show resolved
Hide resolved
.../test/java/io/mosip/registration/processor/verification/service/VerificationServiceTest.java
Show resolved
Hide resolved
...p/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java
Outdated
Show resolved
Hide resolved
...vice/src/test/java/io/mosip/registration/processor/notification/NotificationServiceTest.java
Show resolved
Hide resolved
.../io/mosip/registration/processor/packet/manager/service/impl/test/IdRepoServiceImplTest.java
Show resolved
Hide resolved
.../io/mosip/registration/processor/packet/manager/service/impl/test/IdRepoServiceImplTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Nidhi0201 <nidhi.k@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
registration-processor/core-processor/registration-processor-manual-adjudication-stage/src/test/java/io/mosip/registration/processor/verification/service/ManualAdjudicationServiceTest.java (1)
820-859: Test name could be more specific about the scenario being tested.The test name
testProcessShouldInvalidateMessageWhenDataShareUrlPresentsuggests that the mere presence of a data share URL causes invalidation. However, the test setup shows that you're exercising a complex scenario with:
- ShareableAttributes configured for biometrics
- Filter configurations
- Mocked packet manager responses
- Data share API mocking
The actual scenario being tested appears to be: when biometrics are configured as shareable attributes with filters, the process should invalidate the message without marking it as an internal error (perhaps because required fields are missing or validation fails).
Consider renaming to better reflect the actual test scenario, such as:
testProcessShouldInvalidateMessageWhenBiometricsShareableAttributesConfiguredtestProcessShouldHandleMissingBiometricsGracefullyThis will make the test's intent clearer for future maintainers.
registration-processor/registration-processor-message-sender-impl/src/test/java/io/mosip/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java (1)
554-587: Test name suggests JSON exception handling but tests null-data behavior.The test name
testSendSmsNotificationShouldHandleJsonParseExceptionGracefullyimplies that it verifies how the system handlesJsonProcessingException. However, the test actually:
- Sets
packetManagerService.getFieldsto returnnull- Mocks an empty
SmsResponseDtoresponse- Asserts that
result.getStatus()isnullThis exercises default/null-data behavior rather than exception handling. The try-with-resources for
FileInputStreamis good practice for resource cleanup.Consider renaming to reflect actual behavior:
-testSendSmsNotificationShouldHandleJsonParseExceptionGracefully +testSendSmsNotificationWithNullPacketFieldsReturnsNullStatusBased on learnings: Previous review discussions indicated the developer assessed this test pattern as not requiring changes. If this test adequately covers the intended scenario for your coverage goals, the current implementation is acceptable—just be aware the name may be misleading for future maintainers.
registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java (2)
2839-2876: Make the “idrepo draft returns null” condition explicit + verify the call. Right now the test seems to rely on Mockito’s defaultnullreturn rather than explicitly stubbingidrepoDraftService.idrepoUpdateDraft(...)to returnnull, which makes the test intent less clear and easier to break during refactors.+ when(idrepoDraftService.idrepoUpdateDraft(anyString(), any(), any())).thenReturn(null); MessageDTO result = uinGeneratorStage.process(messageDTO); + verify(idrepoDraftService).idrepoUpdateDraft(anyString(), any(), any()); assertFalse(result.getInternalError()); assertFalse(result.getIsValid());
2932-2979: Consider tightening stubs so the test clearly exercises the “IDRepo DRAFTED” branch.responsedtois returned fromgetApi(...)but appears empty; ifprocess()reads fields from that response, this test may be passing for the wrong reason. Suggest populatingresponsedto.setResponse(...)(or explicitly documenting/verify the minimal contract) and verifying the relevant call(s) (idrepoUpdateDraft/patchApi) to ensure the DRAFTED path is what drivesisValid=true.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
registration-processor/core-processor/registration-processor-bio-dedupe-stage/src/test/java/io/mosip/registration/processor/biodedupe/stage/BioDedupeProcessorTest.java(2 hunks)registration-processor/core-processor/registration-processor-manual-adjudication-stage/src/test/java/io/mosip/registration/processor/verification/service/ManualAdjudicationServiceTest.java(4 hunks)registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java(2 hunks)registration-processor/core-processor/registration-processor-verification-stage/src/test/java/io/mosip/registration/processor/verification/service/VerificationServiceTest.java(3 hunks)registration-processor/registration-processor-message-sender-impl/src/test/java/io/mosip/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java(5 hunks)registration-processor/registration-processor-packet-manager/src/test/java/io/mosip/registration/processor/packet/manager/service/impl/test/IdRepoServiceImplTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- registration-processor/registration-processor-packet-manager/src/test/java/io/mosip/registration/processor/packet/manager/service/impl/test/IdRepoServiceImplTest.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-18T09:19:25.334Z
Learnt from: ashok-ksharma
Repo: mosip/registration PR: 2209
File: registration-processor/core-processor/registration-processor-uin-generator-stage/src/main/java/io/mosip/registration/processor/stages/uingenerator/stage/UinGeneratorStage.java:1168-1177
Timestamp: 2025-11-18T09:19:25.334Z
Learning: In the MOSIP registration processor, the `updatePacketCreatedOnInDemographicIdentity` method in `UinGeneratorStage.java` is intentionally limited to `RegistrationType.NEW` and `RegistrationType.UPDATE` only, because these are the registration types that update biometrics. `RES_UPDATE` and Mapped `UPDATE` do not update biometrics, so they should not trigger the `packetCreatedOn` update logic.
Applied to files:
registration-processor/core-processor/registration-processor-bio-dedupe-stage/src/test/java/io/mosip/registration/processor/biodedupe/stage/BioDedupeProcessorTest.javaregistration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java
🧬 Code graph analysis (2)
registration-processor/registration-processor-message-sender-impl/src/test/java/io/mosip/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java (1)
registration-processor/registration-processor-registration-status-service-impl/src/main/java/io/mosip/registration/processor/status/dto/SyncRegistrationDto.java (1)
SyncRegistrationDto(21-361)
registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java (1)
registration-processor/registration-processor-core/src/main/java/io/mosip/registration/processor/core/constant/MappingJsonConstants.java (1)
MappingJsonConstants(3-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-maven-registration / maven-build
🔇 Additional comments (10)
registration-processor/core-processor/registration-processor-manual-adjudication-stage/src/test/java/io/mosip/registration/processor/verification/service/ManualAdjudicationServiceTest.java (2)
861-890: LGTM! ArgumentCaptor usage properly validates MessageDTO state.The test correctly uses
ArgumentCaptorto capture and verify theMessageDTOsent to the next stage. The assertions confirm both the boolean return value and the state of the captured message (isValid=false, internalError=false for rejected status). This addresses the previous review feedback effectively.
892-907: LGTM! Test correctly validates error handling for invalid RID.The test properly exercises the error path when an empty RID is provided. The assertions confirm that the system correctly marks this as an internal error (internalError=true) and invalid message (isValid=false), which is the expected behavior for malformed input.
registration-processor/registration-processor-message-sender-impl/src/test/java/io/mosip/registration/processor/message/sender/test/service/MessageNotificationServiceImplTest.java (3)
506-514: LGTM! Test correctly verifies exception propagation.The test properly validates that
ApisResourceAccessExceptionfrom the template generator propagates to the caller. The use of@Test(expected = ApisResourceAccessException.class)is appropriate for this scenario.
516-533: LGTM! Test validates successful SMS notification with preferred languages.The test correctly exercises the happy path when preferred languages are configured. The assertion on
result.getStatus()confirms successful notification delivery. This addresses previous review feedback about capturing and asserting on the return value.
535-552: LGTM! Test validates graceful error handling.The test correctly verifies that when fetching preferred languages fails with
ApisResourceAccessException, the system handles it gracefully and still sends the SMS notification successfully (status="OK"). The test name accurately reflects this behavior, addressing previous review feedback about mismatched naming.registration-processor/core-processor/registration-processor-bio-dedupe-stage/src/test/java/io/mosip/registration/processor/biodedupe/stage/BioDedupeProcessorTest.java (1)
951-962: LGTM! Test validates exception handling with correct error flags.The test properly exercises the error path when
abisHandlerUtil.getPacketStatusthrows aRuntimeException. The assertions correctly verify that the system marks the message as invalid (isValid=false) and sets the internal error flag (internalError=true), which is appropriate for unexpected exceptions.registration-processor/core-processor/registration-processor-verification-stage/src/test/java/io/mosip/registration/processor/verification/service/VerificationServiceTest.java (2)
541-559: LGTM! ArgumentCaptor properly validates MessageDTO for rejected status.The test correctly uses
ArgumentCaptorto capture and verify theMessageDTOsent tomanualAdjudicationStage. The assertions confirm:
- The update operation succeeded (result=true)
- The captured message exists
- The message state reflects rejection (isValid=false, internalError=false)
This addresses previous review feedback about using ArgumentCaptor to validate message attributes beyond just the boolean return value.
561-579: LGTM! Test validates approved status with correct assertions.The test properly exercises the approval flow and uses
ArgumentCaptorto verify theMessageDTOstate. The assertions correctly confirm that when manual verification is approved (returnValue=1), the resulting message is valid (isValid=true) with no internal error (internalError=false).registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java (2)
245-246: Good: defineIDREPO_STATUSconstant to avoid undefined symbol.
2779-2807: Good coverage for “reprocess” error-flagging path; assertions are on the returnedMessageDTO(not the mocked DTO).
...ge/src/test/java/io/mosip/registration/processor/biodedupe/stage/BioDedupeProcessorTest.java
Show resolved
Hide resolved
.../src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java
Show resolved
Hide resolved
.../src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java
Show resolved
Hide resolved
|
@ashok-ksharma Addressed the review comments based on discussion |
Signed-off-by: Chetan Kumar Hirematha <chetankumar.h.239@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java (1)
2878-2930: Test name/setup mismatch: “StatusActivated” but mock simulates DEACTIVATED-from-IDRepo.
registrationProcessorRestClientService.getApi(...)returnsidResponseDTO1with"DEACTIVATED"(Line 2916-2923), which reads like “reactivate a DEACTIVATED UIN”. Either rename the test to match what it’s actually doing, or change the mock to return"ACTIVATED"and update expectations accordingly.- public void testReactivateUinWhenStatusActivated() throws Exception { + public void testReactivateUinWhenStatusDeactivated_ShouldSucceed() throws Exception {
🧹 Nitpick comments (4)
registration-processor/registration-processor-packet-manager/src/test/java/io/mosip/registration/processor/packet/manager/service/impl/test/IdRepoServiceImplTest.java (1)
135-169: Populate ResponseDTO to match the mocked JSON structure.The test creates an empty
ResponseDTO(line 141) but mocksmapper.writeValueAsStringto return a specific JSON structure (line 154). This disconnect means the test doesn't verify that the actual DTO serialization produces the expected format.Consider populating the
ResponseDTOfields to align with the mocked JSON output, or adjust the test to verify the actual serialization behavior rather than bypassing it with a mock.For example, if ResponseDTO has an identity field:
ResponseDTO responseDTO = new ResponseDTO(); +// Populate responseDTO.identity or equivalent field with the expected structure +// so that actual serialization matches the mocked output dto.setResponse(responseDTO);This would make the test more accurate and maintainable by ensuring the DTO structure matches what the mock expects.
registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java (3)
2779-2807: Consider asserting the persisted transaction status, not onlyMessageDTOflags.
Right now this test can pass even if the “REPROCESS” mapping isn’t propagated into the registration transaction (it only checksinternalError/isValid). IfuinGeneratorStage.process()is expected to callregistrationStatusService.addRegistrationTransaction(...)(or similar), capturing that DTO and assertinglatestTransactionStatusCode == REPROCESSwould make the test much stronger.
2809-2837: Same strengthening suggestion as reprocess test: assert the recorded FAILED status if applicable.
If the code path writes/updates registration transaction/status, prefer verifying the argument to that service call, so the test validates the “FAILED” mapping is actually used—not just the final flags.
2839-2876: Make the “draft returns null” intent explicit (avoid relying on default Mockito null).
This test currently depends onidrepoDraftService.idrepoUpdateDraft(...)being unstubbed (thus returningnull). Stubbing it explicitly improves readability and guards against future default-answer changes.@@ - MessageDTO result = uinGeneratorStage.process(messageDTO); + when(idrepoDraftService.idrepoUpdateDraft(anyString(), any(), any())).thenReturn(null); + MessageDTO result = uinGeneratorStage.process(messageDTO); assertFalse(result.getInternalError()); assertFalse(result.getIsValid());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java(2 hunks)registration-processor/registration-processor-packet-manager/src/test/java/io/mosip/registration/processor/packet/manager/service/impl/test/IdRepoServiceImplTest.java(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-18T09:19:25.334Z
Learnt from: ashok-ksharma
Repo: mosip/registration PR: 2209
File: registration-processor/core-processor/registration-processor-uin-generator-stage/src/main/java/io/mosip/registration/processor/stages/uingenerator/stage/UinGeneratorStage.java:1168-1177
Timestamp: 2025-11-18T09:19:25.334Z
Learning: In the MOSIP registration processor, the `updatePacketCreatedOnInDemographicIdentity` method in `UinGeneratorStage.java` is intentionally limited to `RegistrationType.NEW` and `RegistrationType.UPDATE` only, because these are the registration types that update biometrics. `RES_UPDATE` and Mapped `UPDATE` do not update biometrics, so they should not trigger the `packetCreatedOn` update logic.
Applied to files:
registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java
🧬 Code graph analysis (1)
registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java (1)
registration-processor/registration-processor-core/src/main/java/io/mosip/registration/processor/core/constant/MappingJsonConstants.java (1)
MappingJsonConstants(3-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-maven-registration / maven-build
🔇 Additional comments (5)
registration-processor/registration-processor-packet-manager/src/test/java/io/mosip/registration/processor/packet/manager/service/impl/test/IdRepoServiceImplTest.java (3)
3-3: LGTM! Necessary imports added.The import additions are properly aligned with the new test methods. All three imports (
assertEquals,ApiName,ResponseDTO) are actively used in the new test cases.Also applies to: 11-12
171-181: LGTM! Proper null response handling test.This test correctly verifies that
getIdJsonFromIDReporeturns null when the response is null. Good coverage for this edge case.
183-192: LGTM! Proper null response handling test.This test correctly verifies that
getIdResponseFromIDReporeturns null when the response is null. Good addition for comprehensive coverage.registration-processor/core-processor/registration-processor-uin-generator-stage/src/test/java/io/mosip/registration/processor/stages/uigenerator/UinGeneratorStageTest.java (2)
245-246: Good compile-safety improvement: centralize IDRepo status string intoIDREPO_STATUS.
This removes reliance on an undefined constant / magic string duplication later.
2932-2979: Please double-check the business expectation: UPDATE + IDRepo status DRAFTED ⇒isValid == true.
If “DRAFTED” is meant to be a terminal/OK outcome for UPDATE, then this is fine; if “DRAFTED” should trigger retry/reprocess/failure, the assertion should reflect that.
5 participants
MOSIP-43903 - Increased Junit code coverage for Registration Processor (develop branch)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.