chore: add regression test for DHIS2-11655

[teleivo]

https://jira.dhis2.org/browse/DHIS2-11655

is already fixed by #9022

However, there was no test preventing the bug/ensuring the above PRs refactoring/removal actually works.

This change adds a regression test for it.

This is how it looks like when for example shouldReturnSingleTeiGivenFilterWhileSkippingPaging fails because the same TEI is returned twice

java.lang.AssertionError: 1 expectation failed.
JSON path instances.findAll { it.trackedEntity == 'JQJFJJUXQNB' }.size() doesn't match.
Expected: is <1>
  Actual: 2

I provoked the failure with a sample payload recorded after replicating the issue.

This is how it looks like when for example shouldReturnSingleTeiGivenFilterWhileSkippingPaging fails because there is a TEI in the result that does not have the expected name (attribute == 'kZeSYCgaHTk') 'Bravo'. The request to DHIS2 was made with a filter expecting every TEI to have the name 'Bravo'.

java.lang.AssertionError: 1 expectation failed.
JSON path instances.attributes.flatten().findAll { it.attribute == 'kZeSYCgaHTk' }.value doesn't match.
Expected: every item is is "Bravo"
  Actual: [Bravo, Test]

[vilkg]

Nice addition, can you use our framework? It has support for everything you need here. Check TrackerActions class.

I am having trouble backporting it to 2.36. Not sure if that is because the e2e are relatively new and mostly evolve on master.

Yes, the framework constantly evolves, so backports aren't as easy as cherry-picking sometimes. Especially since the new tracker importer is new and new functionality is constantly added.

where such functionality would be tested I am totally open to suggestions.

Generally, it's best to cover it in the lowest levels - unit tests, integrations tests. As you know, e2e tests are the most expensive ones and the setup takes time.

[teleivo]

Nice addition, can you use our framework? It has support for everything you need here. Check TrackerActions class.

I honestly tried, but it was too hard 😓 Sometimes abstractions make things harder. This test was super easy to write/is to read and does the job. TrackerApiResponse does not fit with what api/tracker/trackedEntities returns. If you can make it work or can tell me that I overlooked something I am all ears. It does not seem to warrant the effort IMHO.

[vilkg]

@teleivo, consistency matters and you have tons of examples, even in that same class :) Would you start writing code in a different way ignoring the team's established patterns? I can help you:

trackerActions.get("?orgUnit=x&program=x&filter=x")
   .validate() 
   .statusCode(200)
   .body("instances.trackedEntity", contains("Kj6vYde4LHh"));

Not sure if you saw this guide, but hopefully it will give you some overview.

[vilkg]

I'm not sure if these expectations actually cover the case shouldReturnSingleTeiGivenFilterWhileSkippingPaging. You might want to look into different matches that make sure that there's only one item with that id. Or use the id in jsonpath, then it's enough to assert on size.

[teleivo]

package org.hisp.dhis.dashboard;

import org.junit.Test;

import java.util.Arrays;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.contains;

public class SampleTest {

    @Test
    public void passes() {
        assertThat(Arrays.asList("foo"), contains("foo"));
    }

    @Test
    public void fails() {
        assertThat(Arrays.asList("foo", "faa"), contains("foo"));
    }
}

fails() will fail with

java.lang.AssertionError: 
Expected: iterable containing ["foo"]
     but: Not matched: "faa"

	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:8)
	at org.hisp.dhis.dashboard.SampleTest.fails(SampleTest.java:19)
...

and sample from failing e2e is shown here #9095 (comment)

[vilkg]

Not sure I'm understanding the relevance. The regression was about the same TEI returned twice, no? Your test won't fail if there are two TEIs.

[vilkg]

In light of contains checking the size, I would suggest using hasItem matcher and hasSize with greaterOrEqualTo, unless you actually create a TEI with a unique attribute in preconditions :) this will be especially relevant when I finish up enabling parallel test execution.

[teleivo]

Not sure I'm understanding the relevance. The regression was about the same TEI returned twice, no? Your test won't fail if there are two TEIs.

This shows that if the actual returns 2 elements, where one of them is the sole element we expect it will fail. The example failure here #9095 (comment) shows that even if the actual contains the same element twice it will still fail since we only expect one element.

The failed test here #9095 (comment) was generated using the sample JSON I got from reproducing the issue. I recorded the JSON. I ran the test and pointed it to http://www.mbtest.org/docs/gettingStarted which faked DHIS2 tracker API with the recorded JSON. This shows that this test with this assertion would fail on the commit that had the bug.

Whether it's unexpected that contains also accounts for the size of actual and expected is another topic. The behaviour and the javadoc state it does. If you prefer it to be something else that is ok for me 👌🏻

[teleivo]

@teleivo, consistency matters and you have tons of examples, even in that same class :) Would you start writing code in a different way ignoring the team's established patterns? I can help you:

trackerActions.get("?orgUnit=x&program=x&filter=x")
   .validate() 
   .statusCode(200)
   .body("instances.trackedEntity", contains("Kj6vYde4LHh"));

Not sure if you saw this guide, but hopefully it will give you some overview.

I completely agree that consistency matters 👍🏻 You were the first to point me to that guide, thank you! I will make sure to read it thoroughly 👓 You are right, there are examples even in the same test class. There is just a lot going on that obfuscates the gist of what I needed to do to get a test working.

My observation with programming in general, is that often the abstractions we make, make things harder. Harder to read and to change. The tests I wrote earlier are easy to read at first glance, everything is there to for example create a CURL command out of it (ok, the base auth is hidden somewhere in the restassured setup 😂). I was also able to easily point the test to a fake DHIS2 to check that it would fail with the JSON response shown in the jira issue. That was doable with the framework but hacky and it took me a while. I will need to remember that the next time I want to do that. That is friction. Anyway, I really appreciate the effort you put into all kinds of tests! Also the time you put into reviewing my PRs. Challenging what I do. Thank you very much 😄

I updated the tests to use the framework. Are they alright now? Let me know if you want me to exchange the contains for something else. It does work as explained in the comment above.

[vilkg]

I was also able to easily point the test to a fake DHIS2 to check that it would fail with the JSON response shown in the jira issue. That was doable with the framework but hacky and it took me a while.

You can change the props here or define an env variable :)

You can make the tests prettier, eg use getTrackedEntities() from the TrackerActions class, use query params builder and etc. But that's details; it looks fine the way it is. I'm more concerned about flakiness caused by multiple entities having the same attribute and your test failing when there's no bug.

[teleivo]

I was also able to easily point the test to a fake DHIS2 to check that it would fail with the JSON response shown in the jira issue. That was doable with the framework but hacky and it took me a while.

You can change the props here or define an env variable :)

That will not work as I would need to also fake all the other data that is needed by all the subclasses/setup methods that are running.

So what I did was to only point my request in the test to my fake instance.

You can make the tests prettier, eg use getTrackedEntities() from the TrackerActions class, use query params builder and etc. But that's details; it looks fine the way it is. I'm more concerned about flakiness caused by multiple entities having the same attribute and your test failing when there's no bug.

I don't see any getTrackedEntities did you just add that recently?

So you are saying I should set up my own data for the test? There is only one Bravo in here https://github.com/dhis2/dhis2-core/blob/8345bb0e86fc277bf9cbdb1e81cf6ebc3618ce4a/dhis-2/dhis-e2e-test/src/test/resources/tracker/importer/teis/teisWithEnrollmentsAndEvents.json

There is already a dependency between tests when it comes to the data. I mean multiple classes/tests are relying on the teisWithEnrollmentsAndEvents.json

• ImportStrategyTests
• TrackerExportTests (already before my change)
• EventUpdateTests
• TeiImportTests

Does the same argument apply to these tests as well? I mean in terms of flakiness.

[ameenhere]

So you are saying I should set up my own data for the test? There is only one Bravo in here https://github.com/dhis2/dhis2-core/blob/8345bb0e86fc277bf9cbdb1e81cf6ebc3618ce4a/dhis-2/dhis-e2e-test/src/test/resources/tracker/importer/teis/teisWithEnrollmentsAndEvents.json

I would say yes. If no other data or metadata setup files (the .json files present in the resource folders) matches the requirement for your test setup, then we have to create a new one to setup the data for that class. Often we reuse the same data setup files for several test classes because the test scenario revolves around the same setups.

Changing teisWithEnrollmentsAndEvents.json would be risky but you can create a copy of it and make the required changes to suit your testing scenario and rename the file accordingly.

[vilkg]

I don't think it's a problem that you are reusing the TEIs inserted in the general test setup, since you are not modifying them. But I would change the expectations so that test doesn't fail when there are more TEIs returned.

Eg: validate every tracked entity attribute value is equal to Bravo, and instances.trackedEntity hasItem (only one item) "Kj6vYde4LHh".

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[teleivo]

@vilkg I hope this change now reflects what you would like to assert on 😄

Please see
#9095 (comment)

for sample assertion errors I provoked