From 0e53ce58a732553045e90642581debeafab5df1e Mon Sep 17 00:00:00 2001 From: Joris Wouter Jonkers <74975850+ExtraToast@users.noreply.github.com> Date: Fri, 18 Sep 2026 13:07:52 +0200 Subject: [PATCH 1/2] ci(api): the integration suite runs in six shards, and the floors read all six api-tests was 8m20s, of which the integration suite was 5m16s run serially in one JVM over 121 classes. Split it: unit tests get their own job with no database at all, and the integration suite shards six ways on the same stable FQCN hash :tests:system and the frontend e2e suite already use. Each shard gets its own MariaDB service. The fixtures collide on unique fields, so the suite cannot share one database across parallel writers. The coverage floors read a single integrationTest.exec, and a shard's file covers only its own slice, so no floor holds against one alone. Shards upload their execution data and api-coverage merges the six: -PjacocoExecDir points the report and both floors at that directory and lifts the dependency that would otherwise re-run the suite. The floors stay as strict as they were. The shard filter is applied in doFirst, which Gradle does not hash, so the shard number is declared as a task input. Without it every shard has identical inputs and one shard's result is served to the next FROM-CACHE. ktlint and detekt move out of api-static into their own job. They are source-only and nothing waits on them, so 46s leaves the critical path that every test job sits behind. Refs #1318 --- .github/workflows/validate.yml | 167 ++++++++++++++---- .../kotlin/testing-conventions.gradle.kts | 76 +++++++- 2 files changed, 205 insertions(+), 38 deletions(-) diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index 72d233bbb..140e07856 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -270,6 +270,33 @@ jobs: :tests:system:resolveAllDependencies \ :services:api:bootJar + - name: Upload the jar for the test jobs + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 + with: + name: api-jar + # `bootJar` carries the `boot` classifier here; the plain `jar` next + # to it is the test-fixtures artifact and does not run. + path: services/api/build/libs/*-boot.jar + retention-days: 1 + if-no-files-found: error + + api-lint: + # ktlint and detekt are source-only: they need the warm Gradle cache + # api-static leaves behind, but nothing needs them. Off the critical path + # they cost their own runner rather than 46s in front of every test job. + name: API formatting and static analysis + needs: api-static + runs-on: ubuntu-latest + timeout-minutes: 20 + steps: + - name: Checkout repository + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - name: Set up Java + Gradle + uses: ./.github/actions/setup-java-gradle + with: + java-version: ${{ env.JAVA_VERSION }} + - name: Check Kotlin formatting # Its own step so a formatting failure reads as one rather than as a # broken compile. @@ -285,21 +312,61 @@ jobs: detektIntegrationTestSourceSet \ detektTestFixturesSourceSet - - name: Upload the jar for the test jobs + api-unit-tests: + # Unit tests need no database at all, so they no longer queue behind the + # integration suite in the same job. 1006 tests in about a minute. + name: API unit tests + runs-on: ubuntu-latest + timeout-minutes: 20 + needs: [ api-static, fe-static ] + + steps: + - name: Checkout repository + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - name: Install WebP tools + run: sudo apt-get update && sudo apt-get install -y --no-install-recommends webp + + - name: Set up Java + Gradle + uses: ./.github/actions/setup-java-gradle + with: + java-version: ${{ env.JAVA_VERSION }} + + - name: Run API unit tests + run: | + ./gradlew --no-daemon --build-cache \ + :services:api:test \ + :services:api:jacocoTestReport \ + :services:api:jacocoTestCoverageVerification + + - name: Upload API unit test reports + if: always() uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with: - name: api-jar - # `bootJar` carries the `boot` classifier here; the plain `jar` next - # to it is the test-fixtures artifact and does not run. - path: services/api/build/libs/*-boot.jar - retention-days: 1 - if-no-files-found: error + name: api-unit-test-reports + path: | + services/api/build/reports/jacoco/jacocoTestReport/jacocoTestReport.xml + services/api/build/reports/jacoco/jacocoTestReport/html + services/api/build/reports/tests/test + if-no-files-found: warn - api-tests: - name: API unit and integration tests + api-integration-tests: + # The integration suite is 121 classes over about five minutes, run + # serially in one JVM. Shard it the way :tests:system and the frontend e2e + # suite are sharded: a stable hash of the class name picks the slice, so a + # class always lands on the same shard. + # + # Each shard gets its own MariaDB service, which is what makes the split + # safe — the fixtures collide on unique fields, so the suite cannot share + # one database across parallel writers. + name: API integration tests (shard ${{ matrix.shard }}/6) runs-on: ubuntu-latest timeout-minutes: 30 needs: [ api-static, fe-static ] + strategy: + fail-fast: false + matrix: + shard: [ 1, 2, 3, 4, 5, 6 ] services: db: @@ -345,37 +412,74 @@ jobs: with: java-version: ${{ env.JAVA_VERSION }} - - name: Run API unit and integration tests - run: | - ./gradlew --no-daemon --build-cache \ - :services:api:test \ - :services:api:integrationTest \ - :services:api:jacocoTestReport \ - :services:api:jacocoIntegrationTestReport + # The floors are not run here: a shard's .exec covers only its own slice, + # so no floor holds against one alone. api-coverage merges the six and + # measures the whole picture. + - name: Run API integration tests + env: + SHARD_TOTAL: 6 + SHARD_INDEX: ${{ matrix.shard }} + run: ./gradlew --no-daemon --build-cache :services:api:integrationTest + + - name: Upload this shard's coverage execution data + if: always() + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 + with: + name: api-integration-exec-${{ matrix.shard }} + path: services/api/build/jacoco/integrationTest.exec + retention-days: 1 + if-no-files-found: error - # The floors have to be named: they are not reached through `check`, and - # running the reports does not run them (#1224). Separate from the step above - # so a coverage failure reads as a coverage failure rather than as a test one, - # and after it so the reports are still uploaded when a floor is missed. - - name: Hold coverage to its floors + - name: Upload API integration test reports + if: always() + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 + with: + name: api-integration-test-reports-shard-${{ matrix.shard }} + path: services/api/build/reports/tests/integrationTest + if-no-files-found: warn + + api-coverage: + # Fan-in for the sharded integration suite. `-PjacocoExecDir` points the + # report and the floors at every shard's execution data instead of the one + # local file, and lifts the dependency that would otherwise re-run the + # suite here. The floors stay exactly as strict as they were unsharded. + name: API integration coverage + runs-on: ubuntu-latest + timeout-minutes: 20 + needs: api-integration-tests + if: always() && needs.api-integration-tests.result != 'skipped' + + steps: + - name: Checkout repository + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - name: Set up Java + Gradle + uses: ./.github/actions/setup-java-gradle + with: + java-version: ${{ env.JAVA_VERSION }} + + - name: Download every shard's coverage execution data + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8 + with: + path: services/api/build/jacoco/shards + pattern: api-integration-exec-* + + - name: Report on the merged coverage and hold it to its floors run: | ./gradlew --no-daemon --build-cache \ - :services:api:jacocoTestCoverageVerification \ + -PjacocoExecDir=services/api/build/jacoco/shards \ + :services:api:jacocoIntegrationTestReport \ :services:api:jacocoIntegrationTestCoverageVerification - - name: Upload API test reports + - name: Upload API integration coverage report if: always() uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with: - name: api-test-reports + name: api-integration-coverage path: | - services/api/build/reports/jacoco/jacocoTestReport/jacocoTestReport.xml - services/api/build/reports/jacoco/jacocoTestReport/html services/api/build/reports/jacoco/jacocoIntegrationTestReport/jacocoIntegrationTestReport.xml services/api/build/reports/jacoco/jacocoIntegrationTestReport/html - services/api/build/reports/tests/test - services/api/build/reports/tests/integrationTest - if-no-files-found: warn + if-no-files-found: error build-images: # Builds the production images so a broken Dockerfile is caught on the pull @@ -756,7 +860,10 @@ jobs: - flux-kustomize-check - fe-static - api-static - - api-tests + - api-lint + - api-unit-tests + - api-integration-tests + - api-coverage - build-images - system-tests - acceptance-features diff --git a/build-logic/src/main/kotlin/testing-conventions.gradle.kts b/build-logic/src/main/kotlin/testing-conventions.gradle.kts index 0cb00fab3..0f0beacb6 100644 --- a/build-logic/src/main/kotlin/testing-conventions.gradle.kts +++ b/build-logic/src/main/kotlin/testing-conventions.gradle.kts @@ -1,9 +1,68 @@ +import java.io.File + plugins { java jacoco id("test-logging-conventions") } +// Sharding via env vars `SHARD_TOTAL` / `SHARD_INDEX` (1-based), the same +// partition :tests:system uses: scan the test classes directory sorted by FQCN, +// take `absoluteHash(fqcn) % SHARD_TOTAL`, and include only that slice through +// Gradle's `--tests` filter. A class always lands on the same shard, so a +// failure is easy to place, and leaving the env vars unset runs everything in +// one JVM exactly as before. +fun Test.shardByTestClass() { + val shardTotal = System.getenv("SHARD_TOTAL")?.toIntOrNull()?.takeIf { it > 1 } ?: return + val shardIndex = System.getenv("SHARD_INDEX")?.toIntOrNull()?.takeIf { it in 1..shardTotal } ?: return + // The filter below is applied in doFirst, which Gradle does not hash, so + // without these every shard has identical inputs and one shard's result is + // served to the next FROM-CACHE. Naming them makes each slice its own task. + inputs.property("shardTotal", shardTotal) + inputs.property("shardIndex", shardIndex) + doFirst { + val classes = + testClassesDirs.asFileTree + .matching { include("**/*Test.class", "**/*IT.class") } + .files + .mapNotNull { f -> + val root = testClassesDirs.firstOrNull { f.startsWith(it) } ?: return@mapNotNull null + f.relativeTo(root).path.removeSuffix(".class").replace(File.separatorChar, '.') + }.filter { !it.contains('$') } // skip anonymous / nested $-classes + .sorted() + val mine = classes.filter { Math.floorMod(it.hashCode(), shardTotal) == shardIndex - 1 } + logger.lifecycle("Shard $shardIndex/$shardTotal — ${mine.size}/${classes.size} test classes") + filter { + isFailOnNoMatchingTests = false + if (mine.isEmpty()) { + // No classes assigned — exclude everything by including a pattern + // that cannot match. An empty include list would match all. + includeTestsMatching("__no_match__shard_${shardIndex}__") + } else { + mine.forEach { includeTestsMatching(it) } + } + } + } +} + +// Where a report or a floor reads its execution data from. CI shards the +// integration suite, so each shard's `.exec` covers only its own slice and no +// floor holds against one alone; the fan-in job points this at a directory of +// every shard's file and the merged picture is what gets measured. Unset, it is +// the single local file, which is what a developer's run produces. +val mergedExecDir: String? = providers.gradleProperty("jacocoExecDir").orNull + +fun Project.executionDataFor(defaultFile: String): Any = + mergedExecDir?.let { fileTree(it) { include("**/*.exec") } } + ?: layout.buildDirectory.file(defaultFile) + +// Without merged data a report or a floor runs the suite it measures. With it, +// the suite has already run on the shards and the .exec files are all there is +// to read, so the fan-in must not re-run anything. +fun Task.dependsOnUnlessMerged(producer: Any) { + if (mergedExecDir == null) dependsOn(producer) +} + jacoco { toolVersion = "0.8.14" } @@ -44,12 +103,13 @@ tasks.register("integrationTest") { excludeTags("system", "brevo-live", "discord-live") } shouldRunAfter(tasks.test) + shardByTestClass() } // JaCoCo: unit coverage report — wired to the default `test` task only. tasks.jacocoTestReport { - dependsOn(tasks.test) - executionData.setFrom(layout.buildDirectory.file("jacoco/test.exec")) + dependsOnUnlessMerged(tasks.test) + executionData.setFrom(executionDataFor("jacoco/test.exec")) sourceDirectories.setFrom(sourceSets.main.get().allSource.srcDirs) classDirectories.setFrom( files(sourceSets.main.get().output.classesDirs).asFileTree.matching { @@ -71,8 +131,8 @@ tasks.jacocoTestReport { // JaCoCo: integration coverage report — independent file and HTML output. val jacocoIntegrationTestReport by tasks.registering(JacocoReport::class) { - dependsOn(tasks.named("integrationTest")) - executionData.setFrom(layout.buildDirectory.file("jacoco/integrationTest.exec")) + dependsOnUnlessMerged(tasks.named("integrationTest")) + executionData.setFrom(executionDataFor("jacoco/integrationTest.exec")) sourceDirectories.setFrom(sourceSets.main.get().allSource.srcDirs) classDirectories.setFrom( files(sourceSets.main.get().output.classesDirs).asFileTree.matching { @@ -93,8 +153,8 @@ val jacocoIntegrationTestReport by tasks.registering(JacocoReport::class) { // integration tests by nature touch more code paths but lean on real // infrastructure and are slower to run. tasks.jacocoTestCoverageVerification { - dependsOn(tasks.test) - executionData.setFrom(layout.buildDirectory.file("jacoco/test.exec")) + dependsOnUnlessMerged(tasks.test) + executionData.setFrom(executionDataFor("jacoco/test.exec")) sourceDirectories.setFrom(sourceSets.main.get().allSource.srcDirs) classDirectories.setFrom( files(sourceSets.main.get().output.classesDirs).asFileTree.matching { @@ -116,8 +176,8 @@ tasks.jacocoTestCoverageVerification { } val jacocoIntegrationTestCoverageVerification by tasks.registering(JacocoCoverageVerification::class) { - dependsOn(tasks.named("integrationTest")) - executionData.setFrom(layout.buildDirectory.file("jacoco/integrationTest.exec")) + dependsOnUnlessMerged(tasks.named("integrationTest")) + executionData.setFrom(executionDataFor("jacoco/integrationTest.exec")) sourceDirectories.setFrom(sourceSets.main.get().allSource.srcDirs) classDirectories.setFrom( files(sourceSets.main.get().output.classesDirs).asFileTree.matching { From 061c20bd3d0376bb353cb29b1274a53b8fcbaa11 Mon Sep 17 00:00:00 2001 From: Joris Wouter Jonkers <74975850+ExtraToast@users.noreply.github.com> Date: Fri, 18 Sep 2026 13:28:22 +0200 Subject: [PATCH 2/2] fix(ci): the merged coverage path resolves from the root, and an empty one refuses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `fileTree(dir)` inside :services:api resolves against services/api, so -PjacocoExecDir=services/api/build/jacoco/shards looked under services/api/services/api/ and found nothing. Resolve against the root instead. Finding nothing was not loud. JacocoReport with empty execution data goes NO-SOURCE and JacocoCoverageVerification passes with nothing to measure, so a wrong path reads as a green coverage gate — only the report upload failing on if-no-files-found made it visible. The merged mode now refuses an empty set rather than measuring it. Verified against the six .exec files the first run produced: merged instruction coverage 0.6851 against a floor of 0.40, and the report is written. --- .../main/kotlin/testing-conventions.gradle.kts | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/build-logic/src/main/kotlin/testing-conventions.gradle.kts b/build-logic/src/main/kotlin/testing-conventions.gradle.kts index 0f0beacb6..762a494af 100644 --- a/build-logic/src/main/kotlin/testing-conventions.gradle.kts +++ b/build-logic/src/main/kotlin/testing-conventions.gradle.kts @@ -52,9 +52,18 @@ fun Test.shardByTestClass() { // the single local file, which is what a developer's run produces. val mergedExecDir: String? = providers.gradleProperty("jacocoExecDir").orNull -fun Project.executionDataFor(defaultFile: String): Any = - mergedExecDir?.let { fileTree(it) { include("**/*.exec") } } - ?: layout.buildDirectory.file(defaultFile) +fun Project.executionDataFor(defaultFile: String): Any { + val dir = mergedExecDir ?: return layout.buildDirectory.file(defaultFile) + // Resolved against the root, not this project: a relative path would land + // under services/api/ and quietly find nothing. + val resolved = rootProject.file(dir) + val found = fileTree(resolved) { include("**/*.exec") } + // An empty set is not an empty report — JacocoReport goes NO-SOURCE and the + // verification passes with nothing to measure, so a typo in the path reads + // as a green coverage gate. Refuse instead. + check(!found.isEmpty) { "No .exec files under $resolved — the merged coverage would measure nothing." } + return found +} // Without merged data a report or a floor runs the suite it measures. With it, // the suite has already run on the shards and the .exec files are all there is