From 04e661b79709bec5d978c5327bf5434b017a286b Mon Sep 17 00:00:00 2001 From: Cornelius Roemer Date: Wed, 20 Nov 2024 10:52:51 +0100 Subject: [PATCH] chore(backend): fix some more racy backend tests by not asserting on exact order (#3247) * chore(backend): deflake ApproveProcessedDataEndpointTests using order independent helper I keep getting some flakes due to exact order requirements in tests. So I've now added a helper that makes it easy to test for accession versions being present in any order. Using this consistently in one test file where I've gotten a few flakes recently, but could probably be rolled out elsewhere as well. * Deflake DeleteSequencesEndpointTests with order independent assertions --- .../loculus/backend/controller/TestHelpers.kt | 15 ++++++++ .../ApproveProcessedDataEndpointTest.kt | 35 +++++++++---------- .../submission/DeleteSequencesEndpointTest.kt | 35 ++++++------------- 3 files changed, 41 insertions(+), 44 deletions(-) diff --git a/backend/src/test/kotlin/org/loculus/backend/controller/TestHelpers.kt b/backend/src/test/kotlin/org/loculus/backend/controller/TestHelpers.kt index 0675387aeb..246aecd62d 100644 --- a/backend/src/test/kotlin/org/loculus/backend/controller/TestHelpers.kt +++ b/backend/src/test/kotlin/org/loculus/backend/controller/TestHelpers.kt @@ -9,6 +9,9 @@ import kotlinx.datetime.plus import kotlinx.datetime.toLocalDateTime import org.hamcrest.MatcherAssert.assertThat import org.hamcrest.Matchers +import org.hamcrest.Matchers.allOf +import org.hamcrest.Matchers.containsInAnyOrder +import org.hamcrest.Matchers.hasEntry import org.hamcrest.Matchers.`is` import org.hamcrest.Matchers.not import org.loculus.backend.api.AccessionVersion @@ -19,8 +22,10 @@ import org.loculus.backend.api.Status import org.loculus.backend.utils.DateProvider import org.springframework.test.web.servlet.MvcResult import org.springframework.test.web.servlet.ResultActions +import org.springframework.test.web.servlet.ResultMatcher import org.springframework.test.web.servlet.result.MockMvcResultMatchers import org.springframework.test.web.servlet.result.MockMvcResultMatchers.content +import org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath import org.springframework.test.web.servlet.result.MockMvcResultMatchers.status import org.testcontainers.shaded.org.awaitility.Awaitility.await @@ -35,6 +40,16 @@ fun AccessionVersionInterface.toAccessionVersion() = AccessionVersion(this.acces fun List.getAccessionVersions() = map { it.toAccessionVersion() } +fun List.toAccessionVersionMatcher() = map { entry -> + allOf( + hasEntry("accession", entry.accession), + hasEntry("version", entry.version.toInt()), + ) +} + +fun jsonContainsAccessionVersionsInAnyOrder(expectedVersions: List): ResultMatcher = + jsonPath("\$[*]", containsInAnyOrder(expectedVersions.toAccessionVersionMatcher())) + fun addOrganismToPath(path: String, organism: String = DEFAULT_ORGANISM) = "/$organism/${path.trimStart('/')}" val jacksonObjectMapper: ObjectMapper = jacksonObjectMapper().findAndRegisterModules() diff --git a/backend/src/test/kotlin/org/loculus/backend/controller/submission/ApproveProcessedDataEndpointTest.kt b/backend/src/test/kotlin/org/loculus/backend/controller/submission/ApproveProcessedDataEndpointTest.kt index bf7e291b65..9229096563 100644 --- a/backend/src/test/kotlin/org/loculus/backend/controller/submission/ApproveProcessedDataEndpointTest.kt +++ b/backend/src/test/kotlin/org/loculus/backend/controller/submission/ApproveProcessedDataEndpointTest.kt @@ -2,7 +2,6 @@ package org.loculus.backend.controller.submission import org.hamcrest.MatcherAssert.assertThat import org.hamcrest.Matchers.containsString -import org.hamcrest.Matchers.hasItem import org.hamcrest.Matchers.hasSize import org.hamcrest.Matchers.`is` import org.junit.jupiter.api.Test @@ -22,6 +21,7 @@ import org.loculus.backend.controller.assertStatusIs import org.loculus.backend.controller.expectUnauthorizedResponse import org.loculus.backend.controller.generateJwtFor import org.loculus.backend.controller.getAccessionVersions +import org.loculus.backend.controller.jsonContainsAccessionVersionsInAnyOrder import org.loculus.backend.controller.jwtForSuperUser import org.loculus.backend.controller.submission.SubmitFiles.DefaultFiles.NUMBER_OF_SEQUENCES import org.springframework.beans.factory.annotation.Autowired @@ -54,7 +54,7 @@ class ApproveProcessedDataEndpointTest( client.approveProcessedSequenceEntries(scope = ALL, accessionVersions) .andExpect(status().isOk) .andExpect(content().contentType(MediaType.APPLICATION_JSON)) - .andExpect(jsonPath("\$[*].accession").value(accessionVersions.map { it.accession })) + .andExpect(jsonContainsAccessionVersionsInAnyOrder(accessionVersions)) assertThat( convenienceClient.getSequenceEntries().statusCounts[APPROVED_FOR_RELEASE], @@ -69,7 +69,7 @@ class ApproveProcessedDataEndpointTest( client.approveProcessedSequenceEntries(scope = ALL, accessionVersions) .andExpect(status().isOk) .andExpect(content().contentType(MediaType.APPLICATION_JSON)) - .andExpect(jsonPath("\$[*].accession").value(accessionVersions.map { it.accession })) + .andExpect(jsonContainsAccessionVersionsInAnyOrder(accessionVersions)) assertThat( convenienceClient.getSequenceEntries().statusCounts[APPROVED_FOR_RELEASE], @@ -79,12 +79,12 @@ class ApproveProcessedDataEndpointTest( @Test fun `WHEN I approve without accession filter or with full scope THEN all data is approved`() { - val approvableSequences = convenienceClient.prepareDataTo(PROCESSED).map { it.accession } + val accessionVersions = convenienceClient.prepareDataTo(PROCESSED) client.approveProcessedSequenceEntries(scope = ALL) .andExpect(status().isOk) .andExpect(content().contentType(MediaType.APPLICATION_JSON)) - .andExpect(jsonPath("\$[*].accession").value(approvableSequences)) + .andExpect(jsonContainsAccessionVersionsInAnyOrder(accessionVersions)) assertThat( convenienceClient.getSequenceEntriesOfUserInState(status = APPROVED_FOR_RELEASE), @@ -117,31 +117,32 @@ class ApproveProcessedDataEndpointTest( fun `WHEN I approve a sequence entry that does not exist THEN no accession should be approved`() { val nonExistentAccession = "999" - val accessionVersions = convenienceClient.prepareDataTo(PROCESSED).getAccessionVersions() + val processedAccessionVersion = convenienceClient.prepareDataTo(PROCESSED).getAccessionVersions().first() client.approveProcessedSequenceEntries( scope = ALL, listOf( - accessionVersions.first(), + processedAccessionVersion, AccessionVersion(nonExistentAccession, 1), ), ) .andExpect(status().isUnprocessableEntity) .andExpect(content().contentType(MediaType.APPLICATION_JSON)) - .andExpect(jsonPath("$.detail", containsString("Accession versions 999.1 do not exist"))) + .andExpect(jsonPath("$.detail", containsString("Accession versions $nonExistentAccession.1 do not exist"))) - convenienceClient.getSequenceEntry(accessionVersions.first()).assertStatusIs(PROCESSED) + convenienceClient.getSequenceEntry(processedAccessionVersion).assertStatusIs(PROCESSED) } @Test fun `WHEN I approve a sequence entry that does not exist THEN no sequence should be approved`() { val accessionVersions = convenienceClient.prepareDataTo(PROCESSED).getAccessionVersions() val nonExistingVersion = accessionVersions[1].copy(version = 999L) + val processedAccessionVersion = accessionVersions.first() client.approveProcessedSequenceEntries( scope = ALL, listOf( - accessionVersions.first(), + processedAccessionVersion, nonExistingVersion, ), ) @@ -157,7 +158,7 @@ class ApproveProcessedDataEndpointTest( ), ) - convenienceClient.getSequenceEntry(accessionVersions.first()).assertStatusIs(PROCESSED).assertHasError(false) + convenienceClient.getSequenceEntry(processedAccessionVersion).assertStatusIs(PROCESSED).assertHasError(false) } @Test @@ -240,8 +241,7 @@ class ApproveProcessedDataEndpointTest( ) .andExpect(status().isOk) .andExpect(content().contentType(MediaType.APPLICATION_JSON)) - .andExpect(jsonPath("$.[*]", hasSize>(otherOrganismData.size))) - .andExpect(jsonPath("$.[*].accession", hasItem(otherOrganismData.first().accession))) + .andExpect(jsonContainsAccessionVersionsInAnyOrder(otherOrganismData)) convenienceClient.getSequenceEntry( accession = defaultOrganismData.first().accession, @@ -357,8 +357,7 @@ class ApproveProcessedDataEndpointTest( client.approveProcessedSequenceEntries(scope = ALL, jwt = jwtForSuperUser) .andExpect(status().isOk) - .andExpect(jsonPath("\$.length()").value(accessionVersions.size)) - .andExpect(jsonPath("\$[*].accession").value(accessionVersions.map { it.accession })) + .andExpect(jsonContainsAccessionVersionsInAnyOrder(accessionVersions)) convenienceClient.getSequenceEntry(accessionVersions.first()).assertStatusIs(APPROVED_FOR_RELEASE) } @@ -379,8 +378,7 @@ class ApproveProcessedDataEndpointTest( submitterNamesFilter = listOf(DEFAULT_USER_NAME), ) .andExpect(status().isOk) - .andExpect(jsonPath("\$.length()").value(accessionVersions.size)) - .andExpect(jsonPath("\$[*].accession").value(accessionVersions.map { it.accession })) + .andExpect(jsonContainsAccessionVersionsInAnyOrder(accessionVersions)) convenienceClient.getSequenceEntry(accessionVersions.first()).assertStatusIs(APPROVED_FOR_RELEASE) convenienceClient.getSequenceEntry( @@ -402,8 +400,7 @@ class ApproveProcessedDataEndpointTest( jwt = jwtForSuperUser, ) .andExpect(status().isOk) - .andExpect(jsonPath("\$.length()").value(accessionVersions.size)) - .andExpect(jsonPath("\$[*].accession").value(accessionVersions.map { it.accession })) + .andExpect(jsonContainsAccessionVersionsInAnyOrder(accessionVersions)) convenienceClient.getSequenceEntry(accessionVersions.first()).assertStatusIs(APPROVED_FOR_RELEASE) } diff --git a/backend/src/test/kotlin/org/loculus/backend/controller/submission/DeleteSequencesEndpointTest.kt b/backend/src/test/kotlin/org/loculus/backend/controller/submission/DeleteSequencesEndpointTest.kt index 58556acb64..ed4a45ac25 100644 --- a/backend/src/test/kotlin/org/loculus/backend/controller/submission/DeleteSequencesEndpointTest.kt +++ b/backend/src/test/kotlin/org/loculus/backend/controller/submission/DeleteSequencesEndpointTest.kt @@ -1,11 +1,8 @@ package org.loculus.backend.controller.submission import org.hamcrest.MatcherAssert.assertThat -import org.hamcrest.Matchers.allOf -import org.hamcrest.Matchers.containsInAnyOrder import org.hamcrest.Matchers.containsString import org.hamcrest.Matchers.equalTo -import org.hamcrest.Matchers.hasEntry import org.hamcrest.Matchers.hasItem import org.hamcrest.Matchers.hasProperty import org.hamcrest.Matchers.hasSize @@ -28,6 +25,7 @@ import org.loculus.backend.controller.OTHER_ORGANISM import org.loculus.backend.controller.assertStatusIs import org.loculus.backend.controller.expectUnauthorizedResponse import org.loculus.backend.controller.generateJwtFor +import org.loculus.backend.controller.jsonContainsAccessionVersionsInAnyOrder import org.loculus.backend.controller.jwtForSuperUser import org.loculus.backend.controller.submission.SubmitFiles.DefaultFiles.NUMBER_OF_SEQUENCES import org.loculus.backend.controller.toAccessionVersion @@ -79,11 +77,7 @@ class DeleteSequencesEndpointTest( deletionResult .andExpect(status().isOk) .andExpect(content().contentType(MediaType.APPLICATION_JSON_VALUE)) - .andExpect(jsonPath("\$.length()").value(accessionVersionsToDelete.size)) - - accessionVersionsToDelete.forEach { - deletionResult.andExpect(jsonPath("\$[*].accession", hasItem(it.accession))) - } + .andExpect(jsonContainsAccessionVersionsInAnyOrder(accessionVersionsToDelete)) assertThat( convenienceClient.getSequenceEntriesOfUserInState( @@ -157,14 +151,16 @@ class DeleteSequencesEndpointTest( val erroneousSequences = convenienceClient.prepareDataTo(Status.PROCESSED, errors = true) val approvableSequences = convenienceClient.prepareDataTo(Status.PROCESSED) + val allSequences = erroneousSequences + approvableSequences + assertThat( convenienceClient.getSequenceEntries().sequenceEntries, - hasSize(erroneousSequences.size + approvableSequences.size), + hasSize(allSequences.size), ) client.deleteSequenceEntries(scope = ALL) .andExpect(status().isOk) - .andExpect(jsonPath("\$.length()").value(2 * NUMBER_OF_SEQUENCES)) + .andExpect(jsonContainsAccessionVersionsInAnyOrder(allSequences)) assertThat( convenienceClient.getSequenceEntries().sequenceEntries, @@ -193,7 +189,7 @@ class DeleteSequencesEndpointTest( client.deleteSequenceEntries(scope = DeleteSequenceScope.PROCESSED_WITH_ERRORS) .andExpect(status().isOk) - .andExpect(jsonPath("\$.length()").value(NUMBER_OF_SEQUENCES)) + .andExpect(jsonContainsAccessionVersionsInAnyOrder(erroneousSequences)) assertThat( convenienceClient.getProcessingResultCount(ProcessingResult.HAS_ERRORS), @@ -242,8 +238,7 @@ class DeleteSequencesEndpointTest( ) .andExpect(status().isOk) .andExpect(content().contentType(MediaType.APPLICATION_JSON_VALUE)) - .andExpect(jsonPath("$.[*]", hasSize>(otherOrganismData.size))) - .andExpect(jsonPath("$.[*].accession", hasItem(otherOrganismData.first().accession))) + .andExpect(jsonContainsAccessionVersionsInAnyOrder(otherOrganismData)) convenienceClient.getSequenceEntry( accession = defaultOrganismData.first().accession, @@ -299,9 +294,7 @@ class DeleteSequencesEndpointTest( client.deleteSequenceEntries(scope = ALL, jwt = jwtForSuperUser) .andExpect(status().isOk) .andExpect(content().contentType(MediaType.APPLICATION_JSON_VALUE)) - .andExpect(jsonPath("\$.length()").value(accessionVersions.size)) - .andExpect(jsonPath("\$[0].accession").value(accessionVersions.first().accession)) - .andExpect(jsonPath("\$[0].version").value(accessionVersions.first().version)) + .andExpect(jsonContainsAccessionVersionsInAnyOrder(accessionVersions)) } @Test @@ -312,13 +305,6 @@ class DeleteSequencesEndpointTest( ) .submissionIdMappings - val expectedPairs = accessionVersions.map { entry -> - allOf( - hasEntry("accession", entry.accession), - hasEntry("version", entry.version.toInt()), - ) - } - client.deleteSequenceEntries( scope = ALL, accessionVersionsFilter = accessionVersions, @@ -326,8 +312,7 @@ class DeleteSequencesEndpointTest( ) .andExpect(status().isOk) .andExpect(content().contentType(MediaType.APPLICATION_JSON_VALUE)) - .andExpect(jsonPath("\$.length()").value(accessionVersions.size)) - .andExpect(jsonPath("\$[*]", containsInAnyOrder(expectedPairs))) + .andExpect(jsonContainsAccessionVersionsInAnyOrder(accessionVersions)) } @Test