Tests/services for Common-API #272
Conversation
|
Caution Review failedFailed to post review comments Pre-merge checks and finishing touches and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
✨ 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 |
56 tests run on CTIServiceImplTest successfully |
| if (everwellBenRegListAS != null) { | ||
|
|
||
| registerEverWellPatient(everwellBenRegListAS, Authorization); | ||
| registerEverWellPatient(everwellBenRegListAS, Authorization, new RestTemplate()); |
There was a problem hiding this comment.
Why is RestTemplate being passed here?
There was a problem hiding this comment.
because , since the rest template is being created and used in the function its not testable ... so passing a rest template allows the code to be tested
There was a problem hiding this comment.
another approach could also be that , the rest template could be defined at class level, but I wasnt sure that would be appropriate to do
There was a problem hiding this comment.
@Zapper9982 Is it really necessary to add this extra parameter to the function? Have you tested whether it will not affect the flow of the application?
There was a problem hiding this comment.
@vanitha1822 , yes as u can see I have changed all the calls to registereverwellPatient to have the rest template .
src/test/java/com/iemr/common/controller/carestream/CareStreamCreateOrderControllerTest.java
Show resolved
Hide resolved
…/CareStreamCreateOrderControllerTest.java
|
|
@Zapper9982 change the base branch to main |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 65
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/test/java/com/iemr/common/notification/agent/UserNotificationMappingServiceTest.java (6)
28-28: Remove duplicate import.The import
anyStringfrom Mockito appears twice (lines 28 and 66), which is redundant.Apply this diff to remove the duplicate:
-import static org.mockito.Mockito.anyString;Also applies to: 66-66
489-512: Remove duplicate test methods.These test methods duplicate tests already present at lines 336-360:
getMarkQuery_shouldReturnEmptyStringForAnyDto(line 490) duplicates line 337getMarkQuery_shouldReturnEmptyStringForNullDto(line 497) duplicates line 344getDeleteQuery_shouldReturnEmptyStringForAnyDto(line 502) duplicates line 350getDeleteQuery_shouldReturnEmptyStringForNullDto(line 509) duplicates line 357Remove these duplicate tests to maintain a clean test suite.
517-526: Remove duplicate test method.This test duplicates the test at lines 404-413 (
markNotificationList_shouldHandleNullList). Both tests verify the same behavior with null notification lists.Remove this duplicate test.
527-610: Remove duplicate test coverage.This entire section duplicates tests already present at lines 416-488:
getAlertAndNotificationCount_shouldReturnCountWithWorkingLocationID(line 528) duplicates line 416getAlertAndNotificationCount_shouldReturnCountWithoutWorkingLocationID(line 562) duplicates line 441getAlertAndNotificationCount_shouldHandleEmptyListFromRepo(line 586) duplicates line 467getAlertAndNotificationCount_shouldThrowNullPointerExceptionForNullDto(line 606) duplicates line 486The earlier tests already provide comprehensive coverage for all these scenarios.
Remove this duplicate test coverage to keep the test suite maintainable.
194-257: Enforce input validation and implement stub methods inquerySelector
- Add explicit null checks for
dtoanddto.getNotficationStatus(), throwingIllegalArgumentExceptionwith a descriptive message instead of relying on aNullPointerException.- Replace the stub returns in
getMarkQuery,getUnmarkQuery, andgetDeleteQuery(currently"") with actual query-building logic, and update tests to assert the real query strings.
362-380: Implement mapping logic and verify repository interaction
createUserNotificationMappingcurrently only iterates an empty block and returnstrue; add actual persistence via your repository.- Replace implicit NPE with an explicit null check or argument validation.
- Update tests to verify the repository call, for example:
@Test void createUserNotificationMapping_shouldReturnTrueForNonEmptyList() { List<Integer> userIds = Arrays.asList(1, 2, 3); Boolean result = userNotificationMappingService.createUserNotificationMapping(userIds); assertEquals(true, result); + // Verify repository interaction + verify(repo, times(1)).saveAll(anyList()); }src/test/java/com/iemr/common/controller/uptsu/UPTechnicalSupportControllerTest.java (1)
53-57: Remove redundant Mockito initialization; avoid resource leakUsing MockitoExtension already initializes mocks. Drop MockitoAnnotations.openMocks(this); otherwise you also need to close it in @AfterEach.
Apply this diff:
@BeforeEach void setUp() { - MockitoAnnotations.openMocks(this); mockMvc = MockMvcBuilders.standaloneSetup(controller).build(); }If you prefer keeping openMocks, store the AutoCloseable and close it in @AfterEach.
| private MockMvc mockMvc; | ||
|
|
||
| @Mock | ||
| private H104BenHistoryServiceImpl smpleBenHistoryServiceImpl; |
There was a problem hiding this comment.
Fix typo in mock variable name.
The variable name smpleBenHistoryServiceImpl appears to have a typo. Consider renaming to simpleBenHistoryServiceImpl or sampleBenHistoryServiceImpl for better readability.
- private H104BenHistoryServiceImpl smpleBenHistoryServiceImpl;
+ private H104BenHistoryServiceImpl simpleBenHistoryServiceImpl;📝 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.
| private H104BenHistoryServiceImpl smpleBenHistoryServiceImpl; | |
| private H104BenHistoryServiceImpl simpleBenHistoryServiceImpl; |
🤖 Prompt for AI Agents
In
src/test/java/com/iemr/common/controller/helpline104history/Helpline104BeneficiaryHistoryControllerTest.java
around line 56, the mock field is misspelled as `smpleBenHistoryServiceImpl`;
rename it to `sampleBenHistoryServiceImpl` (or `simpleBenHistoryServiceImpl` if
that matches project naming) and update all references/usages in the test class,
including any @InjectMocks/@Mock annotations and setup methods, to use the new
variable name so compilation and readability are fixed.
| mockHistoryList.add(history1); | ||
| mockHistoryList.add(history2); | ||
|
|
||
| when(smpleBenHistoryServiceImpl.geSmpleBenHistory(beneficiaryRegID)).thenReturn(mockHistoryList); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the service method name is correct.
The mocked method name geSmpleBenHistory appears to have typos (missing 't' in 'get' and 'smple' instead of 'simple/sample'). Ensure this matches the actual method name in H104BenHistoryServiceImpl.
🏁 Script executed:
#!/bin/bash
# Verify the actual method name in the service implementation
ast-grep --pattern 'class H104BenHistoryServiceImpl {
$$$
geSmpleBenHistory($$$) {
$$$
}
$$$
}'
# Also search for correctly spelled variants
ast-grep --pattern 'class H104BenHistoryServiceImpl {
$$$
getSimpleBenHistory($$$) {
$$$
}
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
set -e
echo "Occurrences of the mocked method:"
rg -n "geSmpleBenHistory" --glob '*.java' || true
echo -e "\nOccurrences of the correctly-spelled variant:"
rg -n "getSimpleBenHistory" --glob '*.java' || true
echo -e "\nService implementation classes (BenHistoryServiceImpl):"
rg -lP "class .*BenHistoryServiceImpl" --glob '*.java' || true
echo -e "\nContents of identified service impl files (first 200 lines):"
for file in $(rg -lP "class .*BenHistoryServiceImpl" --glob '*.java'); do
echo "### $file"
sed -n '1,200p' "$file"
doneLength of output: 5886
Rename method to getSimpleBenHistory
The service API, its implementation (H104BenHistoryServiceImpl), controller (Helpline104BeneficiaryHistoryController), and related tests currently use the typo’d method geSmpleBenHistory. Rename it to getSimpleBenHistory and update all references in H104BenHistoryService, H104BenHistoryServiceImpl, Helpline104BeneficiaryHistoryController, and their tests.
| @Test | ||
| void testGetActiveOccupations_withValidData() { | ||
| Set<Object[]> repoResult = new HashSet<>(); | ||
| repoResult.add(new Object[]{1L, "TestOccupation"}); | ||
| BeneficiaryOccupation occupation = new BeneficiaryOccupation().getOccupation(1L, "TestOccupation"); | ||
| when(beneficiaryOccupationRepository.getActiveOccupations()).thenReturn(repoResult); | ||
| List<BeneficiaryOccupation> result = service.getActiveOccupations(); | ||
| assertEquals(1, result.size()); | ||
| BeneficiaryOccupation actual = result.get(0); | ||
| assertEquals(occupation.getOccupationID(), actual.getOccupationID()); | ||
| assertEquals(occupation.getOccupationType(), actual.getOccupationType()); | ||
| } |
There was a problem hiding this comment.
Verify against expected values, not a locally constructed object.
The test creates a local BeneficiaryOccupation object at line 59 and then compares the service result against this object's properties (lines 64-65). This approach has two problems:
-
Line 59 pattern: Calling
new BeneficiaryOccupation().getOccupation(1L, "TestOccupation")is unusual. IfgetOccupationis a factory method, it should typically be static or the instance should not be instantiated separately. -
Lines 64-65: The test verifies that the service constructs objects the same way the test does, rather than asserting against known expected values (1L and "TestOccupation"). If the service has a bug in object construction that matches the test's construction bug, the test would pass incorrectly.
Additionally, the test should verify that the repository method was actually called, and should cover the case with multiple occupations.
Apply this diff to improve the test:
@Test
void testGetActiveOccupations_withValidData() {
Set<Object[]> repoResult = new HashSet<>();
repoResult.add(new Object[]{1L, "TestOccupation"});
- BeneficiaryOccupation occupation = new BeneficiaryOccupation().getOccupation(1L, "TestOccupation");
when(beneficiaryOccupationRepository.getActiveOccupations()).thenReturn(repoResult);
+
List<BeneficiaryOccupation> result = service.getActiveOccupations();
+
assertEquals(1, result.size());
BeneficiaryOccupation actual = result.get(0);
- assertEquals(occupation.getOccupationID(), actual.getOccupationID());
- assertEquals(occupation.getOccupationType(), actual.getOccupationType());
+ assertEquals(1L, actual.getOccupationID());
+ assertEquals("TestOccupation", actual.getOccupationType());
+ verify(beneficiaryOccupationRepository).getActiveOccupations();
}📝 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 testGetActiveOccupations_withValidData() { | |
| Set<Object[]> repoResult = new HashSet<>(); | |
| repoResult.add(new Object[]{1L, "TestOccupation"}); | |
| BeneficiaryOccupation occupation = new BeneficiaryOccupation().getOccupation(1L, "TestOccupation"); | |
| when(beneficiaryOccupationRepository.getActiveOccupations()).thenReturn(repoResult); | |
| List<BeneficiaryOccupation> result = service.getActiveOccupations(); | |
| assertEquals(1, result.size()); | |
| BeneficiaryOccupation actual = result.get(0); | |
| assertEquals(occupation.getOccupationID(), actual.getOccupationID()); | |
| assertEquals(occupation.getOccupationType(), actual.getOccupationType()); | |
| } | |
| @Test | |
| void testGetActiveOccupations_withValidData() { | |
| Set<Object[]> repoResult = new HashSet<>(); | |
| repoResult.add(new Object[]{1L, "TestOccupation"}); | |
| when(beneficiaryOccupationRepository.getActiveOccupations()).thenReturn(repoResult); | |
| List<BeneficiaryOccupation> result = service.getActiveOccupations(); | |
| assertEquals(1, result.size()); | |
| BeneficiaryOccupation actual = result.get(0); | |
| assertEquals(1L, actual.getOccupationID()); | |
| assertEquals("TestOccupation", actual.getOccupationType()); | |
| verify(beneficiaryOccupationRepository).getActiveOccupations(); | |
| } |
🤖 Prompt for AI Agents
In
src/test/java/com/iemr/common/service/beneficiary/BeneficiaryOccupationServiceImplTest.java
around lines 55 to 66, the test builds a local BeneficiaryOccupation via new
BeneficiaryOccupation().getOccupation(...) and compares the service result to
that locally constructed object; instead assert against the literal expected
values (assertEquals(1L, actual.getOccupationID()) and
assertEquals("TestOccupation", actual.getOccupationType())), avoid calling
instance factory (use the constructor or make getOccupation static if factory
behavior is required), add
verify(beneficiaryOccupationRepository).getActiveOccupations() to ensure the
repo was called, and extend the test to include multiple Object[] entries in
repoResult and assert the returned list size and contents for each expected
id/type pair.
| @Test | ||
| void testSetBeneficiaryRelationshipTypeRepository() { | ||
| BenRelationshipTypeServiceImpl impl = new BenRelationshipTypeServiceImpl(); | ||
| BeneficiaryRelationshipTypeRepository repo = mock(BeneficiaryRelationshipTypeRepository.class); | ||
| impl.setBeneficiaryRelationshipTypeRepository(repo); | ||
| // No exception, repository is set | ||
| } |
There was a problem hiding this comment.
Test provides minimal verification value.
This test only confirms the setter doesn't throw an exception but doesn't verify the repository was actually stored or can be retrieved. Consider either adding assertions to verify the wiring worked (e.g., by invoking a method that uses the repository) or removing this test if the setter behavior is trivial.
🤖 Prompt for AI Agents
In
src/test/java/com/iemr/common/service/beneficiary/BenRelationshipTypeServiceImplTest.java
around lines 47 to 53, the test only ensures the setter doesn't throw but
doesn't verify the repository was stored; update the test to either assert the
repository was saved (for example call a public method on
BenRelationshipTypeServiceImpl that delegates to the repository using the mock
and verify interaction, or if a getter exists assert it equals the mock) or, if
no meaningful behavior to test, remove the test; if the repository field is
private with no getter, use reflection in the test to read the private field and
assert it equals the mock, or prefer verifying via a delegated method and
Mockito.verify to keep the test behavior-focused.
| @Test | ||
| void testGetActiveRelationshipTypes_withNullObject() { | ||
| Set<Object[]> repoResult = new HashSet<>(); | ||
| repoResult.add(null); | ||
| when(beneficiaryRelationshipTypeRepository.getActiveRelationships()).thenReturn(repoResult); | ||
| List<BenRelationshipType> result = service.getActiveRelationshipTypes(); | ||
| assertTrue(result.isEmpty()); | ||
| } | ||
|
|
||
| @Test | ||
| void testGetActiveRelationshipTypes_withInvalidLength() { | ||
| Set<Object[]> repoResult = new HashSet<>(); | ||
| repoResult.add(new Object[]{1}); // length < 2 | ||
| when(beneficiaryRelationshipTypeRepository.getActiveRelationships()).thenReturn(repoResult); | ||
| List<BenRelationshipType> result = service.getActiveRelationshipTypes(); | ||
| assertTrue(result.isEmpty()); | ||
| } | ||
|
|
||
| @Test | ||
| void testGetActiveRelationshipTypes_withEmptySet() { | ||
| when(beneficiaryRelationshipTypeRepository.getActiveRelationships()).thenReturn(Collections.emptySet()); | ||
| List<BenRelationshipType> result = service.getActiveRelationshipTypes(); | ||
| assertTrue(result.isEmpty()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify null return handling from repository.
The edge case tests are well-structured and cover null entries, invalid length, and empty sets. However, there's no test for when the repository itself returns null instead of an empty Set. This could cause a NullPointerException in the service if not handled.
Run this script to check if the service handles null repository return:
If the service doesn't handle null returns, consider adding a test case:
@Test
void testGetActiveRelationshipTypes_withNullReturn() {
when(beneficiaryRelationshipTypeRepository.getActiveRelationships()).thenReturn(null);
List<BenRelationshipType> result = service.getActiveRelationshipTypes();
assertTrue(result.isEmpty());
}🏁 Script executed:
#!/bin/bash
# Description: Check if BenRelationshipTypeServiceImpl handles null return from repository
# Search for null checks on the repository return value
ast-grep --pattern $'class BenRelationshipTypeServiceImpl {
$$$
getActiveRelationshipTypes($$$) {
$$$
}
$$$
}'
# Also search for null handling patterns
rg -nP --type=java -A10 'getActiveRelationships\(\)' -g '**/BenRelationshipTypeServiceImpl.java'Length of output: 1215
Add null‐return guard in getActiveRelationshipTypes and corresponding unit test
- In BenRelationshipTypeServiceImpl.getActiveRelationshipTypes(), check if
beneficiaryRelationshipTypeRepository.getActiveRelationships()is null (e.g. useOptional.ofNullable(...).orElse(Collections.emptySet())) before iterating to avoid NPE and ensure an empty list is returned. - Add a test case in BenRelationshipTypeServiceImplTest:
@Test void testGetActiveRelationshipTypes_withNullReturn() { when(beneficiaryRelationshipTypeRepository.getActiveRelationships()).thenReturn(null); List<BenRelationshipType> result = service.getActiveRelationshipTypes(); assertTrue(result.isEmpty()); }
🤖 Prompt for AI Agents
In
src/test/java/com/iemr/common/service/beneficiary/BenRelationshipTypeServiceImplTest.java
around lines 69 to 92, the tests and service assume
beneficiaryRelationshipTypeRepository.getActiveRelationships() never returns
null; update BenRelationshipTypeServiceImpl.getActiveRelationshipTypes() to
guard against a null return (e.g. wrap the call with
Optional.ofNullable(...).orElse(Collections.emptySet()) or check for null and
use Collections.emptySet()) before iterating so it returns an empty list instead
of throwing NPE, and add the unit test
testGetActiveRelationshipTypes_withNullReturn() that mocks the repository to
return null and asserts the service returns an empty list.
| @Mock | ||
| private LonicRepository lonicRepository; | ||
|
|
||
| private final Integer LOMIC_PAGE_SIZE = 10; |
There was a problem hiding this comment.
Fix typo in constant name.
The constant name LOMIC_PAGE_SIZE should be LONIC_PAGE_SIZE to correctly reflect "LONIC" (Logical Observation Identifiers Names and Codes).
Apply this diff to fix the typo:
- private final Integer LOMIC_PAGE_SIZE = 10;
+ private final Integer LONIC_PAGE_SIZE = 10;Also update the references to this constant on lines 68, 87, and 117.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/test/java/com/iemr/common/service/lonic/LonicServiceImplTest.java around
line 64, rename the constant LOMIC_PAGE_SIZE to LONIC_PAGE_SIZE and update all
references accordingly (specifically on lines 68, 87, and 117) so the identifier
correctly spells "LONIC"; ensure any imports/usages compile after the rename and
run tests to verify no remaining references to the old name.
| @BeforeEach | ||
| void setUp() { | ||
| ReflectionTestUtils.setField(lonicService, "lonicPageSize", LOMIC_PAGE_SIZE); | ||
| new OutputMapper(); |
There was a problem hiding this comment.
Remove unused OutputMapper instantiation.
The new OutputMapper() statement creates an instance that is never used. Since the test later calls OutputMapper.gson() as a static method (line 96), this instantiation appears to be dead code.
If this was intended to trigger static initialization, please clarify with a comment. Otherwise, apply this diff to remove the unused instantiation:
@BeforeEach
void setUp() {
ReflectionTestUtils.setField(lonicService, "lonicPageSize", LOMIC_PAGE_SIZE);
- new OutputMapper();
}📝 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.
| new OutputMapper(); | |
| @BeforeEach | |
| void setUp() { | |
| ReflectionTestUtils.setField(lonicService, "lonicPageSize", LOMIC_PAGE_SIZE); | |
| } |
🤖 Prompt for AI Agents
In src/test/java/com/iemr/common/service/lonic/LonicServiceImplTest.java around
line 69, there is an unused instantiation "new OutputMapper();" which is dead
code because the test later uses the static OutputMapper.gson() method; remove
the unnecessary "new OutputMapper();" line (or if it was intentionally used to
trigger static initialization, replace it with a short clarifying comment
explaining that intent) so the test contains no unused object creation.
| @BeforeEach | ||
| void setUp() { | ||
| ReflectionTestUtils.setField(questionnaireService, "inputMapper", mockInputMapper); | ||
| // Remove or correct the following line if InputMapper.gson() does not return Gson | ||
| // when(mockInputMapper.gson()).thenReturn(mockGson); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Clarify or remove the commented-out code with the uncertainty note.
The commented lines 72-73 indicate confusion about the InputMapper.gson() API. Based on the relevant code snippets from other test files (e.g., IdentityBeneficiaryServiceImplTest, CalltypeServiceImplTest), InputMapper.gson() is a static method that returns an InputMapper instance, not a Gson instance. Either remove the commented-out code if it's not needed, or clarify the setup by consulting the actual InputMapper implementation.
Consider running the following script to examine the InputMapper class and confirm the return type of gson():
🏁 Script executed:
#!/bin/bash
# Description: Find and display the InputMapper class to verify the gson() method signature
# Search for InputMapper class definition
ast-grep --pattern $'class InputMapper {
$$$
gson($$$) {
$$$
}
$$$
}'
# Also search for static gson method
rg -nP --type=java -C5 'static.*gson\s*\('Length of output: 6628
Remove mocked gson() call and stub fromJson() instead
Delete the commented lines 72–73 (when(mockInputMapper.gson()).thenReturn(mockGson)). InputMapper.gson() is static and returns an InputMapper, not a Gson. If you need to mock JSON deserialization, stub mockInputMapper.fromJson(jsonString, YourDtoClass.class) to return the desired object.
🤖 Prompt for AI Agents
In
src/test/java/com/iemr/common/service/questionconfig/QuestionnaireServiceImplTest.java
around lines 69 to 74, remove the commented stub that mocks
mockInputMapper.gson() because InputMapper.gson() is static and returns an
InputMapper (not a Gson); instead, stub the instance method used for
deserialization by mocking mockInputMapper.fromJson(jsonString,
YourDtoClass.class) to return the desired DTO object (or use
ArgumentMatchers.anyString()/any(Class.class) as appropriate) and ensure
ReflectionTestUtils continues to inject mockInputMapper into
questionnaireService.
| @Test | ||
| void createQuestionnaire_shouldSaveAndReturnDetails() throws Exception { | ||
| String requestJson = "[{\"questionID\":1,\"question\":\"Q1\",\"questionDesc\":\"Desc1\"},{\"questionID\":2,\"question\":\"Q2\",\"questionDesc\":\"Desc2\"}]"; | ||
| QuestionnaireDetail detail1 = new QuestionnaireDetail(1L, "Q1", "Desc1"); | ||
| QuestionnaireDetail detail2 = new QuestionnaireDetail(2L, "Q2", "Desc2"); | ||
| QuestionnaireDetail[] questionnaireDetails = {detail1, detail2}; | ||
|
|
||
| when(questionnaireRepository.save(any(QuestionnaireDetail.class))).thenAnswer(invocation -> invocation.getArgument(0)); | ||
|
|
||
| String result = questionnaireService.createQuestionnaire(requestJson); | ||
|
|
||
| assertEquals(Arrays.toString(questionnaireDetails), result); | ||
| ArgumentCaptor<QuestionnaireDetail> captor = ArgumentCaptor.forClass(QuestionnaireDetail.class); | ||
| verify(questionnaireRepository, times(2)).save(captor.capture()); | ||
| // Assert both QuestionnaireDetail objects were saved with correct fields | ||
| boolean found1 = false, found2 = false; | ||
| for (QuestionnaireDetail captured : captor.getAllValues()) { | ||
| if (captured.getQuestionID().equals(detail1.getQuestionID()) && | ||
| captured.getQuestion().equals(detail1.getQuestion()) && | ||
| captured.getQuestionDesc().equals(detail1.getQuestionDesc())) { | ||
| found1 = true; | ||
| } | ||
| if (captured.getQuestionID().equals(detail2.getQuestionID()) && | ||
| captured.getQuestion().equals(detail2.getQuestion()) && | ||
| captured.getQuestionDesc().equals(detail2.getQuestionDesc())) { | ||
| found2 = true; | ||
| } | ||
| } | ||
| assertEquals(true, found1, "detail1 was saved"); | ||
| assertEquals(true, found2, "detail2 was saved"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify JSON deserialization is properly tested.
The test creates QuestionnaireDetail objects directly (lines 87-89) but doesn't mock the JSON parsing that would convert the requestJson string into these objects. The service implementation likely uses Gson or InputMapper to deserialize the JSON, but this test doesn't set up that mocking.
Looking at similar tests in the codebase (e.g., IdentityBeneficiaryServiceImplTest, CalltypeServiceImplTest), they use mockStatic(InputMapper.class) to mock the static gson() method and then mock fromJson() behavior. Without this setup, the actual JSON parsing logic isn't being tested, or the test might not work as expected.
Either:
- Add proper mocking for JSON deserialization:
try (MockedStatic<InputMapper> inputMapperMock = mockStatic(InputMapper.class)) {
InputMapper mockStaticMapper = mock(InputMapper.class);
inputMapperMock.when(InputMapper::gson).thenReturn(mockStaticMapper);
QuestionnaireDetail[] questionnaireDetails = {detail1, detail2};
when(mockStaticMapper.fromJson(eq(requestJson), eq(QuestionnaireDetail[].class)))
.thenReturn(questionnaireDetails);
// Then proceed with existing test logic
when(questionnaireRepository.save(any(QuestionnaireDetail.class)))
.thenAnswer(invocation -> invocation.getArgument(0));
String result = questionnaireService.createQuestionnaire(requestJson);
// assertions...
}- Or document that this test is only verifying the save/iteration logic, assuming JSON parsing works, and that JSON parsing is tested elsewhere or cannot be tested (as mentioned in the PR objectives for some methods).
Run this script to examine the service implementation and confirm how JSON parsing is performed:
🏁 Script executed:
#!/bin/bash
# Description: Examine the QuestionnaireServiceImpl to verify JSON parsing approach
# Search for createQuestionnaire method implementation
ast-grep --pattern $'class QuestionnaireServiceImpl {
$$$
createQuestionnaire($$$) {
$$$
}
$$$
}'
# Also search for Gson usage patterns
rg -nP --type=java -C10 'createQuestionnaire.*\{' src/main/java/com/iemr/common/service/questionconfig/Length of output: 2383
**Mock JSON deserialization in the test.**
The service uses `inputMapper.gson().fromJson(request, QuestionnaireDetail[].class)`, but the test never stubs `inputMapper` or `InputMapper.gson()`. You must either:
- Mock the static `InputMapper.gson()` (or inject a mock `inputMapper`) and stub `fromJson(requestJson, QuestionnaireDetail[].class)` to return your `questionnaireDetails` array before calling `createQuestionnaire()`,
or
- Instantiate a real mapper in the test so that JSON parsing succeeds reliably.
<details>
<summary>🤖 Prompt for AI Agents</summary>
In
src/test/java/com/iemr/common/service/questionconfig/QuestionnaireServiceImplTest.java
around lines 84 to 114, the test calls the service which uses
inputMapper.gson().fromJson(...) but the test never stubs or provides a real
InputMapper, so JSON deserialization returns null/fails; fix by either mocking
InputMapper (or its static gson() method) and stubbing fromJson(requestJson,
QuestionnaireDetail[].class) to return the questionnaireDetails array before
invoking createQuestionnaire(), or by constructing and injecting a real
InputMapper/Gson instance into the service in the test setup so the requestJson
is parsed into the expected QuestionnaireDetail[].
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
| @Mock | ||
| ServiesRepository serviesRepository; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify repository name spelling.
The repository is named ServiesRepository which appears to be missing a 'c'. Confirm whether this is intentional or should be ServicesRepository.
Run the following script to check the actual repository class name:
🏁 Script executed:
#!/bin/bash
# Description: Find the actual repository class name to verify spelling
# Search for the repository class definition
rg -n --type=java 'class.*Servi.*Repository'
# Search for the repository interface definition
rg -n --type=java 'interface.*Servi.*Repository'Length of output: 1441
Correct repository interface spelling: Rename ServiesRepository to ServicesRepository (add missing 'c') and update all references accordingly.
🤖 Prompt for AI Agents
In src/test/java/com/iemr/common/service/services/ServicesImplTest.java around
lines 43-44, the mocked repository interface is misspelled as ServiesRepository;
rename it to ServicesRepository everywhere in this file (mock declaration,
imports, usages) and update any other project references to the correct
interface name to match the actual repository interface; ensure IDE/refactor
tool updates imports and compile-time references so tests compile.
2 participants
📋 Description
This pull request introduces a significant number of new unit tests and fixes for existing tests across more than 80 files. The primary motivation is to enhance the overall test coverage of the
Common-APIservice layer, improve code quality, and ensure the reliability of existing and new functionality. The changes address various failing tests and add comprehensive test cases for previously untested code paths.NOTE :
Some of the extremely huge services which are Admin and CTI do not have a testcase cause of hallucinations will be looked into it with a later PR .
Some of the services have a
RestTemplateinstantiated within the method and not as a class declaration causing the methods to be rendered non - testable@vanitha1822
✅ Type of Change
ℹ️ Additional Information
The changes were tested by running the full suite of unit tests locally. The focus has been on:
NullPointerException, incorrect mocking, or assertion errors.The work spans over 80 files, indicating a widespread effort to improve the test harness of the application.
Screenshots
@drtechie @vanitha1822 Raising a PR , a total of 1705 Testcases are currently written.
Some of the failures are because of :
Summary by CodeRabbit