Add REST and SOAP endpoint test base classes with comprehensive examples#765
Add REST and SOAP endpoint test base classes with comprehensive examples#765yingbull wants to merge 8 commits into
Conversation
Add CarlosRestTestBase and CarlosSoapTestBase base classes that use CXF's local transport for in-memory HTTP-level endpoint testing. This enables testing the full CXF pipeline (routing, serialization, interceptors) without TCP sockets or an embedded servlet container. - Add cxf-rt-transports-local test dependency (CXF 4.1.5) - CarlosRestTestBase: JAX-RS server with Jackson JSON provider and test auth interceptor for LoggedInInfo injection - CarlosSoapTestBase: JAX-WS server with typed proxy client generation and test auth interceptor - AllergyServiceEndpointTest: example REST endpoint test - SystemInfoWsEndpointTest: example SOAP endpoint test - Add **/*EndpointTest.java to Surefire include patterns - Add endpoint-testing-guide.md documentation - Update CLAUDE.md with new base classes and endpoint tags https://claude.ai/code/session_01768WpqTVcnxHuMFNoUssVU
- SOAP interceptor: also set MessageContext.SERVLET_REQUEST key so AbstractWs.getLoggedInInfo() works (reads via WebServiceContext, not AbstractHTTPDestination) - REST: add request() helper that returns a fresh WebClient copy, preventing path accumulation across multiple calls in one test - Use new LoggedInInfo().LOGGED_IN_INFO_KEY instead of hardcoding the key formula, preventing silent divergence if the key changes - Add JavaDoc warnings about Mockito openMocks lifecycle - Update example test and docs to use request() pattern https://claude.ai/code/session_01768WpqTVcnxHuMFNoUssVU
Add 45 endpoint test files covering all 33 REST services and 13 SOAP services using CXF local transport. Tests verify HTTP routing, status codes, and JSON/XML serialization without database or Spring context. REST tests (31 new): StatusService, MessagingService, InboxService, PreventionService, ProgramService, AppService, MeasurementService, BillingService, ResourceService, PharmacyService, PatientDetailStatus, DemographicMergeService, EFormService, EFormsService, ConsentService, RxLookupService, LabService, DocumentService, DiseaseRegistryService, TicklerWebService, FormsService, ProviderService, PersonaService, OscarJobService, ProductDispensingService, NotesService, RecordUxService, DemographicService, ScheduleService, ConsultationWebService, RxWebService, ReportingService SOAP tests (13 new): FacilityWs, ProgramWs, ProviderWs, LoginWs, AllergyWs, PreventionWs, PrescriptionWs, MeasurementWs, DocumentWs, DemographicWs, BookingWs, ScheduleWs, LabUploadWs https://claude.ai/code/session_01768WpqTVcnxHuMFNoUssVU
Align mock method names with actual DemographicSetsManager and EFormReportToolManager signatures. Remove unused imports. https://claude.ai/code/session_01768WpqTVcnxHuMFNoUssVU
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds CXF local-transport endpoint testing: two test base classes (REST + SOAP), Maven test-scope CXF wiring and Surefire include, ~30 new REST and SOAP endpoint test classes, and documentation (endpoint-testing guide and CLAUDE.md updates). ChangesEndpoint testing (docs, infra, wiring, tests)
Sequence Diagram(s)sequenceDiagram
participant Test as Test Class
participant Setup as BeforeEach Setup
participant Mocks as Mockito
participant CXF as CXF Local Transport
participant Service as Service Bean
participant Client as Test Client (WebClient / JAX-WS proxy)
participant Assert as Test Assertion
Test->>Setup: run `@BeforeEach`
Setup->>Mocks: init mocks & stub behaviours
Setup->>Service: instantiate service bean
Setup->>Service: inject mocked dependencies
Setup->>CXF: start local CXF server with service bean
Setup->>Client: build client targeting local address
Test->>Client: invoke endpoint (HTTP/SOAP)
Client->>CXF: request via local transport
CXF->>Service: dispatch to service bean
Service->>Mocks: call mocked collaborators
Mocks-->>Service: return stubbed data
Service-->>CXF: return response
CXF-->>Client: deliver response
Client->>Assert: verify status/body/assertions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @yingbull, your pull request is larger than the review limit of 150000 diff characters
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. OpenSSF Scorecard
Scanned Files
|
PR Review 🔍
|
PR Code Suggestions ✨
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf81037a04
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * @return the local transport address (default: {@code "local://rest-test"}) | ||
| */ | ||
| protected String getServiceAddress() { | ||
| return "local://rest-test"; |
There was a problem hiding this comment.
The base URI is hard-coded to local://rest-test, so every REST endpoint test class binds the same CXF local-transport destination. Because these new *EndpointTest classes are now included in Surefire and executed with class-level parallelism, concurrent classes in the same fork can race on the same address, causing intermittent server creation failures or request cross-talk between tests. Make the default address unique per class (or per test) to keep parallel execution deterministic.
Useful? React with 👍 / 👎.
| * @return the local transport address (default: {@code "local://soap-test"}) | ||
| */ | ||
| protected String getServiceAddress() { | ||
| return "local://soap-test"; |
There was a problem hiding this comment.
The SOAP base class also reuses a single fixed address (local://soap-test) for every test class. Under the project's parallel test execution settings, multiple SOAP endpoint classes can initialize servers at the same local URI concurrently, which makes the suite flaky with destination binding conflicts and non-deterministic failures. Assign a unique local transport address per class/test instead of a global constant.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new testing infrastructure for REST and SOAP endpoints using CXF's local transport, enabling fast, in-memory verification of the service pipeline without network overhead. It adds the CarlosRestTestBase and CarlosSoapTestBase classes, updates the project configuration, and provides extensive test coverage for numerous web services along with a detailed testing guide. I have no feedback to provide.
There was a problem hiding this comment.
Pull request overview
This PR adds a CXF local-transport based testing approach for HTTP-level REST (JAX-RS) and SOAP (JAX-WS) endpoints, along with a large set of example endpoint tests and supporting documentation/dependency updates.
Changes:
- Adds many new
*EndpointTestclasses demonstrating REST and SOAP endpoint testing patterns using CXF local transport. - Updates Maven Surefire to include
**/*EndpointTest.javaand adds the CXF local transport test dependency. - Adds endpoint-testing documentation and updates
CLAUDE.mdtagging/base-class guidance.
Reviewed changes
Copilot reviewed 52 out of 52 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/io/github/carlos_emr/carlos/webserv/SystemInfoWsEndpointTest.java | SOAP local-transport tests for SystemInfoWs operations |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/StatusServiceEndpointTest.java | REST local-transport test for status/auth check |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/ScheduleServiceEndpointTest.java | REST local-transport tests for schedule/statuses/day/add |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/RxLookupServiceEndpointTest.java | REST local-transport tests for drug lookup/search/details |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/ResourceServiceEndpointTest.java | REST local-transport tests for resources version endpoints |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/ReportingServiceEndpointTest.java | REST local-transport tests for reporting endpoints |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/RecordUxServiceEndpointTest.java | REST local-transport tests for record UX endpoints |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/ProviderServiceEndpointTest.java | REST local-transport tests for provider endpoints |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/ProgramServiceEndpointTest.java | REST local-transport tests for program list endpoint |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/ProductDispensingServiceEndpointTest.java | REST local-transport tests for dispensing/product endpoints |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/PreventionServiceEndpointTest.java | REST local-transport tests for preventions/immunizations |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/PharmacyServiceEndpointTest.java | REST local-transport tests for pharmacy CRUD-ish endpoints |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/PersonaServiceEndpointTest.java | REST local-transport tests for persona/rights/navbar endpoints |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/PatientDetailStatusServiceEndpointTest.java | REST local-transport tests for patient detail status + uniqueness check |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/OscarJobServiceEndpointTest.java | REST local-transport tests for job and job-type endpoints |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/NotesServiceEndpointTest.java | REST local-transport tests for clinical notes filtering endpoint |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/MessagingServiceEndpointTest.java | REST local-transport tests for messaging list/count endpoints |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/MeasurementServiceEndpointTest.java | REST local-transport tests for measurement fetch endpoint |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/LabServiceEndpointTest.java | REST local-transport tests for HL7 lab listing endpoint |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/InboxServiceEndpointTest.java | REST local-transport tests for inbox endpoints (incl. SpringUtils.getBean DAO) |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/EFormsServiceEndpointTest.java | REST local-transport tests for eForms list endpoint |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/EFormServiceEndpointTest.java | REST local-transport tests for eForm load/save endpoints |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/DocumentServiceEndpointTest.java | REST local-transport tests for document save/upload endpoints |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/DiseaseRegistryServiceEndpointTest.java | REST local-transport tests for disease registry endpoints |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/DemographicMergeServiceEndpointTest.java | REST local-transport tests for demographic merge/unmerge endpoints |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/ConsultationWebServiceEndpointTest.java | REST local-transport tests for consultation search/get endpoints |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/ConsentServiceEndpointTest.java | REST local-transport tests for consent endpoints |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/BillingServiceEndpointTest.java | REST local-transport tests for billing config endpoints |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/AppServiceEndpointTest.java | REST local-transport tests for app definitions endpoint |
| src/test/java/io/github/carlos_emr/carlos/webserv/rest/AllergyServiceEndpointTest.java | REST local-transport tests for allergy endpoints |
| src/test/java/io/github/carlos_emr/carlos/webserv/ProviderWsEndpointTest.java | SOAP local-transport tests for ProviderWs operations |
| src/test/java/io/github/carlos_emr/carlos/webserv/ProgramWsEndpointTest.java | SOAP local-transport tests for ProgramWs operations |
| src/test/java/io/github/carlos_emr/carlos/webserv/PreventionWsEndpointTest.java | SOAP local-transport tests for PreventionWs operations |
| src/test/java/io/github/carlos_emr/carlos/webserv/PrescriptionWsEndpointTest.java | SOAP local-transport tests for PrescriptionWs operations |
| src/test/java/io/github/carlos_emr/carlos/webserv/MeasurementWsEndpointTest.java | SOAP local-transport tests for MeasurementWs operations |
| src/test/java/io/github/carlos_emr/carlos/webserv/LoginWsEndpointTest.java | SOAP local-transport tests for LoginWs (static WsUtils mocking) |
| src/test/java/io/github/carlos_emr/carlos/webserv/LabUploadWsEndpointTest.java | SOAP local-transport tests for LabUploadWs (static-heavy mocking) |
| src/test/java/io/github/carlos_emr/carlos/webserv/FacilityWsEndpointTest.java | SOAP local-transport tests for FacilityWs operations |
| src/test/java/io/github/carlos_emr/carlos/webserv/DocumentWsEndpointTest.java | SOAP local-transport tests for DocumentWs operations |
| src/test/java/io/github/carlos_emr/carlos/webserv/DemographicWsEndpointTest.java | SOAP local-transport tests for DemographicWs operations |
| src/test/java/io/github/carlos_emr/carlos/webserv/BookingWsEndpointTest.java | SOAP local-transport tests for BookingWs operations |
| src/test/java/io/github/carlos_emr/carlos/webserv/AllergyWsEndpointTest.java | SOAP local-transport tests for AllergyWs operations |
| pom.xml | Adds CXF local transport test dependency; includes *EndpointTest in Surefire |
| docs/test/endpoint-testing-guide.md | New guide documenting REST/SOAP endpoint testing approach |
| CLAUDE.md | Documents endpoint tags and new endpoint base classes |
| @Override | ||
| protected Object getServiceBean() { | ||
| ResourceService service = new ResourceService(); | ||
| injectDependency(service, "securityInfoManager", mockSecurityInfoManager); | ||
| injectDependency(service, "appDefinitionDao", mockAppDefinitionDao); | ||
| injectDependency(service, "appManager", mockAppManager); | ||
| injectDependency(service, "appUserDao", mockAppUserDao); | ||
| injectDependency(service, "resourceStorageDao", mockResourceStorageDao); | ||
| injectDependency(service, "preventionDS", mockPreventionDS); | ||
| registerMock(CarlosProperties.class, mockCarlosProperties); | ||
| return service; | ||
| } |
There was a problem hiding this comment.
ResourceService calls CarlosProperties.getInstance() directly, so registering mockCarlosProperties via registerMock(CarlosProperties.class, ...) (SpringUtils mocking) will not affect the code under test. This makes the test environment-dependent and may fail depending on the real properties on the build agent. Use a MockedStatic<CarlosProperties> to stub CarlosProperties.getInstance() (and close it in @AfterEach), or refactor the service to use an injectable CarlosProperties field similar to BillingService/PatientDetailStatusService so injectDependency() can override it.
| <configuration> | ||
| <!-- Each fork is a separate JVM with its own H2 mem:testdb instance, | ||
| so forks are completely isolated and safe to run in parallel. --> | ||
| <forkCount>4</forkCount> | ||
| <reuseForks>true</reuseForks> | ||
| <!-- Run test classes in parallel within each fork. | ||
| Safe because: integration tests use @Transactional @Rollback (each test | ||
| gets its own transaction), and the few static-mock unit tests don't share | ||
| Spring context with integration tests. --> | ||
| <parallel>classes</parallel> | ||
| <threadCount>2</threadCount> | ||
| <perCoreThreadCount>false</perCoreThreadCount> | ||
| <includes> | ||
| <include>**/test/**/*Test.java</include> | ||
| <include>**/test/**/*Tests.java</include> | ||
| <include>**/tickler/**/*Test.java</include> | ||
| <include>**/messenger/**/*Test.java</include> | ||
| <include>**/provider/**/*Test.java</include> | ||
| <include>**/managers/**/*Test.java</include> | ||
| <include>**/*IntegrationTest.java</include> | ||
| <include>**/*UnitTest.java</include> | ||
| <include>**/*EndpointTest.java</include> | ||
| </includes> |
There was a problem hiding this comment.
Adding **/*EndpointTest.java to Surefire includes will run many CXF local-transport endpoint tests under the existing parallel test execution settings (<parallel>classes</parallel>, multiple forks). CarlosRestTestBase/CarlosSoapTestBase currently default to fixed addresses (local://rest-test / local://soap-test), which can collide when endpoint tests run concurrently in the same JVM, causing flaky bind/startup failures or cross-test interference. Either make the base classes generate a unique local transport address per test class (e.g., include class name/UUID), or adjust Surefire to run *EndpointTest classes non-parallel (separate execution / disable <parallel> for this include).
| <dependency> | ||
| <groupId>org.apache.cxf</groupId> | ||
| <artifactId>cxf-rt-transports-local</artifactId> | ||
| <version>${cxf.version}</version> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
PR description mentions adding cxf-rt-transport-local, but the POM adds cxf-rt-transports-local (plural). If the description is meant to be exact (e.g., for release notes or dependency auditing), please align the PR text with the actual artifactId (or vice versa).
Signed-off-by: Michael Yingbull <michael@maplecreekmedical.ca>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| .thenReturn(true); | ||
| } | ||
|
|
||
| private Hl7TextMessage createTestHl7Message(int id) { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (23)
src/test/java/io/github/carlos_emr/carlos/webserv/rest/AppServiceEndpointTest.java (1)
69-90: Assert response payload, not only HTTP 200.Both scenarios currently pass as long as the endpoint returns status 200. Please also assert the response entity (e.g., non-empty list vs empty list) to validate routing + serialization behavior end-to-end.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/github/carlos_emr/carlos/webserv/rest/AppServiceEndpointTest.java` around lines 69 - 90, Update the two tests in AppServiceEndpointTest (shouldReturn200_whenAppsExist and shouldReturn200WithEmptyList_whenNoAppsExist) to assert the response entity in addition to status: after calling request().path("/app/getApps/").get() use Response.readEntity(...) or equivalent JSON deserialization to obtain the returned list and assert its contents (for shouldReturn200_whenAppsExist assert the list size is 1 and/or contains an AppDefinitionTo1 instance matching the mocked app; for shouldReturn200WithEmptyList_whenNoAppsExist assert the list is empty). Also ensure you still verify mockAppManager.getAppDefinitions(any(LoggedInInfo.class)) is used as part of the setup.src/test/java/io/github/carlos_emr/carlos/webserv/rest/PharmacyServiceEndpointTest.java (2)
116-128: DELETE test should verify soft-delete behavior.The test asserts HTTP 200 but doesn't verify that the pharmacy was actually soft-deleted. Consider adding an assertion to confirm the expected behavior (e.g., that
pharmacy.setArchived(true)or equivalent was invoked beforesaveEntity).💡 Suggested improvement
void shouldReturn200_whenPharmacySoftDeleted() { PharmacyInfo pharmacy = new PharmacyInfo(); pharmacy.setId(1); pharmacy.setName("Old Pharmacy"); when(mockPharmacyInfoDao.find(eq(1))).thenReturn(pharmacy); when(mockPharmacyInfoDao.saveEntity(any(PharmacyInfo.class))).thenReturn(pharmacy); Response response = request().replaceHeader("Accept", "*/*").path("/pharmacies/1").delete(); assertThat(response.getStatus()).isEqualTo(200); + verify(mockPharmacyInfoDao).saveEntity(argThat(p -> + p.getId() == 1 && /* verify soft-delete flag is set */)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/github/carlos_emr/carlos/webserv/rest/PharmacyServiceEndpointTest.java` around lines 116 - 128, Update the test method shouldReturn200_whenPharmacySoftDeleted to assert the soft-delete actually occurred: after invoking the DELETE request, verify that PharmacyInfo was marked archived (e.g., pharmacy.getArchived()/isArchived() is true) and that mockPharmacyInfoDao.saveEntity(...) was called with that archived PharmacyInfo; you can either assert the in-memory pharmacy object's archived flag or use an ArgumentCaptor/verify on mockPharmacyInfoDao.saveEntity to ensure the saved entity has archived=true.
94-110: Consider adding a not-found test case.The
GetPharmacynested class only tests the happy path. A test for whenmockPharmacyInfoDao.find(eq(id))returnsnullwould verify correct 404 handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/github/carlos_emr/carlos/webserv/rest/PharmacyServiceEndpointTest.java` around lines 94 - 110, Add a negative test in the GetPharmacy nested class that stubs mockPharmacyInfoDao.find(...) to return null for a non-existent id (e.g., when(mockPharmacyInfoDao.find(eq(2))).thenReturn(null)); perform a request().replaceHeader("Accept", "*/*").path("/pharmacies/2").get(); and assert the Response status is 404 to verify PharmacyServiceEndpoint returns Not Found when PharmacyInfo is absent. Use the same test setup style as shouldReturn200_whenPharmacyFound and name the test something like shouldReturn404_whenPharmacyNotFound.src/test/java/io/github/carlos_emr/carlos/webserv/DocumentWsEndpointTest.java (2)
86-92: Clarify relationship betweenregisterMock()andinjectDependency().Both
registerMock()andinjectDependency()are called for the same mocks. IfregisterMock()handles SpringUtils.getBean() resolution andinjectDependency()handles field injection, consider adding a brief comment explaining why both are needed—this will help future maintainers understand the dual registration pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/github/carlos_emr/carlos/webserv/DocumentWsEndpointTest.java` around lines 86 - 92, The test calls both registerMock(...) and injectDependency(...) for the same mocks; add a brief clarifying comment inside setUpMocks explaining that registerMock(DocumentManager.class, documentManager) / registerMock(ProgramManager.class, programManager) registers the mocks with the test Spring/bean resolution helper (so SpringUtils.getBean() will return them) while injectDependency(ws, "documentManager", documentManager) / injectDependency(ws, "programManager", programManager) performs direct field injection into the ws instance (because ws is not managed by the test context), so both steps are required; reference the methods registerMock, injectDependency, setUpMocks, DocumentManager, ProgramManager and ws in the comment.
94-174: All tests are disabled—track resolution.All three nested test classes have
@Disabledtests due to file I/O and internalSpringUtils.getBean()calls. This is appropriate scaffolding, but consider creating a tracking issue to resolve these blockers so the tests can be enabled.Would you like me to open an issue to track enabling these
DocumentWsSOAP endpoint tests?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/github/carlos_emr/carlos/webserv/DocumentWsEndpointTest.java` around lines 94 - 174, Tests in the nested classes GetDocument, GetDocumentsUpdateAfterDate, and GetDocumentsByDemographicIdAfter are all `@Disabled` due to file I/O in DocumentTransfer.toTransfer() and internal SpringUtils.getBean() calls; create a tracking issue to enable these tests and include tasks to (1) refactor DocumentTransfer.toTransfer()/getTransfers() to accept injected dependencies or an InputStream so it no longer reads files from disk (identify usages in DocumentTransfer), (2) remove direct SpringUtils.getBean() calls by introducing injectable beans or factory interfaces so DocumentWsTest can mock them (locate callers inside DocumentTransfer.getTransfers and DocumentWs test helpers), (3) add targeted unit tests that mock file contents and dependencies (e.g., mock documentManager in methods getDocument, getDocumentsUpdateAfterDate, getDocumentsByDemographicIdUpdateAfterDate), and (4) once refactors are in place, re-enable the `@Disabled` tests (remove `@Disabled` annotations from shouldReturnDocumentTransfer_whenFound, shouldReturnDocumentArray_whenResultsExist, shouldReturnEmptyArray_whenNoResults, shouldReturnDocuments_forDemographicAfterDate) and verify they pass.src/test/java/io/github/carlos_emr/carlos/webserv/rest/PersonaServiceEndpointTest.java (1)
144-158: Test name may be misleading—clarify expected behavior.The test is named
shouldReturnError_whenPrivilegeDeniedbut asserts HTTP 200. If the API intentionally returns 200 with an error payload (rather than 403 Forbidden), consider renaming toshouldReturn200WithDeniedFlag_whenPrivilegeDeniedor similar to clarify the expected behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/github/carlos_emr/carlos/webserv/rest/PersonaServiceEndpointTest.java` around lines 144 - 158, The test method shouldReturnError_whenPrivilegeDenied in PersonaServiceEndpointTest is misleading because it asserts HTTP 200; rename the test method and its `@DisplayName` to reflect the actual expected behavior (e.g., shouldReturn200WithDeniedFlag_whenPrivilegeDenied or shouldReturnForbidden_whenPrivilegeDenied if you intend to change behavior), update the method name and the `@DisplayName` accordingly, and ensure any references to the old method name are updated; if you actually want a 403 response instead, change the assertion on response.getStatus() to assert 403 and update the test name/DisplayName to match.src/test/java/io/github/carlos_emr/carlos/webserv/rest/EFormServiceEndpointTest.java (1)
122-135: Mock stub may not match request payload.Line 127 stubs
findByName("Existing Form")with an exact string match, but the JSON payload at line 129 also uses"Existing Form". If the service extracts the name from the JSON differently (e.g., trims whitespace or uses a DTO), the stub may not be triggered. Consider usingany()matcher or verifying the exact extraction logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/github/carlos_emr/carlos/webserv/rest/EFormServiceEndpointTest.java` around lines 122 - 135, The stub for mockEFormDao.findByName uses an exact string which may not match the value extracted by the endpoint; update the test (shouldReturn200WithError_whenNameAlreadyExists) to stub using a matcher (e.g., anyString() or eq(trimmedName)) or otherwise mirror the service extraction logic so the stub is triggered (e.g., when(mockEFormDao.findByName(anyString())).thenReturn(existing)); reference mockEFormDao.findByName and the test's request() call to ensure the mocked behavior aligns with the actual value passed by the endpoint.src/test/java/io/github/carlos_emr/carlos/webserv/rest/NotesServiceEndpointTest.java (1)
99-99: Consider makingObjectMappera static constant.
ObjectMapperis thread-safe and reusable. Making itstatic finalavoids repeated instantiation.♻️ Optional improvement
- private final ObjectMapper objectMapper = new ObjectMapper(); + private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();Then update usages from
objectMappertoOBJECT_MAPPER.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/github/carlos_emr/carlos/webserv/rest/NotesServiceEndpointTest.java` at line 99, The instance field objectMapper should be made a reusable static constant: change the declaration of objectMapper in NotesServiceEndpointTest to a static final ObjectMapper (e.g., OBJECT_MAPPER) since ObjectMapper is thread-safe and expensive to create; then update all usages of objectMapper in the test class to reference OBJECT_MAPPER.src/test/java/io/github/carlos_emr/carlos/test/base/CarlosSoapTestBase.java (2)
129-137: Consider making service address unique per test class to support parallel execution.The default address
local://soap-testcould cause conflicts if tests run in parallel. Consider appending a unique identifier.♻️ Optional: Use unique address per test instance
+ private final String uniqueAddress = "local://soap-test-" + System.nanoTime(); + protected String getServiceAddress() { - return "local://soap-test"; + return uniqueAddress; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/github/carlos_emr/carlos/test/base/CarlosSoapTestBase.java` around lines 129 - 137, The default getServiceAddress() in CarlosSoapTestBase returns a fixed "local://soap-test" which can collide in parallel runs; update getServiceAddress() to return a unique address per test class/instance (e.g., append getClass().getSimpleName() or a short UUID/unique suffix) so each test uses a distinct transport address; modify the implementation of protected String getServiceAddress() in CarlosSoapTestBase to construct and return the unique-local URI (keeping the "local://" prefix) to avoid address conflicts.
150-152: Access static constant directly instead of via instance.
LOGGED_IN_INFO_KEYis a static field onLoggedInInfo. Creating an instance just to access it is unnecessary and misleading.♻️ Proposed fix
- String key = new LoggedInInfo().LOGGED_IN_INFO_KEY; + String key = LoggedInInfo.LOGGED_IN_INFO_KEY; mockServletRequest.setAttribute(key, mockLoggedInInfo); mockSession.setAttribute(key, mockLoggedInInfo);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/github/carlos_emr/carlos/test/base/CarlosSoapTestBase.java` around lines 150 - 152, Replace the instance access to the static constant LOGGED_IN_INFO_KEY by referencing it directly on the class; update the two places where you set attributes (the mockServletRequest.setAttribute(...) and mockSession.setAttribute(...)) to use LoggedInInfo.LOGGED_IN_INFO_KEY instead of new LoggedInInfo().LOGGED_IN_INFO_KEY to avoid creating an unnecessary LoggedInInfo instance.src/test/java/io/github/carlos_emr/carlos/webserv/rest/FormsServiceEndpointTest.java (1)
49-49: Remove unused import.
AbstractSearchResponseis imported but not used in this test class.♻️ Proposed fix
-import io.github.carlos_emr.carlos.webserv.rest.to.AbstractSearchResponse;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/github/carlos_emr/carlos/webserv/rest/FormsServiceEndpointTest.java` at line 49, Remove the unused import AbstractSearchResponse from the test class; open FormsServiceEndpointTest and delete the line importing io.github.carlos_emr.carlos.webserv.rest.to.AbstractSearchResponse so the file no longer contains an unused import.src/test/java/io/github/carlos_emr/carlos/webserv/rest/BillingServiceEndpointTest.java (2)
27-29: Unused import:List.The
java.util.Listimport is not used in this test class.🧹 Suggested fix
import java.util.Collections; -import java.util.List;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/github/carlos_emr/carlos/webserv/rest/BillingServiceEndpointTest.java` around lines 27 - 29, The import java.util.List in BillingServiceEndpointTest is unused; remove the unused import statement (java.util.List) from the top of the file so only java.util.Collections remains, ensuring imports are cleaned up and the test class compiles without warnings.
115-123: Test display name may be misleading.The display name states "should return 200 with error when region not configured" but the test only validates the HTTP status code, not that an error is present in the response body. Consider either:
- Renaming to "should return 200 when region not configured", or
- Adding an assertion to verify the error content in the response body
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/github/carlos_emr/carlos/webserv/rest/BillingServiceEndpointTest.java` around lines 115 - 123, The test method shouldReturn200WithError_whenRegionNotConfigured currently only checks HTTP status and the display name is misleading; update the test to either rename the method/display name to shouldReturn200_whenRegionNotConfigured or add an assertion that validates the response body contains the expected error payload; if choosing to validate the error, read the Response from request().path("/billing/billingRegion").get(), extract the entity/body and assertThat it contains the expected error fields or message (e.g., contains "error" or the specific error text your controller returns) while keeping the existing status assertion and the mock setup on mockCarlosProperties.getProperty("billregion", "").src/test/java/io/github/carlos_emr/carlos/webserv/PreventionWsEndpointTest.java (1)
120-176: Disabled tests documented with clear TODOs.The disabled tests have clear explanations (
PreventionTransfer.getTransfers() calls SpringUtils.getBean() internally) documenting the blocking issue. This is a common pattern in this PR where transfer objects have static dependencies on Spring utilities.Consider consolidating these similar blocking issues across the test suite into a single tracking issue to address the underlying
SpringUtils.getBean()coupling in transfer objects.Would you like me to help create an issue tracking the
SpringUtils.getBean()coupling in transfer objects that blocks multiple endpoint tests?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/github/carlos_emr/carlos/webserv/PreventionWsEndpointTest.java` around lines 120 - 176, Create a single tracking issue to address the static Spring coupling in transfer objects (e.g., PreventionTransfer.getTransfers()) that is blocking multiple endpoint tests (seen in PreventionWsEndpointTest Nested classes GetPreventionsUpdatedAfterDate and GetPreventionsByDemographicIdAfter); in the issue describe the root cause (static SpringUtils.getBean() calls inside transfer methods), list all affected transfer classes and tests (start with PreventionTransfer and the disabled tests in PreventionWsEndpointTest), propose remediation options (remove static SpringUtils usage, inject dependencies, or add a test seam/factory), and add a TODO reference in the disabled tests pointing to the new issue ID so they can be re-enabled once resolved.src/test/java/io/github/carlos_emr/carlos/webserv/ScheduleWsEndpointTest.java (1)
29-32: Unused import:Calendar.The
java.util.Calendarimport is not used in this test class. Consider removing it to keep imports clean.🧹 Suggested fix
import java.util.ArrayList; -import java.util.Calendar; import java.util.Date; import java.util.List;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/github/carlos_emr/carlos/webserv/ScheduleWsEndpointTest.java` around lines 29 - 32, Remove the unused import of java.util.Calendar from the ScheduleWsEndpointTest class; open the file ScheduleWsEndpointTest (look for the class name ScheduleWsEndpointTest) and delete the line importing java.util.Calendar so only the actually used imports (e.g., ArrayList, Date, List) remain.src/test/java/io/github/carlos_emr/carlos/webserv/BookingWsEndpointTest.java (1)
106-139: Consider using consistent argument matchers.In
shouldReturnEmptyBookingTypes_whenNoTypesConfigured, the mock usesanyString()for the provider parameter, whileshouldReturnBookingTypes_forProvideruseseq("001"). For consistency and to ensure the mock is properly targeted, consider usingeq("001")in both tests since the proxy call uses"001".♻️ Suggested consistency improvement
`@Test` `@DisplayName`("should return null or empty booking types when no appointment types configured (JAXB empty array serialization)") void shouldReturnEmptyBookingTypes_whenNoTypesConfigured() { when(appointmentSearchManager.getProviderSearchConfig(anyString())).thenReturn(searchConfig); - when(appointmentSearchManager.getAppointmentTypes(any(SearchConfig.class), anyString())) + when(appointmentSearchManager.getAppointmentTypes(any(SearchConfig.class), eq("001"))) .thenReturn(new ArrayList<>());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/github/carlos_emr/carlos/webserv/BookingWsEndpointTest.java` around lines 106 - 139, Tests use inconsistent Mockito argument matchers: in shouldReturnEmptyBookingTypes_whenNoTypesConfigured the provider parameter is matched with anyString() while the tested call uses "001", causing fragile or unintended stubbing. Update the stubbings in shouldReturnEmptyBookingTypes_whenNoTypesConfigured (the when calls on appointmentSearchManager.getProviderSearchConfig(...) and getAppointmentTypes(...)) to use eq("001") for the provider argument (keep any(SearchConfig.class) for the SearchConfig argument) so the mocks match the actual proxy call; reference the test methods shouldReturnEmptyBookingTypes_whenNoTypesConfigured and shouldReturnBookingTypes_forProvider and the mocked methods getProviderSearchConfig and getAppointmentTypes.src/test/java/io/github/carlos_emr/carlos/webserv/rest/ResourceServiceEndpointTest.java (1)
104-136: All tests are disabled - consider removing or tracking in an issue.Both test methods are
@Disableddue to missinguiResourcesresource bundle. While scaffolding disabled tests can serve as documentation of intended coverage, consider:
- Creating a tracking issue for enabling these tests
- Adding a class-level comment explaining the prerequisite for enabling them
Would you like me to create an issue to track enabling these tests once the
uiResourcesbundle is available in the test classpath?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/github/carlos_emr/carlos/webserv/rest/ResourceServiceEndpointTest.java` around lines 104 - 136, Two nested test cases (classes GetCurrentPreventionRulesVersion and GetCurrentLuCodesVersion with test methods shouldReturn200_withDefaultVersion) are fully disabled due to a missing uiResources resource bundle; create a tracking issue (or JIRA/issue ID) to enable these tests when uiResources is added, then document that ID and the prerequisite in the test source: replace the bare `@Disabled` on each test with an annotated disable message referencing the issue (e.g., `@Disabled`("See ISSUE-123: requires uiResources bundle in test classpath")), and add a short class-level comment above the nested classes explaining the uiResources prerequisite and linking the issue so future maintainers know how/when to re-enable the tests.src/test/java/io/github/carlos_emr/carlos/webserv/rest/InboxServiceEndpointTest.java (1)
30-31: Remove unused import.
MediaTypeis imported but not used.🧹 Suggested fix
-import jakarta.ws.rs.core.MediaType; import jakarta.ws.rs.core.Response;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/github/carlos_emr/carlos/webserv/rest/InboxServiceEndpointTest.java` around lines 30 - 31, Remove the unused import of MediaType from the InboxServiceEndpointTest class: delete the line importing jakarta.ws.rs.core.MediaType so only jakarta.ws.rs.core.Response remains; this cleans up the imports and resolves the unused-import warning in InboxServiceEndpointTest.src/test/java/io/github/carlos_emr/carlos/webserv/rest/DemographicServiceEndpointTest.java (1)
33-34: Remove unused imports.
MediaType(line 33) andOscarSearchResponse(line 58) are imported but never used in this test class.🧹 Suggested cleanup
-import jakarta.ws.rs.core.MediaType; import jakarta.ws.rs.core.Response;import io.github.carlos_emr.carlos.webserv.rest.to.OscarSearchResponse; -import io.github.carlos_emr.carlos.webserv.rest.to.model.DemographicSearchResult; +import io.github.carlos_emr.carlos.webserv.rest.to.model.DemographicSearchResult;Actually, only remove
OscarSearchResponse:-import io.github.carlos_emr.carlos.webserv.rest.to.OscarSearchResponse;Also applies to: 58-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/github/carlos_emr/carlos/webserv/rest/DemographicServiceEndpointTest.java` around lines 33 - 34, The test class DemographicServiceEndpointTest has an unused import for OscarSearchResponse; remove the import statement that references OscarSearchResponse so the class no longer imports that unused type, but leave the existing MediaType import in place since it is used elsewhere in the file; update any import formatting if needed to keep imports tidy.src/test/java/io/github/carlos_emr/carlos/webserv/rest/RxWebServiceEndpointTest.java (2)
32-33: Remove unused import.
Entityfromjakarta.ws.rs.clientis imported but not used in this test class.🧹 Suggested fix
-import jakarta.ws.rs.client.Entity; import jakarta.ws.rs.core.Response;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/github/carlos_emr/carlos/webserv/rest/RxWebServiceEndpointTest.java` around lines 32 - 33, The import jakarta.ws.rs.client.Entity is unused in the RxWebServiceEndpointTest class; remove the unused import statement (the line importing Entity) from the top of RxWebServiceEndpointTest so only needed imports like jakarta.ws.rs.core.Response remain, and run a quick rebuild to ensure no remaining references to Entity in that test class.
122-122: Remove unnecessarythrows Exceptionclauses.These test methods don't actually throw checked exceptions. The
throws Exceptiondeclarations are unnecessary.🧹 Example fix for one method
-void shouldReturn200_whenDrugsExist() throws Exception { +void shouldReturn200_whenDrugsExist() {Also applies to: 142-142, 160-160, 180-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/github/carlos_emr/carlos/webserv/rest/RxWebServiceEndpointTest.java` at line 122, Remove the unnecessary "throws Exception" declarations from the test method signatures in RxWebServiceEndpointTest (e.g., shouldReturn200_whenDrugsExist() and the other test methods flagged at the same file). Edit each method signature to delete the "throws Exception" clause (e.g., change "void shouldReturn200_whenDrugsExist() throws Exception" to "void shouldReturn200_whenDrugsExist()"); if any method actually uses checked exceptions, handle them locally or declare the specific checked exception instead.src/test/java/io/github/carlos_emr/carlos/webserv/PrescriptionWsEndpointTest.java (1)
70-87: Consider consolidating mock injection intogetServiceBean()for consistency.The current pattern of storing
wsas an instance variable and injecting dependencies in@BeforeEachworks correctly but differs fromProgramWsEndpointTest, which consolidates mock injection ingetServiceBean(). This refactoring is safe becauseCarlosSoapTestBase.setUpSoapEndpoint()callsMockitoAnnotations.openMocks(this)before invokinggetServiceBean(), ensuring@Mockfields are initialized beforehand.♻️ Alternative approach for consistency
- private PrescriptionWs ws; - `@Override` protected Object getServiceBean() { - ws = new PrescriptionWs(); - return ws; + PrescriptionWs ws = new PrescriptionWs(); + registerMock(PrescriptionManager.class, prescriptionManager); + injectDependency(ws, "prescriptionManager", prescriptionManager); + return ws; } - - `@BeforeEach` - void setUpMocks() { - registerMock(PrescriptionManager.class, prescriptionManager); - injectDependency(ws, "prescriptionManager", prescriptionManager); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/github/carlos_emr/carlos/webserv/PrescriptionWsEndpointTest.java` around lines 70 - 87, Move the mock registration and dependency injection into getServiceBean(): create the PrescriptionWs instance inside getServiceBean(), call registerMock(PrescriptionManager.class, prescriptionManager) and injectDependency(ws, "prescriptionManager", prescriptionManager) there (remove the separate `@BeforeEach` setUpMocks and the ws instance field or make ws a local variable returned by getServiceBean()); keep getServiceInterface() returning PrescriptionWs.class and ensure you reference PrescriptionWs, getServiceBean(), prescriptionManager, registerMock and injectDependency when making the change.src/test/java/io/github/carlos_emr/carlos/webserv/rest/ProviderServiceEndpointTest.java (1)
102-105: Assert response payloads in addition to HTTP status.These tests currently pass even if payload mapping/serialization is wrong. Please assert key body fields (or list size) for representative happy paths to validate the endpoint contract end-to-end.
Example tightening for one test
Response response = request().path("/providerService/providers_json").get(); assertThat(response.getStatus()).isEqualTo(200); +List<?> providers = response.readEntity(List.class); +assertThat(providers).hasSize(1);Also applies to: 129-132, 156-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/github/carlos_emr/carlos/webserv/rest/ProviderServiceEndpointTest.java` around lines 102 - 105, The test in ProviderServiceEndpointTest that calls request().path("/providerService/providers_json").get() only asserts HTTP 200; parse the response payload (e.g., read entity as JSON/JsonNode or List<Map>) from the response variable and add assertions on the body such as expected list size and key fields (e.g., first element has "id", "name" or other domain-specific fields) to validate serialization and mapping; apply the same tightening to the other tests in this class that perform similar GETs to providerService (the other occurrences around the same tests) so they assert both status and representative payload contents.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/test/endpoint-testing-guide.md`:
- Around line 174-180: The fenced code block showing the test class hierarchy is
missing a language specifier (triggers MD040); update the triple-backtick fence
for that block in docs/test/endpoint-testing-guide.md to use a language hint
(e.g., ```text) so markdownlint passes; locate the block containing
"CarlosUnitTestBase ... CarlosRestTestBase ... CarlosSoapTestBase" and replace
the opening ``` with ```text.
In `@src/test/java/io/github/carlos_emr/carlos/test/base/CarlosRestTestBase.java`:
- Around line 166-168: Replace the instance access new
LoggedInInfo().LOGGED_IN_INFO_KEY with a static reference to the constant: use
LoggedInInfo.LOGGED_IN_INFO_KEY when calling
mockServletRequest.setAttribute(...) and mockSession.setAttribute(...); update
the two places where the key is retrieved so they reference the static member on
the LoggedInInfo class rather than creating a new LoggedInInfo object.
In `@src/test/java/io/github/carlos_emr/carlos/webserv/LoginWsEndpointTest.java`:
- Around line 100-103: The test method
LoginWsEndpointTest.shouldReturnLoginResult_whenCredentialsValid is disabled
because WsUtils' static initializer calls SpringUtils.getBean(ProviderDao.class)
which Mockito cannot instrument; either (A) make the test executable by removing
the `@Disabled` and preventing the static init from invoking SpringUtils (e.g.,
refactor WsUtils to allow injection or add a test-only hook) or mock the static
call in test setup (use Mockito.mockStatic for SpringUtils.getBean or a
supported static-mocking tool) so the test can run, ensuring any required
ProviderDao or related beans are supplied as test doubles; or (B) if you cannot
make that change in this PR, re-enable the test later by moving this test to a
tracked follow-up and remove or add the class LoginWsEndpointTest to the
test-excludes so the suite has runtime coverage now. Ensure references to
WsUtils, SpringUtils.getBean(ProviderDao.class), and the method
shouldReturnLoginResult_whenCredentialsValid are addressed.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/EFormServiceEndpointTest.java`:
- Around line 94-102: The implementation currently returns
RestResponse.errorResponse() from EFormService.loadEForm(), which JAX-RS
serializes as HTTP 200; change EFormService.loadEForm() to return a JAX-RS
Response (or the project response wrapper that allows setting status) with
Response.status(404) and the error payload when the eForm is not found instead
of RestResponse.errorResponse(), and then update the test
shouldReturn200WithError_whenEFormNotFound() to expect 404 and assert the error
body contents accordingly; references: EFormService.loadEForm(),
RestResponse.errorResponse(), and the test method
shouldReturn200WithError_whenEFormNotFound().
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/LabServiceEndpointTest.java`:
- Around line 83-88: The helper method createTestHl7Message(int id) declares an
unused id parameter; either remove the parameter from the method signature and
all call sites, or assign the id to the message before returning (e.g., call the
appropriate setter on Hl7TextMessage such as msg.setId(id) or
msg.setMessageId(String.valueOf(id)) depending on the class API) so the id is
actually used; update any tests that call createTestHl7Message to match the
chosen change.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/PatientDetailStatusServiceEndpointTest.java`:
- Around line 98-126: The tests should assert the response body to distinguish
unique vs non-unique HIN rather than only HTTP 200: update
shouldReturn200Success_whenHinIsUnique and shouldReturn200Error_whenHinNotUnique
to parse the Response entity returned by the request to
/patientDetailStatusService/isUniqueHC and assert the expected contract (e.g.,
JSON boolean flag, status field, or error message) produced by the endpoint;
locate the call to mockDemographicManager.searchByHealthCard and the Demographic
demo1 setup to keep mocks the same, then add assertions on the response entity
(for example verifying a "unique": true for the unique case and "unique": false
or an error message for the non-unique case) so each branch’s behavior is
verified.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/ScheduleServiceEndpointTest.java`:
- Around line 157-168: The test method should not remain disabled in the
committed suite; either make it runnable by mocking the
NewAppointmentConverter/SpringUtils.getBean interaction or remove/track it as a
follow-up. To fix, choose one: (A) Enable the test by adding setup in
ScheduleServiceEndpointTest's `@BeforeEach` to mock SpringUtils.getBean (using
Mockito's mockStatic or a test-friendly wrapper) to return a mocked
NewAppointmentConverter and configure that mock's conversion/behavior so
request().path("/schedule/add").post(json) succeeds, or (B) remove the `@Disabled`
test method and instead create a short TODO comment and link to a tracked
follow-up issue for implementing NewAppointmentConverter/SpringUtils.getBean
mocks; ensure the test method name should be preserved
(shouldReturn200_whenAppointmentAdded) if re-adding later.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/StatusServiceEndpointTest.java`:
- Around line 58-66: The test should assert the response payload contains the
authenticated provider number in addition to the 200 status: after calling
request().path("/status/checkIfAuthed").get() (in
shouldReturn200WithProviderNo_whenAuthenticated) read the response entity as a
String (or JSON) and assert it contains the mocked provider value returned by
mockLoggedInInfo.getLoggedInProviderNo() ("999998"); update the assertion to
check the response body includes that provider number (or specific JSON field if
the endpoint returns structured JSON) so the test fails if the provider data is
omitted.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/TicklerWebServiceEndpointTest.java`:
- Around line 107-121: The test
shouldReturn200WithTicklers_whenSearchReturnsResults is stubbing
mockTicklerManager.getTicklers to return Collections.emptyList(), so it never
exercises the non-empty serialization path; update the stub to return a
meaningful non-empty List<Tickler> (e.g. a single Tickler with known fields),
post the same request, then assert response.getStatus() == 200 and that the
TicklerResponse returned by response.readEntity(TicklerResponse.class) contains
the expected item(s) and field values; make the same change for the other
positive test that currently returns empty (the second test referenced around
the 149-160 region) so both positive-path cases validate actual non-empty
payload serialization.
---
Nitpick comments:
In `@src/test/java/io/github/carlos_emr/carlos/test/base/CarlosSoapTestBase.java`:
- Around line 129-137: The default getServiceAddress() in CarlosSoapTestBase
returns a fixed "local://soap-test" which can collide in parallel runs; update
getServiceAddress() to return a unique address per test class/instance (e.g.,
append getClass().getSimpleName() or a short UUID/unique suffix) so each test
uses a distinct transport address; modify the implementation of protected String
getServiceAddress() in CarlosSoapTestBase to construct and return the
unique-local URI (keeping the "local://" prefix) to avoid address conflicts.
- Around line 150-152: Replace the instance access to the static constant
LOGGED_IN_INFO_KEY by referencing it directly on the class; update the two
places where you set attributes (the mockServletRequest.setAttribute(...) and
mockSession.setAttribute(...)) to use LoggedInInfo.LOGGED_IN_INFO_KEY instead of
new LoggedInInfo().LOGGED_IN_INFO_KEY to avoid creating an unnecessary
LoggedInInfo instance.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/BookingWsEndpointTest.java`:
- Around line 106-139: Tests use inconsistent Mockito argument matchers: in
shouldReturnEmptyBookingTypes_whenNoTypesConfigured the provider parameter is
matched with anyString() while the tested call uses "001", causing fragile or
unintended stubbing. Update the stubbings in
shouldReturnEmptyBookingTypes_whenNoTypesConfigured (the when calls on
appointmentSearchManager.getProviderSearchConfig(...) and
getAppointmentTypes(...)) to use eq("001") for the provider argument (keep
any(SearchConfig.class) for the SearchConfig argument) so the mocks match the
actual proxy call; reference the test methods
shouldReturnEmptyBookingTypes_whenNoTypesConfigured and
shouldReturnBookingTypes_forProvider and the mocked methods
getProviderSearchConfig and getAppointmentTypes.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/DocumentWsEndpointTest.java`:
- Around line 86-92: The test calls both registerMock(...) and
injectDependency(...) for the same mocks; add a brief clarifying comment inside
setUpMocks explaining that registerMock(DocumentManager.class, documentManager)
/ registerMock(ProgramManager.class, programManager) registers the mocks with
the test Spring/bean resolution helper (so SpringUtils.getBean() will return
them) while injectDependency(ws, "documentManager", documentManager) /
injectDependency(ws, "programManager", programManager) performs direct field
injection into the ws instance (because ws is not managed by the test context),
so both steps are required; reference the methods registerMock,
injectDependency, setUpMocks, DocumentManager, ProgramManager and ws in the
comment.
- Around line 94-174: Tests in the nested classes GetDocument,
GetDocumentsUpdateAfterDate, and GetDocumentsByDemographicIdAfter are all
`@Disabled` due to file I/O in DocumentTransfer.toTransfer() and internal
SpringUtils.getBean() calls; create a tracking issue to enable these tests and
include tasks to (1) refactor DocumentTransfer.toTransfer()/getTransfers() to
accept injected dependencies or an InputStream so it no longer reads files from
disk (identify usages in DocumentTransfer), (2) remove direct
SpringUtils.getBean() calls by introducing injectable beans or factory
interfaces so DocumentWsTest can mock them (locate callers inside
DocumentTransfer.getTransfers and DocumentWs test helpers), (3) add targeted
unit tests that mock file contents and dependencies (e.g., mock documentManager
in methods getDocument, getDocumentsUpdateAfterDate,
getDocumentsByDemographicIdUpdateAfterDate), and (4) once refactors are in
place, re-enable the `@Disabled` tests (remove `@Disabled` annotations from
shouldReturnDocumentTransfer_whenFound,
shouldReturnDocumentArray_whenResultsExist,
shouldReturnEmptyArray_whenNoResults,
shouldReturnDocuments_forDemographicAfterDate) and verify they pass.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/PrescriptionWsEndpointTest.java`:
- Around line 70-87: Move the mock registration and dependency injection into
getServiceBean(): create the PrescriptionWs instance inside getServiceBean(),
call registerMock(PrescriptionManager.class, prescriptionManager) and
injectDependency(ws, "prescriptionManager", prescriptionManager) there (remove
the separate `@BeforeEach` setUpMocks and the ws instance field or make ws a local
variable returned by getServiceBean()); keep getServiceInterface() returning
PrescriptionWs.class and ensure you reference PrescriptionWs, getServiceBean(),
prescriptionManager, registerMock and injectDependency when making the change.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/PreventionWsEndpointTest.java`:
- Around line 120-176: Create a single tracking issue to address the static
Spring coupling in transfer objects (e.g., PreventionTransfer.getTransfers())
that is blocking multiple endpoint tests (seen in PreventionWsEndpointTest
Nested classes GetPreventionsUpdatedAfterDate and
GetPreventionsByDemographicIdAfter); in the issue describe the root cause
(static SpringUtils.getBean() calls inside transfer methods), list all affected
transfer classes and tests (start with PreventionTransfer and the disabled tests
in PreventionWsEndpointTest), propose remediation options (remove static
SpringUtils usage, inject dependencies, or add a test seam/factory), and add a
TODO reference in the disabled tests pointing to the new issue ID so they can be
re-enabled once resolved.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/AppServiceEndpointTest.java`:
- Around line 69-90: Update the two tests in AppServiceEndpointTest
(shouldReturn200_whenAppsExist and shouldReturn200WithEmptyList_whenNoAppsExist)
to assert the response entity in addition to status: after calling
request().path("/app/getApps/").get() use Response.readEntity(...) or equivalent
JSON deserialization to obtain the returned list and assert its contents (for
shouldReturn200_whenAppsExist assert the list size is 1 and/or contains an
AppDefinitionTo1 instance matching the mocked app; for
shouldReturn200WithEmptyList_whenNoAppsExist assert the list is empty). Also
ensure you still verify
mockAppManager.getAppDefinitions(any(LoggedInInfo.class)) is used as part of the
setup.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/BillingServiceEndpointTest.java`:
- Around line 27-29: The import java.util.List in BillingServiceEndpointTest is
unused; remove the unused import statement (java.util.List) from the top of the
file so only java.util.Collections remains, ensuring imports are cleaned up and
the test class compiles without warnings.
- Around line 115-123: The test method
shouldReturn200WithError_whenRegionNotConfigured currently only checks HTTP
status and the display name is misleading; update the test to either rename the
method/display name to shouldReturn200_whenRegionNotConfigured or add an
assertion that validates the response body contains the expected error payload;
if choosing to validate the error, read the Response from
request().path("/billing/billingRegion").get(), extract the entity/body and
assertThat it contains the expected error fields or message (e.g., contains
"error" or the specific error text your controller returns) while keeping the
existing status assertion and the mock setup on
mockCarlosProperties.getProperty("billregion", "").
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/DemographicServiceEndpointTest.java`:
- Around line 33-34: The test class DemographicServiceEndpointTest has an unused
import for OscarSearchResponse; remove the import statement that references
OscarSearchResponse so the class no longer imports that unused type, but leave
the existing MediaType import in place since it is used elsewhere in the file;
update any import formatting if needed to keep imports tidy.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/EFormServiceEndpointTest.java`:
- Around line 122-135: The stub for mockEFormDao.findByName uses an exact string
which may not match the value extracted by the endpoint; update the test
(shouldReturn200WithError_whenNameAlreadyExists) to stub using a matcher (e.g.,
anyString() or eq(trimmedName)) or otherwise mirror the service extraction logic
so the stub is triggered (e.g.,
when(mockEFormDao.findByName(anyString())).thenReturn(existing)); reference
mockEFormDao.findByName and the test's request() call to ensure the mocked
behavior aligns with the actual value passed by the endpoint.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/FormsServiceEndpointTest.java`:
- Line 49: Remove the unused import AbstractSearchResponse from the test class;
open FormsServiceEndpointTest and delete the line importing
io.github.carlos_emr.carlos.webserv.rest.to.AbstractSearchResponse so the file
no longer contains an unused import.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/InboxServiceEndpointTest.java`:
- Around line 30-31: Remove the unused import of MediaType from the
InboxServiceEndpointTest class: delete the line importing
jakarta.ws.rs.core.MediaType so only jakarta.ws.rs.core.Response remains; this
cleans up the imports and resolves the unused-import warning in
InboxServiceEndpointTest.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/NotesServiceEndpointTest.java`:
- Line 99: The instance field objectMapper should be made a reusable static
constant: change the declaration of objectMapper in NotesServiceEndpointTest to
a static final ObjectMapper (e.g., OBJECT_MAPPER) since ObjectMapper is
thread-safe and expensive to create; then update all usages of objectMapper in
the test class to reference OBJECT_MAPPER.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/PersonaServiceEndpointTest.java`:
- Around line 144-158: The test method shouldReturnError_whenPrivilegeDenied in
PersonaServiceEndpointTest is misleading because it asserts HTTP 200; rename the
test method and its `@DisplayName` to reflect the actual expected behavior (e.g.,
shouldReturn200WithDeniedFlag_whenPrivilegeDenied or
shouldReturnForbidden_whenPrivilegeDenied if you intend to change behavior),
update the method name and the `@DisplayName` accordingly, and ensure any
references to the old method name are updated; if you actually want a 403
response instead, change the assertion on response.getStatus() to assert 403 and
update the test name/DisplayName to match.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/PharmacyServiceEndpointTest.java`:
- Around line 116-128: Update the test method
shouldReturn200_whenPharmacySoftDeleted to assert the soft-delete actually
occurred: after invoking the DELETE request, verify that PharmacyInfo was marked
archived (e.g., pharmacy.getArchived()/isArchived() is true) and that
mockPharmacyInfoDao.saveEntity(...) was called with that archived PharmacyInfo;
you can either assert the in-memory pharmacy object's archived flag or use an
ArgumentCaptor/verify on mockPharmacyInfoDao.saveEntity to ensure the saved
entity has archived=true.
- Around line 94-110: Add a negative test in the GetPharmacy nested class that
stubs mockPharmacyInfoDao.find(...) to return null for a non-existent id (e.g.,
when(mockPharmacyInfoDao.find(eq(2))).thenReturn(null)); perform a
request().replaceHeader("Accept", "*/*").path("/pharmacies/2").get(); and assert
the Response status is 404 to verify PharmacyServiceEndpoint returns Not Found
when PharmacyInfo is absent. Use the same test setup style as
shouldReturn200_whenPharmacyFound and name the test something like
shouldReturn404_whenPharmacyNotFound.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/ProviderServiceEndpointTest.java`:
- Around line 102-105: The test in ProviderServiceEndpointTest that calls
request().path("/providerService/providers_json").get() only asserts HTTP 200;
parse the response payload (e.g., read entity as JSON/JsonNode or List<Map>)
from the response variable and add assertions on the body such as expected list
size and key fields (e.g., first element has "id", "name" or other
domain-specific fields) to validate serialization and mapping; apply the same
tightening to the other tests in this class that perform similar GETs to
providerService (the other occurrences around the same tests) so they assert
both status and representative payload contents.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/ResourceServiceEndpointTest.java`:
- Around line 104-136: Two nested test cases (classes
GetCurrentPreventionRulesVersion and GetCurrentLuCodesVersion with test methods
shouldReturn200_withDefaultVersion) are fully disabled due to a missing
uiResources resource bundle; create a tracking issue (or JIRA/issue ID) to
enable these tests when uiResources is added, then document that ID and the
prerequisite in the test source: replace the bare `@Disabled` on each test with an
annotated disable message referencing the issue (e.g., `@Disabled`("See ISSUE-123:
requires uiResources bundle in test classpath")), and add a short class-level
comment above the nested classes explaining the uiResources prerequisite and
linking the issue so future maintainers know how/when to re-enable the tests.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/RxWebServiceEndpointTest.java`:
- Around line 32-33: The import jakarta.ws.rs.client.Entity is unused in the
RxWebServiceEndpointTest class; remove the unused import statement (the line
importing Entity) from the top of RxWebServiceEndpointTest so only needed
imports like jakarta.ws.rs.core.Response remain, and run a quick rebuild to
ensure no remaining references to Entity in that test class.
- Line 122: Remove the unnecessary "throws Exception" declarations from the test
method signatures in RxWebServiceEndpointTest (e.g.,
shouldReturn200_whenDrugsExist() and the other test methods flagged at the same
file). Edit each method signature to delete the "throws Exception" clause (e.g.,
change "void shouldReturn200_whenDrugsExist() throws Exception" to "void
shouldReturn200_whenDrugsExist()"); if any method actually uses checked
exceptions, handle them locally or declare the specific checked exception
instead.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/ScheduleWsEndpointTest.java`:
- Around line 29-32: Remove the unused import of java.util.Calendar from the
ScheduleWsEndpointTest class; open the file ScheduleWsEndpointTest (look for the
class name ScheduleWsEndpointTest) and delete the line importing
java.util.Calendar so only the actually used imports (e.g., ArrayList, Date,
List) remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd6563d5-174d-420b-9559-36479b984d5e
📒 Files selected for processing (53)
CLAUDE.mddependencies-lock.jsondocs/test/endpoint-testing-guide.mdpom.xmlsrc/test/java/io/github/carlos_emr/carlos/test/base/CarlosRestTestBase.javasrc/test/java/io/github/carlos_emr/carlos/test/base/CarlosSoapTestBase.javasrc/test/java/io/github/carlos_emr/carlos/webserv/AllergyWsEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/BookingWsEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/DemographicWsEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/DocumentWsEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/FacilityWsEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/LabUploadWsEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/LoginWsEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/MeasurementWsEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/PrescriptionWsEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/PreventionWsEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/ProgramWsEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/ProviderWsEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/ScheduleWsEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/SystemInfoWsEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/AllergyServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/AppServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/BillingServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/ConsentServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/ConsultationWebServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/DemographicMergeServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/DemographicServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/DiseaseRegistryServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/DocumentServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/EFormServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/EFormsServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/FormsServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/InboxServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/LabServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/MeasurementServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/MessagingServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/NotesServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/OscarJobServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/PatientDetailStatusServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/PersonaServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/PharmacyServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/PreventionServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/ProductDispensingServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/ProgramServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/ProviderServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/RecordUxServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/ReportingServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/ResourceServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/RxLookupServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/RxWebServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/ScheduleServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/StatusServiceEndpointTest.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/TicklerWebServiceEndpointTest.java
| ``` | ||
| CarlosUnitTestBase (SpringUtils mocking, no Spring context) | ||
| ├── CarlosRestTestBase (CXF JAX-RS server + WebClient) | ||
| │ └── *EndpointTest.java (REST endpoint tests) | ||
| └── CarlosSoapTestBase (CXF JAX-WS server + proxy client) | ||
| └── *EndpointTest.java (SOAP endpoint tests) | ||
| ``` |
There was a problem hiding this comment.
Add a language hint to the fenced code block.
This block is missing a language specifier and triggers markdownlint MD040.
Suggested patch
-```
+```text
CarlosUnitTestBase (SpringUtils mocking, no Spring context)
├── CarlosRestTestBase (CXF JAX-RS server + WebClient)
│ └── *EndpointTest.java (REST endpoint tests)
└── CarlosSoapTestBase (CXF JAX-WS server + proxy client)
└── *EndpointTest.java (SOAP endpoint tests)</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 174-174: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/test/endpoint-testing-guide.md` around lines 174 - 180, The fenced code
block showing the test class hierarchy is missing a language specifier (triggers
MD040); update the triple-backtick fence for that block in
docs/test/endpoint-testing-guide.md to use a language hint (e.g., ```text) so
markdownlint passes; locate the block containing "CarlosUnitTestBase ...
CarlosRestTestBase ... CarlosSoapTestBase" and replace the opening ``` with
```text.
| String key = new LoggedInInfo().LOGGED_IN_INFO_KEY; | ||
| mockServletRequest.setAttribute(key, mockLoggedInInfo); | ||
| mockSession.setAttribute(key, mockLoggedInInfo); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use static access for constant LOGGED_IN_INFO_KEY.
Creating a new LoggedInInfo instance just to access a static constant is unnecessary and potentially confusing. Access the constant statically.
🔧 Suggested fix
- String key = new LoggedInInfo().LOGGED_IN_INFO_KEY;
+ String key = LoggedInInfo.LOGGED_IN_INFO_KEY;
mockServletRequest.setAttribute(key, mockLoggedInInfo);
mockSession.setAttribute(key, mockLoggedInInfo);📝 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.
| String key = new LoggedInInfo().LOGGED_IN_INFO_KEY; | |
| mockServletRequest.setAttribute(key, mockLoggedInInfo); | |
| mockSession.setAttribute(key, mockLoggedInInfo); | |
| String key = LoggedInInfo.LOGGED_IN_INFO_KEY; | |
| mockServletRequest.setAttribute(key, mockLoggedInInfo); | |
| mockSession.setAttribute(key, mockLoggedInInfo); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/io/github/carlos_emr/carlos/test/base/CarlosRestTestBase.java`
around lines 166 - 168, Replace the instance access new
LoggedInInfo().LOGGED_IN_INFO_KEY with a static reference to the constant: use
LoggedInInfo.LOGGED_IN_INFO_KEY when calling
mockServletRequest.setAttribute(...) and mockSession.setAttribute(...); update
the two places where the key is retrieved so they reference the static member on
the LoggedInInfo class rather than creating a new LoggedInInfo object.
| @Test | ||
| @Disabled("TODO: WsUtils static initializer calls SpringUtils.getBean(ProviderDao.class), cannot be instrumented by Mockito") | ||
| @DisplayName("should return login result with security token on valid login") | ||
| void shouldReturnLoginResult_whenCredentialsValid() throws NotAuthorisedException { |
There was a problem hiding this comment.
The only endpoint test is disabled, so this suite currently provides no runtime coverage.
Please either make this test executable in this PR or move it to a tracked follow-up with this class excluded until enabled.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/io/github/carlos_emr/carlos/webserv/LoginWsEndpointTest.java`
around lines 100 - 103, The test method
LoginWsEndpointTest.shouldReturnLoginResult_whenCredentialsValid is disabled
because WsUtils' static initializer calls SpringUtils.getBean(ProviderDao.class)
which Mockito cannot instrument; either (A) make the test executable by removing
the `@Disabled` and preventing the static init from invoking SpringUtils (e.g.,
refactor WsUtils to allow injection or add a test-only hook) or mock the static
call in test setup (use Mockito.mockStatic for SpringUtils.getBean or a
supported static-mocking tool) so the test can run, ensuring any required
ProviderDao or related beans are supplied as test doubles; or (B) if you cannot
make that change in this PR, re-enable the test later by moving this test to a
tracked follow-up and remove or add the class LoginWsEndpointTest to the
test-excludes so the suite has runtime coverage now. Ensure references to
WsUtils, SpringUtils.getBean(ProviderDao.class), and the method
shouldReturnLoginResult_whenCredentialsValid are addressed.
| @Test | ||
| @DisplayName("should return 200 with error when eForm not found") | ||
| void shouldReturn200WithError_whenEFormNotFound() { | ||
| when(mockEFormDao.findById(eq(999))).thenReturn(null); | ||
|
|
||
| Response response = request().path("/eform/999").get(); | ||
|
|
||
| assertThat(response.getStatus()).isEqualTo(200); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify what EFormService returns when eForm is not found
ast-grep --pattern $'@GET
$$$
public $_ $_($$$) {
$$$
return $_;
}'
# Search for the actual implementation to confirm response behavior
rg -n -A 10 'findById.*null' --type=java -g '*EFormService.java'Repository: carlos-emr/carlos
Length of output: 43
🏁 Script executed:
fd -type f -name '*EFormService*.java' | head -20Repository: carlos-emr/carlos
Length of output: 231
🏁 Script executed:
fd -type f -name '*EForm*.java' | grep -E '(Service|Endpoint)' | head -20Repository: carlos-emr/carlos
Length of output: 231
🏁 Script executed:
rg -l 'EForm.*Service|EForm.*Endpoint' --type=java | head -20Repository: carlos-emr/carlos
Length of output: 541
🏁 Script executed:
cat -n src/main/java/io/github/carlos_emr/carlos/webserv/rest/EFormService.java | head -150Repository: carlos-emr/carlos
Length of output: 6985
🏁 Script executed:
rg -n -A 15 'GET.*eform.*dataId|@GetMapping' src/main/java/io/github/carlos_emr/carlos/webserv/rest/EFormService.javaRepository: carlos-emr/carlos
Length of output: 845
🏁 Script executed:
fd -name 'RestResponse.java' | head -5Repository: carlos-emr/carlos
Length of output: 291
🏁 Script executed:
rg -n 'class RestResponse|errorResponse' --type=java -A 5 | grep -A 5 'errorResponse'Repository: carlos-emr/carlos
Length of output: 42012
🏁 Script executed:
rg -n 'eform == null|findById.*null' src/main/java/io/github/carlos_emr/carlos/webserv/rest --type=java -A 3Repository: carlos-emr/carlos
Length of output: 503
🏁 Script executed:
fd -name '*.java' -path '*/webserv/rest/*Service.java' | head -5 | xargs grep -l 'findById' | xargs grep -A 5 'findById.*null'Repository: carlos-emr/carlos
Length of output: 293
🏁 Script executed:
fd -name 'ResponseStatus.java' -o -name 'RestResponse.java' | xargs cat -n | head -200Repository: carlos-emr/carlos
Length of output: 291
🏁 Script executed:
rg -n 'ResponseStatus|HTTP|getStatus|Status.*200|Status.*404' src/main/java/io/github/carlos_emr/carlos/webserv/rest/to/RestResponse.java -A 2 -B 2Repository: carlos-emr/carlos
Length of output: 3309
🏁 Script executed:
rg -l 'RestResponse' src/main/java/io/github/carlos_emr/carlos/webserv/rest/to --type=java | head -5Repository: carlos-emr/carlos
Length of output: 383
🏁 Script executed:
cat src/main/java/io/github/carlos_emr/carlos/webserv/rest/to/RestResponse.java | head -80Repository: carlos-emr/carlos
Length of output: 3523
🏁 Script executed:
cat src/main/java/io/github/carlos_emr/carlos/webserv/rest/to/GenericRestResponse.java | head -150Repository: carlos-emr/carlos
Length of output: 5236
🏁 Script executed:
rg -n 'getStatus|Response.status|Status.OK|Status.ERROR' src/main/java/io/github/carlos_emr/carlos/webserv/rest/to --type=java -B 2 -A 2Repository: carlos-emr/carlos
Length of output: 20394
🏁 Script executed:
cat src/test/java/io/github/carlos_emr/carlos/webserv/rest/EFormServiceEndpointTest.java | head -110Repository: carlos-emr/carlos
Length of output: 3851
🏁 Script executed:
rg -n 'request\(\)|Response.ok|Response.status' src/test/java/io/github/carlos_emr/carlos/test/base/CarlosRestTestBase.java -B 2 -A 3 | head -50Repository: carlos-emr/carlos
Length of output: 254
🏁 Script executed:
rg -n 'class CarlosRestTestBase|extends|request()' src/test/java/io/github/carlos_emr/carlos/test/base/CarlosRestTestBase.java | head -10Repository: carlos-emr/carlos
Length of output: 701
🏁 Script executed:
rg -n 'ExceptionMapper|ResponseWriter|MessageBodyWriter' src/main/java/io/github/carlos_emr/carlos/webserv/rest --type=java | head -20Repository: carlos-emr/carlos
Length of output: 43
🏁 Script executed:
fd -name '*Provider.java' -o -name '*Mapper.java' | grep -i response | head -10Repository: carlos-emr/carlos
Length of output: 291
🏁 Script executed:
rg -n 'Response.ok|Response.status|@Produces|200|404' src/main/java/io/github/carlos_emr/carlos/webserv/rest/EFormService.javaRepository: carlos-emr/carlos
Length of output: 274
🏁 Script executed:
git log --oneline -n 5 -- src/main/java/io/github/carlos_emr/carlos/webserv/rest/EFormService.javaRepository: carlos-emr/carlos
Length of output: 92
The API implementation violates REST guidelines by returning HTTP 200 with an error response when eForm is not found; it should return 404.
The test correctly validates the current (albeit non-standard) API behavior. However, the underlying EFormService.loadEForm() method returns RestResponse.errorResponse() for not-found cases, which JAX-RS serializes as HTTP 200 with a JSON error body. Per the REST API guidelines (return 404 for not found), the implementation should use a response wrapper that sets the HTTP status to 404. Once the API is corrected, this test assertion should also be updated to expect 404 and validate the error response body.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/EFormServiceEndpointTest.java`
around lines 94 - 102, The implementation currently returns
RestResponse.errorResponse() from EFormService.loadEForm(), which JAX-RS
serializes as HTTP 200; change EFormService.loadEForm() to return a JAX-RS
Response (or the project response wrapper that allows setting status) with
Response.status(404) and the error payload when the eForm is not found instead
of RestResponse.errorResponse(), and then update the test
shouldReturn200WithError_whenEFormNotFound() to expect 404 and assert the error
body contents accordingly; references: EFormService.loadEForm(),
RestResponse.errorResponse(), and the test method
shouldReturn200WithError_whenEFormNotFound().
| private Hl7TextMessage createTestHl7Message(int id) { | ||
| Hl7TextMessage msg = new Hl7TextMessage(); | ||
| msg.setType("HL7"); | ||
| msg.setServiceName("TestLab"); | ||
| return msg; | ||
| } |
There was a problem hiding this comment.
Unused id parameter in helper method.
The createTestHl7Message(int id) method accepts an id parameter but never uses it. Either remove the parameter or set it on the message object.
💡 Suggested fix
- private Hl7TextMessage createTestHl7Message(int id) {
+ private Hl7TextMessage createTestHl7Message() {
Hl7TextMessage msg = new Hl7TextMessage();
msg.setType("HL7");
msg.setServiceName("TestLab");
return msg;
}Or if the ID should be used:
private Hl7TextMessage createTestHl7Message(int id) {
Hl7TextMessage msg = new Hl7TextMessage();
+ msg.setLabNo(id);
msg.setType("HL7");
msg.setServiceName("TestLab");
return msg;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/LabServiceEndpointTest.java`
around lines 83 - 88, The helper method createTestHl7Message(int id) declares an
unused id parameter; either remove the parameter from the method signature and
all call sites, or assign the id to the message before returning (e.g., call the
appropriate setter on Hl7TextMessage such as msg.setId(id) or
msg.setMessageId(String.valueOf(id)) depending on the class API) so the id is
actually used; update any tests that call createTestHl7Message to match the
chosen change.
| @Test | ||
| @DisplayName("should return 200 success when HIN is unique") | ||
| void shouldReturn200Success_whenHinIsUnique() { | ||
| when(mockDemographicManager.searchByHealthCard(any(LoggedInInfo.class), eq("1234567890"))) | ||
| .thenReturn(Collections.emptyList()); | ||
|
|
||
| Response response = request().path("/patientDetailStatusService/isUniqueHC") | ||
| .query("hin", "1234567890") | ||
| .query("demographicNo", 123) | ||
| .get(); | ||
|
|
||
| assertThat(response.getStatus()).isEqualTo(200); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("should return 200 error when HIN is not unique") | ||
| void shouldReturn200Error_whenHinNotUnique() { | ||
| Demographic demo1 = new Demographic(); | ||
| demo1.setDemographicNo(456); | ||
| when(mockDemographicManager.searchByHealthCard(any(LoggedInInfo.class), eq("1234567890"))) | ||
| .thenReturn(List.of(demo1)); | ||
|
|
||
| Response response = request().path("/patientDetailStatusService/isUniqueHC") | ||
| .query("hin", "1234567890") | ||
| .query("demographicNo", 123) | ||
| .get(); | ||
|
|
||
| assertThat(response.getStatus()).isEqualTo(200); | ||
| } |
There was a problem hiding this comment.
Unique vs non-unique HIN branches are not behavior-verified.
Both tests validate only HTTP 200, so they do not prove that Line 100 (“unique”) and Line 114 (“not unique”) produce different functional outcomes. Please assert the response entity contract (e.g., success/error flag or message) for each branch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/PatientDetailStatusServiceEndpointTest.java`
around lines 98 - 126, The tests should assert the response body to distinguish
unique vs non-unique HIN rather than only HTTP 200: update
shouldReturn200Success_whenHinIsUnique and shouldReturn200Error_whenHinNotUnique
to parse the Response entity returned by the request to
/patientDetailStatusService/isUniqueHC and assert the expected contract (e.g.,
JSON boolean flag, status field, or error message) produced by the endpoint;
locate the call to mockDemographicManager.searchByHealthCard and the Demographic
demo1 setup to keep mocks the same, then add assertions on the response entity
(for example verifying a "unique": true for the unique case and "unique": false
or an error message for the non-unique case) so each branch’s behavior is
verified.
| @Test | ||
| @Disabled("TODO: Requires mock setup for NewAppointmentConverter internal SpringUtils.getBean() calls on CXF thread") | ||
| @DisplayName("should return 200 when appointment is added successfully") | ||
| void shouldReturn200_whenAppointmentAdded() { | ||
| // addAppointment returns void; default mock behavior (do nothing) is sufficient | ||
|
|
||
| String json = "{\"demographicNo\":1,\"providerNo\":\"999001\",\"startDate\":\"2026-04-01\",\"startTime\":\"09:00\",\"endTime\":\"09:15\"}"; | ||
|
|
||
| Response response = request().path("/schedule/add").post(json); | ||
|
|
||
| assertThat(response.getStatus()).isEqualTo(200); | ||
| } |
There was a problem hiding this comment.
Avoid keeping disabled endpoint tests in the committed suite.
Line 158 leaves /schedule/add effectively untested in CI. Please either complete the mock setup now or move this case to a tracked follow-up and remove it from the suite until it’s runnable.
I can help draft the NewAppointmentConverter/SpringUtils.getBean() mocking approach for this test if you want.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/ScheduleServiceEndpointTest.java`
around lines 157 - 168, The test method should not remain disabled in the
committed suite; either make it runnable by mocking the
NewAppointmentConverter/SpringUtils.getBean interaction or remove/track it as a
follow-up. To fix, choose one: (A) Enable the test by adding setup in
ScheduleServiceEndpointTest's `@BeforeEach` to mock SpringUtils.getBean (using
Mockito's mockStatic or a test-friendly wrapper) to return a mocked
NewAppointmentConverter and configure that mock's conversion/behavior so
request().path("/schedule/add").post(json) succeeds, or (B) remove the `@Disabled`
test method and instead create a short TODO comment and link to a tracked
follow-up issue for implementing NewAppointmentConverter/SpringUtils.getBean
mocks; ensure the test method name should be preserved
(shouldReturn200_whenAppointmentAdded) if re-adding later.
| @Test | ||
| @DisplayName("should return 200 with provider number when authenticated") | ||
| void shouldReturn200WithProviderNo_whenAuthenticated() { | ||
| when(mockLoggedInInfo.getLoggedInProviderNo()).thenReturn("999998"); | ||
|
|
||
| Response response = request().path("/status/checkIfAuthed").get(); | ||
|
|
||
| assertThat(response.getStatus()).isEqualTo(200); | ||
| } |
There was a problem hiding this comment.
Test does not validate authenticated payload semantics.
Line 65 only checks status code. If the endpoint stops returning provider data, this test still passes. Please assert the response body includes the authenticated provider number.
Proposed test-hardening diff
`@Test`
`@DisplayName`("should return 200 with provider number when authenticated")
void shouldReturn200WithProviderNo_whenAuthenticated() {
when(mockLoggedInInfo.getLoggedInProviderNo()).thenReturn("999998");
- Response response = request().path("/status/checkIfAuthed").get();
-
- assertThat(response.getStatus()).isEqualTo(200);
+ try (Response response = request().path("/status/checkIfAuthed").get()) {
+ assertThat(response.getStatus()).isEqualTo(200);
+ assertThat(response.readEntity(String.class)).contains("999998");
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/StatusServiceEndpointTest.java`
around lines 58 - 66, The test should assert the response payload contains the
authenticated provider number in addition to the 200 status: after calling
request().path("/status/checkIfAuthed").get() (in
shouldReturn200WithProviderNo_whenAuthenticated) read the response entity as a
String (or JSON) and assert it contains the mocked provider value returned by
mockLoggedInInfo.getLoggedInProviderNo() ("999998"); update the assertion to
check the response body includes that provider number (or specific JSON field if
the endpoint returns structured JSON) so the test fails if the provider data is
omitted.
| void shouldReturn200WithTicklers_whenSearchReturnsResults() { | ||
| when(mockTicklerManager.getTicklers(any(LoggedInInfo.class), any(CustomFilter.class), eq(0), eq(10))) | ||
| .thenReturn(Collections.emptyList()); | ||
|
|
||
| String searchJson = "{\"status\":\"A\"}"; | ||
|
|
||
| Response response = request().path("/tickler/search") | ||
| .query("startIndex", 0) | ||
| .query("limit", 10) | ||
| .post(Entity.json(searchJson)); | ||
|
|
||
| assertThat(response.getStatus()).isEqualTo(200); | ||
| TicklerResponse body = response.readEntity(TicklerResponse.class); | ||
| assertThat(body.getContent()).isEmpty(); | ||
| } |
There was a problem hiding this comment.
“Results exist” scenarios are currently testing empty responses.
Both tests named as non-empty cases stub Collections.emptyList() and assert empty content, so the positive serialization path is not validated.
Suggested adjustment for true positive-path coverage
void shouldReturn200WithTicklers_whenSearchReturnsResults() {
- when(mockTicklerManager.getTicklers(any(LoggedInInfo.class), any(CustomFilter.class), eq(0), eq(10)))
- .thenReturn(Collections.emptyList());
+ Tickler tickler = createTestTickler(1, "Follow up in 2 weeks");
+ when(mockTicklerManager.getTicklers(any(LoggedInInfo.class), any(CustomFilter.class), eq(0), eq(10)))
+ .thenReturn(List.of(tickler));
@@
- assertThat(body.getContent()).isEmpty();
+ assertThat(body.getContent()).isNotEmpty();
}Also applies to: 149-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/TicklerWebServiceEndpointTest.java`
around lines 107 - 121, The test
shouldReturn200WithTicklers_whenSearchReturnsResults is stubbing
mockTicklerManager.getTicklers to return Collections.emptyList(), so it never
exercises the non-empty serialization path; update the stub to return a
meaningful non-empty List<Tickler> (e.g. a single Tickler with known fields),
post the same request, then assert response.getStatus() == 200 and that the
TicklerResponse returned by response.readEntity(TicklerResponse.class) contains
the expected item(s) and field values; make the same change for the other
positive test that currently returns empty (the second test referenced around
the 149-160 region) so both positive-path cases validate actual non-empty
payload serialization.
|
@claude review all bot comments against the code and determine if they are valid, why or why not, what should be done and why, and provide a summary for me to review. Check for errors or omissions in the code scans and what might have been missed by the other reviewers. |
|
Claude finished @yingbull's task in 17m 1s —— View job Bot Comment Review AnalysisTasks:
OverviewReviewed 5 bots (CodeRabbit, Copilot, Codex, Penify, github-code-quality) against the actual code. Findings are grouped by validity below. ✅ Valid Bot Comments🔴 High Priority 1. Parallel execution / fixed CXF address collision (Copilot, CodeRabbit, Codex) The base classes do have a 2. 3. 4. 🟡 Medium Priority 5. Unused 6. Unnecessary 7. Unused imports (multiple bots)
Note: CodeRabbit's comment on 8. Misleading test name 9. 10. Tests that only check HTTP 200 without asserting response body (CodeRabbit)
11. 12. Disabled tests in committed suite (CodeRabbit) ❌ Invalid / False Positive Bot Comments1. CodeRabbit: "Use static access for String key = new LoggedInInfo().LOGGED_IN_INFO_KEY;CodeRabbit and CarlosSoapTestBase review suggest // LoggedInInfo.java line 51:
public final String LOGGED_IN_INFO_KEY = LoggedInInfo.class.getName() + ".LOGGED_IN_INFO_KEY";
2. Penify: "Add null checks for 3. Penify: "Use 4. CodeRabbit: "Add null check for 5. Penify: "Validate 🔍 Issues Missed by All Bots1. 2. CodeRabbit's 3. Tests are in 4. Base classes call 5. No test verifying CXF interceptor properly injects Summary
Top 3 actions to take:
The codebase otherwise shows solid test infrastructure design — good use of CXF local transport, proper Mockito lifecycle management, and clear |
|
@copilot resolve the merge conflicts in this pull request |
Agent-Logs-Url: https://github.com/carlos-emr/carlos/sessions/298b7653-9a9e-4e36-a308-dbc4daf4b597 Co-authored-by: yingbull <8680161+yingbull@users.noreply.github.com>
Co-authored-by: yingbull <8680161+yingbull@users.noreply.github.com>
Resolved the merge conflicts by merging Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pom.xml (1)
1596-1609:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAddress registration conflicts in parallel endpoint tests: use unique
local://URLs per test class.The existing Surefire configuration runs test classes in parallel (
<parallel>classes</parallel>,<threadCount>2</threadCount>). When two*EndpointTestclasses execute concurrently, bothCarlosSoapTestBaseandCarlosRestTestBaseregister their CXF servers at the samelocal://address:
- All SOAP endpoint tests use the hard-coded address
"local://soap-test"- All REST endpoint tests use the hard-coded address
"local://rest-test"CXF's
LocalTransportFactorymaintains a global, shared registry of endpoints keyed by address. WhenAllergyWsEndpointTestandBookingWsEndpointTeststart@BeforeEachconcurrently, they both attempt to register servers at"local://soap-test"in the same global registry, causing a race condition. This manifests as:
- Address already bound or endpoint conflict exceptions
- Non-deterministic test failures depending on registration order
Solution: Override
getServiceAddress()in each*EndpointTestclass to use a unique address, preventing registry collisions:// In AllergyWsEndpointTest `@Override` protected String getServiceAddress() { return "local://allergy-ws-test"; } // In BookingWsEndpointTest `@Override` protected String getServiceAddress() { return "local://booking-ws-test"; } // ... and so on for each endpoint test classAlternatively, add a
@Tag("endpoint")exclusion to the Surefire parallel execution (both base classes already declare@Tag("endpoint")), or use a dedicated sequential execution for endpoint tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pom.xml` around lines 1596 - 1609, Parallel test classes register CXF local:// addresses globally causing collisions: when CarlosSoapTestBase and CarlosRestTestBase use hard-coded "local://soap-test" / "local://rest-test" multiple *EndpointTest classes running in parallel hit the same LocalTransportFactory registry and race. Fix by overriding getServiceAddress() in each concrete *EndpointTest (e.g., AllergyWsEndpointTest, BookingWsEndpointTest) to return a unique local:// address per class (for example "local://allergy-ws-test", "local://booking-ws-test"), or alternatively change Surefire config to avoid parallel class execution for endpoint tests (exclude `@Tag`("endpoint") from parallel runs or run those tests sequentially); ensure changes reference CarlosSoapTestBase/CarlosRestTestBase getServiceAddress behavior so each test class registers a distinct address.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pom.xml`:
- Around line 1596-1609: Parallel test classes register CXF local:// addresses
globally causing collisions: when CarlosSoapTestBase and CarlosRestTestBase use
hard-coded "local://soap-test" / "local://rest-test" multiple *EndpointTest
classes running in parallel hit the same LocalTransportFactory registry and
race. Fix by overriding getServiceAddress() in each concrete *EndpointTest
(e.g., AllergyWsEndpointTest, BookingWsEndpointTest) to return a unique local://
address per class (for example "local://allergy-ws-test",
"local://booking-ws-test"), or alternatively change Surefire config to avoid
parallel class execution for endpoint tests (exclude `@Tag`("endpoint") from
parallel runs or run those tests sequentially); ensure changes reference
CarlosSoapTestBase/CarlosRestTestBase getServiceAddress behavior so each test
class registers a distinct address.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b9569830-0e7e-4e81-adf1-f1833e01c2fd
📒 Files selected for processing (3)
CLAUDE.mddependencies-lock.jsonpom.xml
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: carlos-emr/carlos
Timestamp: 2026-05-04T18:24:05.961Z
Learning: Always refer to this system as 'CARLOS EMR' or 'CARLOS' in all user-facing content (UI, documentation, README files, issue templates, workflows)
Learnt from: CR
Repo: carlos-emr/carlos
Timestamp: 2026-05-04T18:24:05.961Z
Learning: Never log or expose PHI (Patient Health Information) such as HIN/health card number, patient name, DOB, address, phone, diagnosis text, clinical notes, or lab values in logs or error messages
Learnt from: CR
Repo: carlos-emr/carlos
Timestamp: 2026-05-04T18:24:05.961Z
Learning: Commit messages must follow Conventional Commits format with prefixes: feat:, fix:, chore:, update:
🔇 Additional comments (5)
CLAUDE.md (3)
354-354: LGTM! Endpoint tags accurately documented.The new endpoint testing tags (
endpoint,rest,soap) are correctly documented and match the actual@Tagannotations onCarlosRestTestBaseandCarlosSoapTestBase. The grouping under "Endpoint Tags" is clear and follows the existing documentation pattern.
397-398: LGTM! Base class descriptions are accurate and well-placed.The descriptions of
CarlosRestTestBaseandCarlosSoapTestBasecorrectly state that both:
- Use CXF local transport for in-memory testing
- Run without database access (inheriting from
CarlosUnitTestBase)- Target REST and SOAP endpoints respectively
The placement in the "choose the right base class" section is logical, and the style matches the existing entries.
1228-1228: LGTM! Documentation reference is accurate and well-placed.The reference to
docs/test/endpoint-testing-guide.mdcorrectly describes the content (REST & SOAP endpoint testing with CXF local transport) and is appropriately placed in the Testing Patterns section alongside other test framework documentation references.dependencies-lock.json (1)
1735-1741: LGTM — scope alignment is correct.The
testscope fororg.apache.cxf:cxf-rt-transports-local:4.1.5matches the<scope>test</scope>declared inpom.xml, keeping it off the production/runtime classpath. No other fields were altered.pom.xml (1)
1079-1084: LGTM — test-scoped dependency is correctly declared.
${cxf.version}(4.1.5) is consistent with all other CXF modules, and<scope>test</scope>ensuresLocalTransportFactoryclasses never reach the production WAR. No exclusions are needed at test scope.
| protected String getServiceAddress() { | ||
| return "local://rest-test"; |
| protected String getServiceAddress() { | ||
| return "local://soap-test"; |
| protected ObjectMapper createTestObjectMapper() { | ||
| ObjectMapper mapper = new ObjectMapper(); | ||
| mapper.setAnnotationIntrospector( | ||
| new AnnotationIntrospectorPair( | ||
| new JacksonAnnotationIntrospector(), | ||
| new JakartaXmlBindAnnotationIntrospector(TypeFactory.defaultInstance()) | ||
| ) | ||
| ); | ||
| return mapper; |
| class GetCurrentPreventionRulesVersion { | ||
|
|
||
| @Test | ||
| @Disabled("TODO: Requires uiResources resource bundle in test classpath for getResourceBundle() call") |
| class GetMyInbox { | ||
|
|
||
| @Test | ||
| @Disabled("TODO: Requires mock setup for internal SpringUtils.getBean() calls in InboxService on CXF thread") |
| } | ||
|
|
||
| @Test | ||
| @Disabled("TODO: WsUtils static initializer calls SpringUtils.getBean(ProviderDao.class), cannot be instrumented by Mockito") |
| @Disabled("TODO: DocumentTransfer.toTransfer reads file contents from disk via getDocumentFileContentsAsBytes()") | ||
| @DisplayName("should return document transfer when found") | ||
| void shouldReturnDocumentTransfer_whenFound() throws IOException { | ||
| Document document = new Document(); | ||
| document.setDocumentNo(50); | ||
| CtlDocument ctlDocument = new CtlDocument(); | ||
| when(documentManager.getDocument(any(LoggedInInfo.class), eq(50))).thenReturn(document); | ||
| when(documentManager.getCtlDocumentByDocumentId(any(LoggedInInfo.class), eq(50))).thenReturn(ctlDocument); | ||
|
|
||
| DocumentWs proxy = createClient(DocumentWs.class); | ||
| DocumentTransfer result = proxy.getDocument(50); | ||
|
|
||
| assertThat(result).isNotNull(); |
| sf.setProviders(List.of(new JacksonJsonProvider(createTestObjectMapper()))); | ||
| sf.getInInterceptors().add(new TestAuthenticationInterceptor(mockServletRequest)); | ||
| sf.setTransportId(LocalTransportFactory.TRANSPORT_ID); | ||
| server = sf.create(); | ||
|
|
||
| client = WebClient.create(getServiceAddress(), | ||
| List.of(new JacksonJsonProvider(createTestObjectMapper()))) | ||
| .accept(MediaType.APPLICATION_JSON) |
|
@codex resolve all valid issues on review comments |
|
Summary
Testing
Reference (web check)
|
User description
Description
This PR introduces two new test base classes for HTTP-level endpoint testing using CXF's local transport, along with comprehensive test examples and documentation.
Key Changes
New Test Base Classes:
CarlosRestTestBase: Base class for testing JAX-RS REST endpoints using CXF local transportCarlosSoapTestBase: Base class for testing JAX-WS SOAP endpoints using CXF local transportBoth classes extend
CarlosUnitTestBaseand provide in-memory HTTP testing without requiring Spring context or database access, enabling fast unit-level endpoint testing.Comprehensive Test Examples: Added 40+ endpoint test classes demonstrating:
Documentation: Added
endpoint-testing-guide.mdproviding:Dependencies: Added
cxf-rt-transport-localto pom.xml to support CXF's local transport mechanism.Testing Strategy
The test base classes use CXF's local transport to exercise the full CXF pipeline (routing, serialization, interceptors, content negotiation) without opening TCP sockets. This provides:
Related Issues
N/A
How Was This Tested?
The PR includes 40+ endpoint test classes that serve as both examples and validation of the test base classes. These tests demonstrate:
Checklist
git commit -s)https://claude.ai/code/session_01768WpqTVcnxHuMFNoUssVU
Description
Changes walkthrough 📝
DemographicServiceEndpointTest.java
Add tests for DemographicService REST endpointsrc/test/java/io/github/carlos_emr/carlos/webserv/rest/DemographicServiceEndpointTest.java
RxWebServiceEndpointTest.java
Add tests for RxWebService REST endpointsrc/test/java/io/github/carlos_emr/carlos/webserv/rest/RxWebServiceEndpointTest.java
/rx/drugs/current/{demographicNo}.
TicklerWebServiceEndpointTest.java
Add tests for TicklerWebService REST endpointsrc/test/java/io/github/carlos_emr/carlos/webserv/rest/TicklerWebServiceEndpointTest.java
FormsServiceEndpointTest.java
Add tests for FormsService REST endpointsrc/test/java/io/github/carlos_emr/carlos/webserv/rest/FormsServiceEndpointTest.java
/forms/allEForms.
DiseaseRegistryServiceEndpointTest.java
Add tests for DiseaseRegistryService REST endpointsrc/test/java/io/github/carlos_emr/carlos/webserv/rest/DiseaseRegistryServiceEndpointTest.java
/dxRegisty/findLikeIssue.
ConsultationWebServiceEndpointTest.java
Add tests for ConsultationWebService REST endpointsrc/test/java/io/github/carlos_emr/carlos/webserv/rest/ConsultationWebServiceEndpointTest.java
/consults/getRequest.
MessagingServiceEndpointTest.java
Add tests for MessagingService REST endpointsrc/test/java/io/github/carlos_emr/carlos/webserv/rest/MessagingServiceEndpointTest.java
CarlosRestTestBase.java
Create CarlosRestTestBase for REST endpoint testingsrc/test/java/io/github/carlos_emr/carlos/test/base/CarlosRestTestBase.java
transport.