Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: move TrackedEntitiesExportControllerTest to postgres DHIS2-18541 #19672

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

teleivo
Copy link
Contributor

@teleivo teleivo commented Jan 15, 2025

  • Make controller test a postgres one. The controller test using H2 could only test the /tracker/trackedEntities/{uid} and not /tracker/trackedEntities (due to JSONB features not present in H2).
  • Set the default orgUnitMode in OperationParams to ACCESSIBLE as this is the safest/least surprising/mostly what we want value. null is not a valid value and makes the OperationParams harder to use.
  • improve Assertions

Found Bug

The bug was fixed in PR #19647 but is now testable via the changes in this PR. We only test the /tracker/trackedEntities/{uid} using controller tests so far. That code path is extremely complicated 😢 (aggregate). We'll replace the aggregate code soon enough by using the enrollment/event service.

### fields * gets the tracked entity with enrollments.events.relationships
GET {{PROTOCOL}}://{{AUTH}}@{{HOST}}/api/tracker/trackedEntities?trackedEntities=Kj6vYde4LHh&fields=*
Accept: application/json

### fields enrollments did not get the enrollments.events.relationships
GET {{PROTOCOL}}://{{AUTH}}@{{HOST}}/api/tracker/trackedEntities?trackedEntities=Kj6vYde4LHh&fields=enrollments
Accept: application/json

Reason was

--- a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/export/trackedentity/aggregates/EventAggregate.java
+++ b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/export/trackedentity/aggregates/EventAggregate.java
@@ -105,7 +105,7 @@ public class EventAggregate implements Aggregate {
               Multimap<String, RelationshipItem> relationships = relationshipAsync.join();

               for (Event event : events.values()) {
-                if (ctx.getParams().isIncludeRelationships()) {
+                if (ctx.getParams().getEventParams().isIncludeRelationships()) {
                   event.setRelationshipItems(new HashSet<>(relationships.get(event.getUid())));
                 }

We used the wrong param to detect whether relationships should be set on the event. TrackedEntityParams are another source of unnecessary complexity.

Someday

  • migrate remaining tests in TrackedEntitiesExportControllerTest to use the json fixture instead of creating the setup using the manager
  • backport the bug fix to previous versions
  • improve test setup
    • Merge the two test/resources/tracker/event_and_enrollment.json into one
    • We need to make our test code more composable. Extract a setup class to create metadata/tracker data so we are free to extend any test class and do not create the next TestBase 😬
    • We need to be able to share test code across controller and service integration tests. This is currently not possible due to our module setup. Unless we put test code into src/main in the tracker module. Should we merge the test-integration and test-web-api modules? We need to investigate the pros/cons.

@teleivo teleivo changed the title test: DHIS2-18541 test: move TrackedEntitiesExportControllerTest to postgres DHIS2-18541 Jan 15, 2025
@teleivo teleivo force-pushed the DHIS2-18541-tests branch 3 times, most recently from e55bcd1 to 14049a2 Compare January 15, 2025 14:27
@teleivo teleivo marked this pull request as ready for review January 16, 2025 04:31
@teleivo teleivo requested a review from a team as a code owner January 16, 2025 04:31
@teleivo teleivo merged commit 73ccf63 into master Jan 16, 2025
17 checks passed
@teleivo teleivo deleted the DHIS2-18541-tests branch January 16, 2025 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants