MOSIP-44120 Improved code coverage #281
Conversation
Signed-off-by: kameshsr <kameshsr1338@gmail.com>
WalkthroughA new unit test class Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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
Fix all issues with AI Agents 🤖
In
@commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.java:
- Around line 78-84: The test is trying to stub private methods validateSchema
and fileAndChecksumValidation on spyValidator which causes compilation failures;
fix by either (a) changing those methods' visibility from private to
package-private or protected so Mockito can stub them, or (b) avoid stubbing
private methods and instead stub the public method that calls them or refactor
internal logic into a new injectable collaborator to mock; after making the
methods accessible or refactoring, update the test to use
doReturn(true).when(spyValidator).validateSchema(...) and
doReturn(true).when(spyValidator).fileAndChecksumValidation(...), and also
ensure the asserted audit log message exactly matches the implementation’s
"successful" string (adjust expected text to match the real log output).
- Around line 50-55: The test currently creates a Mockito spy (spyValidator) on
packetValidator and attempts to stub private methods validateSchema and
fileAndChecksumValidation which causes compilation failures; remove the spy and
instead test the public validate method by mocking the dependencies used inside
PacketValidator (e.g., idObjectValidator, idSchemaUtils, checksum/file
validators) to produce controlled outcomes, or change the access of
validateSchema/fileAndChecksumValidation to package-private/protected if you
must unit-test them directly; alternatively, if you must stub private methods
(not recommended), use PowerMock—preferably refactor tests to mock dependencies
and assert behavior of validate on PacketValidatorTest.
- Around line 95-113: The audit verification in the test
validateReturnsFalseWhenFileValidationFails is wrong: when
fileAndChecksumValidation on spyValidator is stubbed to return false and
validate("123", "source", "process") asserts false, update the
verify(auditLogEntry).addAudit(...) assertion to expect a failure message (e.g.,
use contains("failed") or contains("unsuccessful") instead of
contains("successful")) so the auditLogEntry.addAudit verification matches the
failed validation path.
🧹 Nitpick comments (1)
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.java (1)
57-113: Consider more precise test assertions.All three tests use
anyString()matchers when verifying method calls. While functional, using more specific matchers (likeeq("123"),eq("source"),eq("process")) would make the tests more precise and catch bugs where wrong parameters are passed.Additionally, consider adding edge case tests:
- Null parameters
- Empty strings
- Exception handling scenarios
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.java
🧰 Additional context used
🪛 GitHub Actions: Maven Package upon a push
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.java
[error] 62-62: validateSchema(java.lang.String,java.lang.String,java.lang.String) has private access in io.mosip.commons.packet.util.PacketValidator
[error] 80-80: validateSchema(java.lang.String,java.lang.String,java.lang.String) has private access in io.mosip.commons.packet.util.PacketValidator
[error] 100-100: validateSchema(java.lang.String,java.lang.String,java.lang.String) has private access in io.mosip.commons.packet.util.PacketValidator
🔇 Additional comments (1)
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.java (1)
69-72: No issues found. The test code is correct:
- Audit log verification: The test correctly uses
contains("failed")to match the actual audit message "Id object validation failed" that is logged when schema validation fails.- Method visibility: The
fileAndChecksumValidationmethod ispublic, not private, so theverify()call at line 72 will compile and execute without issues. Additionally, Mockito can handle stubbing of private methods (likevalidateSchema) on spy objects via reflection.The test will compile and run successfully.
| private PacketValidator spyValidator; | ||
|
|
||
| @Before | ||
| public void setup() { | ||
| spyValidator = Mockito.spy(packetValidator); | ||
| } |
There was a problem hiding this comment.
Critical: Cannot spy on private methods - compilation failures.
The pipeline failures indicate that validateSchema has private access in PacketValidator. Mockito's doReturn().when() cannot stub private methods on a spy. This approach will not compile.
Consider these alternatives:
- Test the public
validatemethod's behavior without spying on internal methods by setting up the proper mock responses on dependencies - If you need to test
validateSchemaandfileAndChecksumValidationseparately, make them package-private or protected - Use PowerMock if private method testing is absolutely necessary (though generally discouraged)
🔎 Recommended approach: Test public behavior without spying
Remove the spy and test the public API by properly mocking the dependencies that validateSchema and fileAndChecksumValidation use internally:
- private PacketValidator spyValidator;
-
- @Before
- public void setup() {
- spyValidator = Mockito.spy(packetValidator);
- }Then in your tests, set up mocks on idObjectValidator, idSchemaUtils, etc., to control the validation outcomes.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.java
around lines 50-55, The test currently creates a Mockito spy (spyValidator) on
packetValidator and attempts to stub private methods validateSchema and
fileAndChecksumValidation which causes compilation failures; remove the spy and
instead test the public validate method by mocking the dependencies used inside
PacketValidator (e.g., idObjectValidator, idSchemaUtils, checksum/file
validators) to produce controlled outcomes, or change the access of
validateSchema/fileAndChecksumValidation to package-private/protected if you
must unit-test them directly; alternatively, if you must stub private methods
(not recommended), use PowerMock—preferably refactor tests to mock dependencies
and assert behavior of validate on PacketValidatorTest.
| doReturn(true) | ||
| .when(spyValidator) | ||
| .validateSchema(anyString(), anyString(), anyString()); | ||
|
|
||
| doReturn(true) | ||
| .when(spyValidator) | ||
| .fileAndChecksumValidation(anyString(), anyString(), anyString()); |
There was a problem hiding this comment.
Private method access issues on both stubbed methods.
Both validateSchema (line 80) and fileAndChecksumValidation (line 84) have private access according to the pipeline failures. These stubbing attempts will fail at compilation.
Additionally, verify that the audit log message format matches the expected "successful" string in the actual implementation.
🧰 Tools
🪛 GitHub Actions: Maven Package upon a push
[error] 80-80: validateSchema(java.lang.String,java.lang.String,java.lang.String) has private access in io.mosip.commons.packet.util.PacketValidator
🤖 Prompt for AI Agents
In
@commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.java
around lines 78-84, The test is trying to stub private methods validateSchema
and fileAndChecksumValidation on spyValidator which causes compilation failures;
fix by either (a) changing those methods' visibility from private to
package-private or protected so Mockito can stub them, or (b) avoid stubbing
private methods and instead stub the public method that calls them or refactor
internal logic into a new injectable collaborator to mock; after making the
methods accessible or refactoring, update the test to use
doReturn(true).when(spyValidator).validateSchema(...) and
doReturn(true).when(spyValidator).fileAndChecksumValidation(...), and also
ensure the asserted audit log message exactly matches the implementation’s
"successful" string (adjust expected text to match the real log output).
| @Test | ||
| public void validateReturnsFalseWhenFileValidationFails() throws Exception { | ||
| // Arrange | ||
| doReturn(true) | ||
| .when(spyValidator) | ||
| .validateSchema(anyString(), anyString(), anyString()); | ||
|
|
||
| doReturn(false) | ||
| .when(spyValidator) | ||
| .fileAndChecksumValidation(anyString(), anyString(), anyString()); | ||
|
|
||
| // Act | ||
| boolean result = spyValidator.validate("123", "source", "process"); | ||
|
|
||
| // Assert | ||
| assertFalse(result); | ||
| verify(auditLogEntry) | ||
| .addAudit(contains("successful"), any(), any(), any(), any(), any(), eq("123")); | ||
| } |
There was a problem hiding this comment.
Inconsistent audit log verification when file validation fails.
The test stubs fileAndChecksumValidation to return false (line 102) and correctly asserts the result is false (line 110). However, the audit log verification at line 112 checks for contains("successful"), which is inconsistent with a failed validation.
If file validation fails, the audit should log a failure. This appears to be a copy-paste error from the previous test method.
🔎 Proposed fix
// Assert
assertFalse(result);
verify(auditLogEntry)
- .addAudit(contains("successful"), any(), any(), any(), any(), any(), eq("123"));
+ .addAudit(contains("failed"), any(), any(), any(), any(), any(), eq("123"));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| public void validateReturnsFalseWhenFileValidationFails() throws Exception { | |
| // Arrange | |
| doReturn(true) | |
| .when(spyValidator) | |
| .validateSchema(anyString(), anyString(), anyString()); | |
| doReturn(false) | |
| .when(spyValidator) | |
| .fileAndChecksumValidation(anyString(), anyString(), anyString()); | |
| // Act | |
| boolean result = spyValidator.validate("123", "source", "process"); | |
| // Assert | |
| assertFalse(result); | |
| verify(auditLogEntry) | |
| .addAudit(contains("successful"), any(), any(), any(), any(), any(), eq("123")); | |
| } | |
| @Test | |
| public void validateReturnsFalseWhenFileValidationFails() throws Exception { | |
| // Arrange | |
| doReturn(true) | |
| .when(spyValidator) | |
| .validateSchema(anyString(), anyString(), anyString()); | |
| doReturn(false) | |
| .when(spyValidator) | |
| .fileAndChecksumValidation(anyString(), anyString(), anyString()); | |
| // Act | |
| boolean result = spyValidator.validate("123", "source", "process"); | |
| // Assert | |
| assertFalse(result); | |
| verify(auditLogEntry) | |
| .addAudit(contains("failed"), any(), any(), any(), any(), any(), eq("123")); | |
| } |
🧰 Tools
🪛 GitHub Actions: Maven Package upon a push
[error] 100-100: validateSchema(java.lang.String,java.lang.String,java.lang.String) has private access in io.mosip.commons.packet.util.PacketValidator
🤖 Prompt for AI Agents
In
@commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.java
around lines 95-113, The audit verification in the test
validateReturnsFalseWhenFileValidationFails is wrong: when
fileAndChecksumValidation on spyValidator is stubbed to return false and
validate("123", "source", "process") asserts false, update the
verify(auditLogEntry).addAudit(...) assertion to expect a failure message (e.g.,
use contains("failed") or contains("unsuccessful") instead of
contains("successful")) so the auditLogEntry.addAudit verification matches the
failed validation path.
Signed-off-by: kameshsr <kameshsr1338@gmail.com>
kameshsr
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.java (2)
98-101: Critical: Cannot spy on private methodfileAndChecksumValidation- compilation will fail.Past reviews confirm that
fileAndChecksumValidationhas private access inPacketValidator. Mockito's spy cannot stub private methods, causing compilation failures.Consider these alternatives:
- Change visibility (simplest): Make
fileAndChecksumValidationpackage-private or protected if you need to test it separately- Mock dependencies (preferred): Remove the spy and properly mock all dependencies that
fileAndChecksumValidationuses internally (e.g.,packetKeeper.getPacket(), file validators, etc.)- PowerMock (not recommended): Only if absolutely necessary for private method testing
Based on past reviews, this same issue was flagged but not resolved.
🔎 Recommended approach: Mock dependencies instead of spying
Instead of spying on
fileAndChecksumValidation, mock the dependencies it uses:- // ---- spy only public method ---- - PacketValidator spyValidator = spy(packetValidator); - doReturn(true) - .when(spyValidator) - .fileAndChecksumValidation(any(), any(), any()); + // ---- Mock dependencies used by fileAndChecksumValidation ---- + when(packetKeeper.getPacket(any())).thenReturn(/* appropriate mock packet */); + // Add other necessary mocks for file/checksum validationThen test the public
validatemethod directly:// ---- act ---- - boolean result = spyValidator.validate("123", "source", "process"); + boolean result = packetValidator.validate("123", "source", "process");
136-139: Critical: Cannot spy on private methodfileAndChecksumValidation- compilation will fail.Same issue as in the previous test: attempting to spy and stub the private method
fileAndChecksumValidationwill cause compilation failures.Apply the same fix as recommended for lines 98-101: either change the method's visibility or mock the dependencies that
fileAndChecksumValidationuses internally.🔎 Alternative: Mock dependencies
- // ---- Force file validation to fail ---- - PacketValidator spyValidator = spy(packetValidator); - doReturn(false) - .when(spyValidator) - .fileAndChecksumValidation(any(), any(), any()); + // ---- Mock dependencies to make file validation fail ---- + when(packetKeeper.getPacket(any())).thenThrow(new RuntimeException("File not found")); + // Or mock other file validation dependencies to return failure statesThen test with the original injected instance:
// ---- Act ---- - boolean result = spyValidator.validate("123", "source", "process"); + boolean result = packetValidator.validate("123", "source", "process");
🧹 Nitpick comments (1)
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.java (1)
54-59: Consider removing redundant mock setup.Line 58 sets
env.getProperty(anyString())to returnnull, but this is overridden in every test method. Since the default behavior doesn't provide value and might cause confusion, consider removing it or adding a comment explaining why this default is useful.💡 Optional: Remove or clarify default setup
@Before public void setup() { - // env.getProperty(...) returns null - // → validateSchema() returns false - when(env.getProperty(anyString())).thenReturn(null); + // Default setup if needed }Or if keeping it:
@Before public void setup() { - // env.getProperty(...) returns null - // → validateSchema() returns false + // Default: env.getProperty returns null to simulate missing configuration. + // Individual tests override this as needed. when(env.getProperty(anyString())).thenReturn(null); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.java
🧰 Additional context used
🧬 Code graph analysis (1)
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.java (1)
commons-packet/commons-packet-manager/src/main/java/io/mosip/commons/packet/constants/PacketManagerConstants.java (1)
PacketManagerConstants(3-119)
⏰ 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: commons-packet-service / build-dockers
🔇 Additional comments (1)
commons-packet/commons-packet-manager/src/test/java/io/mosip/commons/packet/test/util/PacketValidatorTest.java (1)
76-77: Audit message verification confirms test assertion is correct.The test correctly verifies that when schema validation fails, the audit log message "Id object validation failed" contains "failed". The implementation at line 96 of PacketValidator.java logs this exact message, matching the
contains("failed")assertion. All other parameters in the assertion (eventId, eventName, eventType, null, null) align with the implementation. The test logic is sound and requires no changes.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.