test(service) : Added some unit tests for MMU-API #108
Conversation
WalkthroughAdds a comprehensive unit test suite for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@vanitha1822 the merge conflicts occur because :
Can you please review these changes . these are some few refactor I have done as that can be seen in the PR . |
|
|
@Zapper9982 can you please fix these conflicts? |
|
cc: @vanitha1822 |
|
@vanitha1822 |
@Zapper9982 yes, we updated some changes in these 2 files. So please take the recent pull and accept the incoming change please. |
|
@Zapper9982 can you resolve the conflicts? |
|
![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
🧹 Nitpick comments (5)
src/test/java/com/iemr/mmu/service/dataSyncLayerCentral/GetDataFromVanAndSyncToDBImplTest.java (5)
47-68: Strengthen test assertions with verification of mock interactions.Most tests only assert that the result equals
"data sync passed", but they don't verify that the mocked repository methods were called with the expected parameters. This reduces the tests' ability to catch regressions.Consider adding
verify()calls to ensure:
dataSyncRepositoryCentral.checkRecordIsAlreadyPresentOrNot()is called with correct parametersdataSyncRepositoryCentral.syncDataToCentralDB()is called with expected query and data- The correct number of interactions occurred
For exception tests (lines 284-296), verify the specific exception type and message rather than just checking the exception is not null.
Example enhancement for a test:
String result = spyService.syncDataToServer(json, "auth"); assertEquals("data sync passed", result); + verify(dataSyncRepositoryCentral).checkRecordIsAlreadyPresentOrNot( + eq("schema"), eq("t_indent"), eq("FromFacilityID"), + anyString(), eq("v1"), eq(1)); + verify(dataSyncRepositoryCentral).syncDataToCentralDB( + anyString(), eq("schema"), any(), anyString(), anyList());For exception tests:
- Exception exception = assertThrows(Exception.class, () -> spyService.syncDataToServer(null, "auth")); - assertNotNull(exception); + Exception exception = assertThrows(NullPointerException.class, + () -> spyService.syncDataToServer(null, "auth")); + assertTrue(exception.getMessage().contains("expected error context"));Also applies to: 70-76, 78-90, 92-115, 117-141, 186-209, 212-236, 238-254, 256-270, 272-281, 284-288, 290-296, 299-322, 325-348, 351-375, 378-407, 410-441, 444-466, 469-496, 499-521, 524-545, 547-569
143-158: Use consistent service instance pattern.These tests create new
GetDataFromVanAndSyncToDBImpl()instances instead of using the@InjectMocksservice field used in other tests. This inconsistency makes the test class harder to maintain.Since these tests are testing methods that don't require mocked dependencies, the new instance creation is acceptable, but consider using the injected instance for consistency or clearly documenting why certain tests need fresh instances.
Also applies to: 160-183, 571-590
47-545: Consider extracting magic strings to constants.The tests use many repeated string literals like
"data sync passed","schema","table","user","auth", etc. Extracting these to private static final constants would improve maintainability and make it clearer which values are significant.Example:
private static final String EXPECTED_SYNC_SUCCESS = "data sync passed"; private static final String TEST_SCHEMA = "schema"; private static final String TEST_TABLE = "table"; private static final String TEST_USER = "user"; private static final String TEST_AUTH = "auth";
592-594: Remove incomplete comment or implement the suggested utility.The comment at the end of the file mentions a "JSON parsing stub" that "may need to add... for full testability," but no implementation is provided. Either:
- Remove the comment if the utility is not needed
- Implement the utility if it's required for test functionality
- Clarify what JSON parsing functionality is missing
The tests currently pass JSON strings to
syncDataToServer()without showing how the actual JSON parsing is handled or mocked.
92-545: Consider parameterized tests to reduce duplication.Many tests follow an identical structure but with different input data and expected behaviors. JUnit 5's
@ParameterizedTestcould reduce this duplication and make the test suite more maintainable.For example, tests for different table names (lines 299-322) could be consolidated into a single parameterized test.
Example:
@ParameterizedTest @ValueSource(strings = {"t_indent", "t_indentorder", "t_indentissue", "t_stocktransfer", "t_itemstockentry"}) void testSyncDataToServer_specialTableNames(String tableName) throws Exception { // Common setup with tableName parameter // Assertions }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/com/iemr/mmu/service/dataSyncLayerCentral/GetDataFromVanAndSyncToDBImplTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/test/java/com/iemr/mmu/service/dataSyncLayerCentral/GetDataFromVanAndSyncToDBImplTest.java (1)
src/main/java/com/iemr/mmu/data/syncActivity_syncLayer/SyncUploadDataDigester.java (1)
SyncUploadDataDigester(27-121)
🔇 Additional comments (3)
src/test/java/com/iemr/mmu/service/dataSyncLayerCentral/GetDataFromVanAndSyncToDBImplTest.java (3)
36-45: LGTM!The test class setup correctly uses Mockito annotations and properly initializes mocks in the
@BeforeEachmethod.
547-569: Verify that null is the expected behavior for length mismatch.This test expects a
nullreturn value when the number of sync records doesn't match the result array length. Please verify:
- Is returning
nullthe intended behavior for this error condition?- Should the method throw an exception or log an error instead?
- How should calling code handle a
nullresult?Silently returning
nullon errors can lead to null pointer exceptions in calling code. Consider whether an exception or explicit error result would be more appropriate.
47-590: Comprehensive test coverage.The test suite provides excellent coverage of edge cases including:
- Empty and null sync data
- Insert and update flows
- Length mismatches between expected and actual results
- Special table name handling
- Boolean value handling
- Null facilityID scenarios
- Invalid JSON inputs
This thorough testing approach will help catch regressions and ensure the synchronization logic works correctly across various scenarios.
| @Test | ||
| void testSyncDataToServer_otherTable_insertAndUpdate() throws Exception { | ||
| SyncUploadDataDigester digester = mock(SyncUploadDataDigester.class); | ||
| when(digester.getTableName()).thenReturn("t_indent"); | ||
| when(digester.getSyncData()).thenReturn(Arrays.asList( | ||
| new HashMap<String, Object>() {{ | ||
| put("FromFacilityID", 1.0); | ||
| put("SyncFacilityID", 1); | ||
| put("VanID", "v1"); | ||
| put("date_format(SyncedDate,'%Y-%m-%d %H:%i:%s')", ""); | ||
| }} | ||
| )); | ||
| when(digester.getVanAutoIncColumnName()).thenReturn("FromFacilityID"); | ||
| when(digester.getFacilityID()).thenReturn(1); | ||
| when(digester.getSchemaName()).thenReturn("schema"); | ||
| when(digester.getServerColumns()).thenReturn("col1,col2"); | ||
| when(dataSyncRepositoryCentral.checkRecordIsAlreadyPresentOrNot(anyString(), anyString(), anyString(), anyString(), anyString(), anyInt())).thenReturn(0); | ||
| when(dataSyncRepositoryCentral.syncDataToCentralDB(anyString(), anyString(), any(), anyString(), anyList())).thenReturn(new int[]{1}); | ||
| GetDataFromVanAndSyncToDBImpl spyService = spy(service); | ||
| doReturn("t_indent").when(digester).getTableName(); | ||
| String json = "{\"tableName\":\"t_indent\"}"; | ||
| String result = spyService.syncDataToServer(json, "auth"); | ||
| assertEquals("data sync passed", result); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove redundant mock configuration.
Throughout these tests, the digester mock is configured with when().thenReturn() statements, and then the same values are set again using doReturn().when() (e.g., lines 111, 137, 205, 232, 317, 344, 371, 403, 437, 462, 492, 517, 541). This is redundant because the mock is already properly configured with the when() statements.
The doReturn() calls should only be used on the spyService, not on the already-mocked digester.
Example fix for lines 110-111:
when(dataSyncRepositoryCentral.syncDataToCentralDB(anyString(), anyString(), any(), anyString(), anyList())).thenReturn(new int[]{1});
GetDataFromVanAndSyncToDBImpl spyService = spy(service);
- doReturn("t_indent").when(digester).getTableName();
String json = "{\"tableName\":\"t_indent\"}";Apply similar changes to all affected tests.
Also applies to: 117-141, 186-209, 212-236, 299-322, 325-348, 351-375, 378-407, 410-441, 444-466, 469-496, 499-521, 524-545
🤖 Prompt for AI Agents
In
src/test/java/com/iemr/mmu/service/dataSyncLayerCentral/GetDataFromVanAndSyncToDBImplTest.java
around lines 92 to 115, the test configures the mocked digester with
when(...).thenReturn(...) and then redundantly calls
doReturn(...).when(digester) later; remove the redundant doReturn() calls that
target the digester (keep the when().thenReturn() stubs) and only use doReturn()
on the spyService where needed; update all similar occurrences listed (117-141,
186-209, 212-236, 299-322, 325-348, 351-375, 378-407, 410-441, 444-466, 469-496,
499-521, 524-545) by deleting the doReturn(...).when(digester) lines and leaving
the original when(...).thenReturn(...) mocks intact so the tests rely on the
mocked digester and the spyService stubbing remains via doReturn().
| @Test | ||
| void testGetqueryFor_M_BeneficiaryRegIdMapping() { | ||
| GetDataFromVanAndSyncToDBImpl service = new GetDataFromVanAndSyncToDBImpl(); | ||
| // Using reflection to test the private method | ||
| try { | ||
| java.lang.reflect.Method method = GetDataFromVanAndSyncToDBImpl.class.getDeclaredMethod("getqueryFor_M_BeneficiaryRegIdMapping", String.class, String.class); | ||
| method.setAccessible(true); | ||
| String query = (String) method.invoke(service, "testSchema", "testTable"); | ||
| assertTrue(query.contains("UPDATE testSchema.testTable")); | ||
| String normalizedQuery = query.replaceAll("\\s+", " ").trim(); | ||
| if (!normalizedQuery.contains("SET Provisioned = true, SyncedDate = now(), syncedBy = ?")) { | ||
| System.out.println("Actual query: " + normalizedQuery); | ||
| } | ||
| assertTrue(normalizedQuery.contains("SET Provisioned = true, SyncedDate = now(), syncedBy = ?")); // actual query uses lowercase 'syncedBy' | ||
| assertTrue(normalizedQuery.contains("AND BeneficiaryID = ?")); | ||
| assertTrue(normalizedQuery.contains("AND VanID = ?")); | ||
| } catch (Exception e) { | ||
| fail("Failed to test getqueryFor_M_BeneficiaryRegIdMapping: " + e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid testing private methods via reflection.
Testing private methods directly using reflection is a code smell. Private methods should be tested indirectly through the public API. If this method requires direct testing, consider whether it should be package-private or if the class design needs refactoring.
Additionally, remove the debug System.out.println statement at line 582.
Apply this diff to remove the debug statement:
String query = (String) method.invoke(service, "testSchema", "testTable");
assertTrue(query.contains("UPDATE testSchema.testTable"));
String normalizedQuery = query.replaceAll("\\s+", " ").trim();
- if (!normalizedQuery.contains("SET Provisioned = true, SyncedDate = now(), syncedBy = ?")) {
- System.out.println("Actual query: " + normalizedQuery);
- }
assertTrue(normalizedQuery.contains("SET Provisioned = true, SyncedDate = now(), syncedBy = ?")); // actual query uses lowercase 'syncedBy'
assertTrue(normalizedQuery.contains("AND BeneficiaryID = ?"));
assertTrue(normalizedQuery.contains("AND VanID = ?"));📝 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 | |
| void testGetqueryFor_M_BeneficiaryRegIdMapping() { | |
| GetDataFromVanAndSyncToDBImpl service = new GetDataFromVanAndSyncToDBImpl(); | |
| // Using reflection to test the private method | |
| try { | |
| java.lang.reflect.Method method = GetDataFromVanAndSyncToDBImpl.class.getDeclaredMethod("getqueryFor_M_BeneficiaryRegIdMapping", String.class, String.class); | |
| method.setAccessible(true); | |
| String query = (String) method.invoke(service, "testSchema", "testTable"); | |
| assertTrue(query.contains("UPDATE testSchema.testTable")); | |
| String normalizedQuery = query.replaceAll("\\s+", " ").trim(); | |
| if (!normalizedQuery.contains("SET Provisioned = true, SyncedDate = now(), syncedBy = ?")) { | |
| System.out.println("Actual query: " + normalizedQuery); | |
| } | |
| assertTrue(normalizedQuery.contains("SET Provisioned = true, SyncedDate = now(), syncedBy = ?")); // actual query uses lowercase 'syncedBy' | |
| assertTrue(normalizedQuery.contains("AND BeneficiaryID = ?")); | |
| assertTrue(normalizedQuery.contains("AND VanID = ?")); | |
| } catch (Exception e) { | |
| fail("Failed to test getqueryFor_M_BeneficiaryRegIdMapping: " + e.getMessage()); | |
| } | |
| } | |
| @Test | |
| void testGetqueryFor_M_BeneficiaryRegIdMapping() { | |
| GetDataFromVanAndSyncToDBImpl service = new GetDataFromVanAndSyncToDBImpl(); | |
| // Using reflection to test the private method | |
| try { | |
| java.lang.reflect.Method method = GetDataFromVanAndSyncToDBImpl.class.getDeclaredMethod("getqueryFor_M_BeneficiaryRegIdMapping", String.class, String.class); | |
| method.setAccessible(true); | |
| String query = (String) method.invoke(service, "testSchema", "testTable"); | |
| assertTrue(query.contains("UPDATE testSchema.testTable")); | |
| String normalizedQuery = query.replaceAll("\\s+", " ").trim(); | |
| assertTrue(normalizedQuery.contains("SET Provisioned = true, SyncedDate = now(), syncedBy = ?")); // actual query uses lowercase 'syncedBy' | |
| assertTrue(normalizedQuery.contains("AND BeneficiaryID = ?")); | |
| assertTrue(normalizedQuery.contains("AND VanID = ?")); | |
| } catch (Exception e) { | |
| fail("Failed to test getqueryFor_M_BeneficiaryRegIdMapping: " + e.getMessage()); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
src/test/java/com/iemr/mmu/service/dataSyncLayerCentral/GetDataFromVanAndSyncToDBImplTest.java
around lines 571 to 590, the test uses reflection to call a private method
(getqueryFor_M_BeneficiaryRegIdMapping) and contains a debug System.out.println;
remove the println at line 582 and stop testing the private method via
reflection — instead test the behavior through the class's public API (call the
public method(s) that exercise getqueryFor_M_BeneficiaryRegIdMapping and assert
on their outcomes) or, if direct testing is essential, change the method
visibility to package-private and add a package-scoped test, then update the
test to call the method directly without reflection.
3 participants
📋 Description
JIRA ID:
@drtechie @vanitha1822
✅ Type of Change
Screenshots
N/A – This PR only affects backend unit tests and does not introduce UI changes.
Additional Notes:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.