diff --git a/.github/instructions/database.instructions.md b/.github/instructions/database.instructions.md index b2a01c516d..dc815003ba 100644 --- a/.github/instructions/database.instructions.md +++ b/.github/instructions/database.instructions.md @@ -101,6 +101,13 @@ CREATE TABLE example_table ( CREATE INDEX idx_example_name ON example_table(name); ``` +### Reference Data in Migrations + +Integration tests reset the database by truncating every table except `flyway_schema_history` and `local_council`. +If a migration inserts reference data that tests rely on, add that table to `PRESERVED_TABLES` in +`src/test/kotlin/uk/gov/communities/prsdb/webapp/testHelpers/IntegrationTestHelper.kt`. Otherwise the data will be +deleted before every test that resets the database. + ## Search with Trigrams - Use `pg_trgm` extension for fuzzy text search - See existing search implementations for patterns diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index 9fa6f11af1..c7e082aab9 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -51,7 +51,17 @@ jobs: env: EMAILNOTIFICATIONS_APIKEY: ${{ secrets.NOTIFY_KEY }} EMAILNOTIFICATIONS_USE_PRODUCTION_NOTIFY: ${{ github.base_ref == 'production' || github.merge_group.base_ref == 'refs/head/production' }} - run: ./gradlew clean check --no-daemon + run: ./gradlew clean check --no-daemon -PtestForks=2 + + # TEMPORARY, experiment branch only: gives each CI run per-class timings rather than a single + # noisy wall-clock number, so changes under ~10% can be judged against CI's ~2 min variance. + - name: Upload test results + if: always() + uses: actions/upload-artifact@v4 + with: + name: test-results-${{ github.run_attempt }} + path: build/test-results/test/*.xml + retention-days: 5 - name: Run ktlintFormat run: ./gradlew ktlintFormat --no-daemon diff --git a/build.gradle.kts b/build.gradle.kts index ddf77022b8..7695d93cfe 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -1,5 +1,6 @@ import org.gradle.internal.os.OperatingSystem import org.jetbrains.kotlin.gradle.tasks.KotlinCompile +import java.util.concurrent.ConcurrentLinkedQueue plugins { kotlin("jvm") version "1.9.25" @@ -159,10 +160,47 @@ tasks.withType { dependsOn("copyBuiltAssets") } +val testForks = (project.findProperty("testForks") as String?)?.toInt() ?: 1 +val slowTestCount = (project.findProperty("slowTestCount") as String?)?.toInt() ?: 15 + tasks.withType { useJUnitPlatform() dependsOn("copyBuiltAssets") maxHeapSize = "2g" + maxParallelForks = testForks + + val taskLogger = logger + val testDurations = ConcurrentLinkedQueue>() + + addTestListener( + object : TestListener { + override fun beforeSuite(suite: TestDescriptor) = Unit + + override fun beforeTest(testDescriptor: TestDescriptor) = Unit + + override fun afterTest( + testDescriptor: TestDescriptor, + result: TestResult, + ) { + val name = "${testDescriptor.className}.${testDescriptor.name}" + testDurations.add(name to (result.endTime - result.startTime)) + } + + override fun afterSuite( + suite: TestDescriptor, + result: TestResult, + ) { + if (suite.parent != null) return + val slowest = testDurations.sortedByDescending { it.second }.take(slowTestCount) + if (slowest.isEmpty()) return + taskLogger.lifecycle("") + taskLogger.lifecycle("Slowest ${slowest.size} tests:") + slowest.forEach { (name, durationMs) -> + taskLogger.lifecycle(String.format(" %8.2fs %s", durationMs / 1000.0, name)) + } + } + }, + ) } tasks.register("playwright") { diff --git a/docs/IntegrationTestReadMe.md b/docs/IntegrationTestReadMe.md index 11d230593f..28f30df005 100644 --- a/docs/IntegrationTestReadMe.md +++ b/docs/IntegrationTestReadMe.md @@ -13,6 +13,27 @@ We set seed data by passing SQL script names into integration test class constru Tests that require different seed data to the rest of the class must be put in nested classes that inherit from `NestedIntegrationTestWithMutableData` or `NestedIntegrationTestWithImmutableData` depending on the outer class. +Resetting the database means truncating every table in the `public` schema with `RESTART IDENTITY CASCADE`, then +running the seed scripts. Two tables are deliberately preserved: + +* `flyway_schema_history` - Flyway's own bookkeeping. Dropping it would make every migration re-run. +* `local_council` - reference data inserted by `V1_0_0__la_and_address_tables.sql` (as `local_authority`, renamed by + `V1_6_0`). No seed script writes to it. + +These are the only two tables with rows after a bare migrate. If you add a migration that inserts reference data, add +its table to `PRESERVED_TABLES` in `IntegrationTestHelper`, or the data will be wiped before every test. You can check +which tables hold reference data by migrating an empty database and looking for non-empty tables. + +## Test Timing + +Every run ends with a table of the slowest tests, so that suite speed stays visible as scenarios are added. Change +its length with `-PslowTestCount=N`. + +Running the suite across parallel Gradle forks was measured and rejected. Each fork needs its own Spring contexts, +Postgres and Redis containers, application instance and browser, and Playwright tests are latency-sensitive, so the +contention inflates per-test time faster than the parallelism recovers it: summed test time rose 32% at two forks and +84% at three, making wall clock no better than serial while introducing timeout and Docker-discovery flakes. + ## Page Objects (and Components) We encapsulate logic for interacting with our pages into page objects (as described by [Martin Fowler](https://martinfowler.com/bliki/PageObject.html) and diff --git a/src/test/kotlin/uk/gov/communities/prsdb/webapp/TestcontainersConfiguration.kt b/src/test/kotlin/uk/gov/communities/prsdb/webapp/TestcontainersConfiguration.kt index 52edb4554e..103fbbabc4 100644 --- a/src/test/kotlin/uk/gov/communities/prsdb/webapp/TestcontainersConfiguration.kt +++ b/src/test/kotlin/uk/gov/communities/prsdb/webapp/TestcontainersConfiguration.kt @@ -9,11 +9,39 @@ import org.testcontainers.utility.DockerImageName @TestConfiguration(proxyBeanMethods = false) class TestcontainersConfiguration { + companion object { + // One Postgres and one Redis per JVM, shared by every Spring context. Without this, each of the + // ~16 distinct integration contexts starts its own pair and a serial run ends with 32 containers + // alive, none of which are ever released. + // + // stop() is deliberately a no-op. Spring Boot closes container beans when their context is + // destroyed, and the test context cache is LRU with a default maxSize of 32 against more contexts + // than that in a full run. An eviction would otherwise stop these containers out from under the + // contexts still using them. Testcontainers' Ryuk sidecar removes them when the JVM exits. + private val postgres: PostgreSQLContainer<*> = + object : PostgreSQLContainer(DockerImageName.parse("postgres:latest")) { + override fun stop() = Unit + }.apply { + // One container now serves every Spring context, and each context keeps its own Hikari + // pool, so Postgres' default limit of 100 connections is exhausted once ~10 contexts exist. + setCommand("postgres", "-c", "max_connections=400") + start() + } + + private val redis: GenericContainer<*> = + object : GenericContainer(DockerImageName.parse("redis:latest")) { + override fun stop() = Unit + }.apply { + addExposedPort(6379) + start() + } + } + @Bean @ServiceConnection - fun postgresContainer(): PostgreSQLContainer<*> = PostgreSQLContainer(DockerImageName.parse("postgres:latest")) + fun postgresContainer(): PostgreSQLContainer<*> = postgres @Bean @ServiceConnection(name = "redis") - fun redisContainer(): GenericContainer<*> = GenericContainer(DockerImageName.parse("redis:latest")).withExposedPorts(6379) + fun redisContainer(): GenericContainer<*> = redis } diff --git a/src/test/kotlin/uk/gov/communities/prsdb/webapp/integration/IntegrationTest.kt b/src/test/kotlin/uk/gov/communities/prsdb/webapp/integration/IntegrationTest.kt index c255e5bc72..01d0976461 100644 --- a/src/test/kotlin/uk/gov/communities/prsdb/webapp/integration/IntegrationTest.kt +++ b/src/test/kotlin/uk/gov/communities/prsdb/webapp/integration/IntegrationTest.kt @@ -34,10 +34,7 @@ import uk.gov.service.notify.NotificationClient import kotlin.reflect.full.isSubclassOf @Import(TestcontainersConfiguration::class) -@SpringBootTest( - webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT, - properties = ["spring.flyway.clean-disabled=false"], -) +@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) @TestInstance(TestInstance.Lifecycle.PER_CLASS) @UsePlaywright @ActiveProfiles(profiles = ["local", "local-no-auth"]) @@ -135,6 +132,7 @@ abstract class IntegrationTest { @BeforeEach fun setUp(page: Page) { + blockExternalAnalytics(page) navigator = Navigator(page, port) } @@ -161,7 +159,16 @@ abstract class IntegrationTest { fun createPageAndNavigator(browserContext: BrowserContext): Pair { val page = browserContext.newPage() + blockExternalAnalytics(page) val navigator = Navigator(page, port) return Pair(page, navigator) } + + // The layout renders