Simplify controller tests

[fengelniederhammer]

Placeholder - to be discussed

[fhennig]

Yes, thanks for greating the issue. Here some thoughts I wrote down a while ago:

Test coverage is amazing, but in certain places I think there are a few too many indirections.
The tests are sometimes difficult to grasp. Examples below.

---

mockMvc.perform(getSample("/${SampleRoute.AGGREGATED.pathSegment}?dataFormat=fasta"))

While it's nice to use the constant, and the getSample function (which is also sort of like a constant) - I think we lose the benefit of the clarity of the test we would get otherwise. Especially for newcomers to the project, this version would be much clearer, and the test would serve also as documentation:

mockMvc.perform(get("/sample/aggregated?dataFormat=fasta"))

---

// scenario def with fields:
fields = listOf("region", "date", "primaryKey")

// ... elsewhere, in a test:
postSample("/details")  
    .content("""{"fields": ${(objectMapper.writeValueAsString(scenario.fields))}}""")  
    .contentType(MediaType.APPLICATION_JSON)  
    .header(ACCEPT, "text/csv"),

I think using the auto wired ObjectMapper that is also used live, to create test data, isn't ideal. I would prefer if instead we did it in a simpler way, without JSON serialisation. This makes it easier to read, also easier to copy-paste the snippet to test manually, and we do not rely on a component from the side that is under test. Changing a config in the system under test shouldn't - in my opinion - change things in the test code.

// scenario def with query def:
query = """{"fields": ["region", "date", "primaryKey"]}"""

// ... elsewhere, in a test:
postSample("/details")  
    .content(scenario.query)
    .contentType(MediaType.APPLICATION_JSON)  
    .header(ACCEPT, "text/csv"),

---

I've also looked at

LAPIS/lapis/src/test/kotlin/org/genspectrum/lapis/controller/LapisControllerDataFormatTest.kt

Line 55 in d912c66

@MethodSource("getRequestsWithInvalidDataFormat")

and the way the request is built there also involves quite a bit of indirection, and it's hard to dissect what the actual request is, in the end.

[fhennig]

Fabian, Christian and I just looked at the file I linked above as an example, and we agreed that at least the two examples are things that we could simplify. In general the way the tests work could possibly be refactored a bit, to make things more tidy.

The way tests are built is all a bit 'historically grown', there might also be duplications etc.

[fengelniederhammer]

IMO there are some low hanging fruits (low effort, little yield) like getting rid of getSample and using JSON directly instead of generating it via the object mapper.

But we could also restructure the tests and

• define more clearly what is tested where
• get rid of potential duplications
• simplify the setup
• document the tests if they are still too complex.

That would be more effort since it involves coming up with a concept, moving every test around and refactoring the current "framework".
I imagine that we could group the tests by

• "tests controller input" (the correct arguments are passed on to the model)
• "tests controller output" (the response looks as expected)