From 2e0eaef4e9650cc3b389d1ec4a03d36e15c6f0fe Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 4 Jul 2026 11:27:17 -0500 Subject: [PATCH] feat(ci): no-regression JVM coverage gate scoped to the testable surface Scope :app:jacocoTestReport's denominator to the JVM-testable surface and add a :app:jacocoTestCoverageVerification no-regression gate that shares the same classDirectories/executionData/sourceDirectories, wired into both the `check` lifecycle task and CI's unit-test job (part of the `CI passed` gate). Excluded from the denominator (structurally unreachable from a JVM unit test): Compose screen/component render code, Android framework entry points (*Activity/*Service/Application/*BackupAgent), Hilt DI (**/di/**), and the src/debug cold-open probe. Kept in scope: ViewModels, repositories, mappers, DAOs, utils, richtext, mail, reporting logic, and the six WorkManager Workers. Corrects PR #292, which excluded **/*Worker*: SyncWorker, BackfillWorker, PruneWorker, SendWorker, ReportPurgeWorker and ReportUploadWorker are all directly unit-tested, so they stay counted in both numerator and denominator (only their Hilt wiring, WorkManagerModule, is excluded, via **/di/**). Baseline: 80.21% line (4838/6032). Floor: 0.79 (~1.2% headroom) so ordinary noise doesn't red-flag it while a real drop fails. Manual ratchet for now: bump the floor up in the same PR when coverage rises materially. Closes #251 Closes #292 Co-Authored-By: Claude Opus 4.8 --- .github/workflows/ci.yml | 14 ++- app/build.gradle.kts | 235 +++++++++++++++++++++++++++++---------- 2 files changed, 189 insertions(+), 60 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 30dd301..e59fdb4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -346,9 +346,10 @@ jobs: path: app/build/reports/tests/testDebugUnitTest/ if-no-files-found: warn - # JaCoCo XML + HTML coverage for the JVM unit tests (issue #192). Reporting only — no - # threshold gate yet, so a coverage regression does not fail CI (a jacocoTestCoverageVerification - # gate is a natural follow-up once there's a baseline to enforce). + # JaCoCo XML + HTML coverage for the JVM unit tests (issue #192), scoped to the JVM-testable + # surface (issues #290/#292). The report is generated and uploaded first, then a no-regression + # gate (issue #251) fails the job if overall LINE coverage drops below the floor pinned in + # app/build.gradle.kts. Kept in this unit-test job so it is part of the `CI passed` gate. - name: Generate JaCoCo coverage report if: ${{ !cancelled() }} run: ./gradlew :app:jacocoTestReport --stacktrace @@ -361,6 +362,13 @@ jobs: path: app/build/reports/jacoco/jacocoTestReport/ if-no-files-found: warn + # No-regression coverage gate (issue #251): fails CI if scoped LINE coverage regresses below the + # floor pinned in app/build.gradle.kts. Runs after the upload so the HTML/XML report is always + # archived for triage even when this step goes red. + - name: Verify JaCoCo coverage (no-regression floor) + if: ${{ !cancelled() }} + run: ./gradlew :app:jacocoTestCoverageVerification --stacktrace + static-analysis: name: Static analysis needs: traffic-control # order after runner-priority orchestration (P0/broken preempt; P1–P9 hold-back) diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 5eb564b..c63c990 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -1,4 +1,5 @@ // SPDX-License-Identifier: GPL-3.0-or-later +import org.gradle.testing.jacoco.tasks.JacocoCoverageVerification import org.gradle.testing.jacoco.tasks.JacocoReport import java.util.Properties @@ -199,10 +200,146 @@ jacoco { toolVersion = libs.versions.jacoco.get() } -// Unit-test coverage report (issue #192). Reads the exec data the base `jacoco` plugin records for -// the JVM `testDebugUnitTest` task, mapped against the debug variant's compiled Kotlin classes and -// the hand-written main sources. Produces machine-readable XML + human-readable HTML under -// build/reports/jacoco/jacocoTestReport/. Instrumented/E2E coverage is out of scope (issue #192). +// Unit-test coverage (issue #192). Two tasks share ONE scoping so they can never measure different +// surfaces: `jacocoTestReport` (XML+HTML under build/reports/jacoco/jacocoTestReport/) and +// `jacocoTestCoverageVerification` (the no-regression gate, further down). Both read the exec data +// the base `jacoco` plugin records for the JVM `testDebugUnitTest` task, mapped against the debug +// variant's compiled Kotlin classes and the hand-written main sources. Instrumented/E2E coverage is +// out of scope (issue #192). + +// Strip generated code from the denominator so the % reflects hand-written Kotlin. Verified +// against an actual compileDebugKotlin output tree: Room's KSP-generated `_Impl` DAOs/database +// and the Compose compiler's per-file ComposableSingletons holders are the only generated code +// that actually lands in classDirectories below (Room's KSP output is added as an extra Kotlin +// source root on the *same* compile task, so it comes out the same door as hand-written code). +// Hilt/Dagger's generated Java (Hilt_*, Dagger*_HiltComponents*, *_GeneratedInjector, *_Factory, +// *_MembersInjector, hilt_aggregated_deps) and AGP's BuildConfig/R/Manifest are compiled by a +// separate javac task (hiltJavaCompileDebug / compileDebugJavaWithJavac) into a directory this +// report never reads, so those patterns are conventional belt-and-suspenders in case that ever +// changes. DataBinding isn't enabled in this module (no buildFeatures.dataBinding/viewBinding), +// so there's nothing generated for it to exclude; if it's turned on later, add "**/BR.class", +// "**/DataBinderMapperImpl*.class" and "**/*Binding.class". +// +// Deliberately NOT excluded: Kotlin's own `$$inlined$` synthetic classes (e.g. for +// `Flow.map { ... }` in the repositories) — those hold real hand-written transform logic, not +// generated boilerplate, so stripping them would silently shrink the measured surface. +val jacocoGeneratedExcludes = listOf( + "**/R.class", + "**/R\$*.class", + "**/BuildConfig.*", + "**/Manifest*.*", + "**/Hilt_*.class", + "**/Dagger*.class", + "**/*_Hilt*", + "**/*_GeneratedInjector.class", + "**/hilt_aggregated_deps/**", + "**/dagger/**", + "**/*_Factory*", + "**/*_MembersInjector*", + "**/*_Provide*", + "**/*_Impl*", + "**/ComposableSingletons*", +) + +// Scope the denominator to the JVM-testable surface (issues #290/#292, following the Phase-2 coverage +// audit): unlike `jacocoGeneratedExcludes` above, none of this is generated code — it is hand-written +// but structurally unreachable from a JVM unit test, so counting it against the metric just measures +// how much Compose/framework glue exists rather than how well the logic is tested. Four buckets: +// 1. Compose screen/component render code — only exercisable via a Compose UI test or an emulator. +// 2. Android framework entry points the OS instantiates directly (Activity/Service/Application/ +// BackupAgent) rather than the app's own code constructing them. +// 3. Hilt DI modules — `@Provides`/`@Binds` one-liners with no branching logic. +// 4. The `src/debug` cold-open probe (issue #221), a `ContentProvider` that only runs in a forked +// instrumented process (see its kdoc) and is never packaged in a release build anyway. +// +// KEPT IN SCOPE — this corrects #292, which excluded `**/*Worker*`: the six WorkManager workers +// (SyncWorker, BackfillWorker, PruneWorker, SendWorker, ReportPurgeWorker, ReportUploadWorker) are +// all directly unit-tested today (construct-the-worker-and-call-doWork(), e.g. SyncWorkerTest, +// SendWorkerTest), so they carry real tested logic and belong in BOTH the numerator and denominator. +// Only their Hilt wiring (WorkManagerModule) is excluded, and that falls under `**/di/**` below — so +// there is intentionally no `**/*Worker*` glob in the list. +// +// Also deliberately NOT excluded, even though each sits in a package/pattern above and renders UI: +// files that carry plain, unit-tested logic alongside their `@Composable` functions. JaCoCo has no +// finer granularity than a class file, and Kotlin compiles every top-level function in a .kt file — +// `@Composable` or not — into the SAME facade class (`Kt.class`); excluding that class would +// silently zero out the tested function's coverage too, not just the render code's. Confirmed +// against these files' own dedicated tests before leaving them out of the list below: +// - ui/compose/RichTextEditor.kt (RichTextEditorTest) — the AnnotatedString<->RichTextContent +// editor-op functions (applyStyle/applyBlock/applyLink/toRichContent/toAnnotatedString/...). +// - ui/settings/AccountReorderList.kt (AccountReorderListTest) — commitDrag's reorder maths. +// - ui/reader/HtmlBody.kt (HtmlBodyTest, InlineImageResolverTest) — cidKey/resolveInlineImage/ +// wrapHtml/toCssHex. +// - ui/reporting/ReportReviewScreen.kt (ReportReviewClipboardTest) — copyReportPayloadToClipboard. +// (ui/compose/format/FontRegistry.kt and ui/mailbox/FolderLabels.kt are plain logic files with no +// `@Composable` at all — never at risk — but sit right next to excluded files below.) For the same +// reason this list names each Screen/component file individually rather than a package-wide +// "**/ui/**": a blanket pattern can't carve the four files above back out, and would also reach +// every `*ViewModel*`. +val jacocoNonJvmTestableSurface = listOf( + // --- Compose UI render code: one glob per screen/component file (see the exceptions above) --- + "**/LibreMailApp*", + "**/AccountPickerScreen*", + "**/AppPasswordSetupScreen*", + "**/ManualSetupScreen*", + "**/ComposeScreen*", + "**/ColorSwatch*", + "**/FontPicker*", + "**/FontSizePicker*", + "**/ParagraphAlignmentControl*", + "**/DraftsScreen*", + "**/LockScreen*", + "**/AppLockGateHost*", + "**/FolderDrawer*", + "**/MailboxScreen*", + "**/AddAnotherAccountScreen*", + "**/BatteryOptimizationScreen*", + "**/ContactsAccessScreen*", + "**/LicenseScreen*", + "**/OnboardingWelcomeScreen*", + "**/OutboxScreen*", + "**/ReaderScreen*", + "**/ProblemReportsScreen*", + "**/AccountSettingsScreen*", + "**/SettingsScreen*", + "**/SettingsComponents*", + "**/SignatureEditScreen*", + "**/SignaturesScreen*", + // --- Android framework entry points (OS-instantiated). NB: Workers are intentionally NOT here + // --- (they are unit-tested — see the KEPT IN SCOPE note above). + "**/*Activity*", + "**/*Service*", + "**/LibreMailApplication*", + "**/*BackupAgent*", + // --- Hilt DI wiring (includes WorkManagerModule) --- + "**/di/**", + // --- src/debug cold-open probe (issue #221) --- + "**/data/local/coldopen/**", +) + +// Classes = the debug variant's compiled Kotlin (AGP 9 built-in Kotlin output), with the generated +// code and the non-JVM-testable surface above stripped out. All hand-written code here is Kotlin, so +// the javac output (purely Hilt/Dagger/BuildConfig generated) is omitted. Hoisted to a shared val so +// the report and the verification gate always run against the identical denominator. +val jacocoDebugKotlinClasses = layout.buildDirectory.dir( + "intermediates/built_in_kotlinc/debug/compileDebugKotlin/classes", +) +val jacocoClassDirectories = fileTree(jacocoDebugKotlinClasses) { + exclude(jacocoGeneratedExcludes + jacocoNonJvmTestableSurface) +} + +// Sources = hand-written main Kotlin. +val jacocoSourceDirectories = files("src/main/kotlin") + +// Exec data written by the instrumented testDebugUnitTest task. Accept the base `jacoco` plugin's +// default location and AGP's enableUnitTestCoverage location so the wiring is robust either way. +val jacocoExecutionData = fileTree(layout.buildDirectory) { + include( + "jacoco/testDebugUnitTest.exec", + "outputs/unit_test_code_coverage/debugUnitTest/testDebugUnitTest.exec", + ) +} + tasks.register("jacocoTestReport") { // Ensure the unit tests (and thus their coverage exec data) have run first. dependsOn("testDebugUnitTest") @@ -214,62 +351,46 @@ tasks.register("jacocoTestReport") { html.required.set(true) } - // Strip generated code from the denominator so the % reflects hand-written Kotlin. Verified - // against an actual compileDebugKotlin output tree: Room's KSP-generated `_Impl` DAOs/database - // and the Compose compiler's per-file ComposableSingletons holders are the only generated code - // that actually lands in classDirectories below (Room's KSP output is added as an extra Kotlin - // source root on the *same* compile task, so it comes out the same door as hand-written code). - // Hilt/Dagger's generated Java (Hilt_*, Dagger*_HiltComponents*, *_GeneratedInjector, *_Factory, - // *_MembersInjector, hilt_aggregated_deps) and AGP's BuildConfig/R/Manifest are compiled by a - // separate javac task (hiltJavaCompileDebug / compileDebugJavaWithJavac) into a directory this - // report never reads, so those patterns are conventional belt-and-suspenders in case that ever - // changes. DataBinding isn't enabled in this module (no buildFeatures.dataBinding/viewBinding), - // so there's nothing generated for it to exclude; if it's turned on later, add "**/BR.class", - // "**/DataBinderMapperImpl*.class" and "**/*Binding.class". - // - // Deliberately NOT excluded: Kotlin's own `$$inlined$` synthetic classes (e.g. for - // `Flow.map { ... }` in the repositories) — those hold real hand-written transform logic, not - // generated boilerplate, so stripping them would silently shrink the measured surface. - val generated = listOf( - "**/R.class", - "**/R\$*.class", - "**/BuildConfig.*", - "**/Manifest*.*", - "**/Hilt_*.class", - "**/Dagger*.class", - "**/*_Hilt*", - "**/*_GeneratedInjector.class", - "**/hilt_aggregated_deps/**", - "**/dagger/**", - "**/*_Factory*", - "**/*_MembersInjector*", - "**/*_Provide*", - "**/*_Impl*", - "**/ComposableSingletons*", - ) + classDirectories.setFrom(jacocoClassDirectories) + sourceDirectories.setFrom(jacocoSourceDirectories) + executionData.setFrom(jacocoExecutionData) +} - // Classes = the debug variant's compiled Kotlin (AGP 9 built-in Kotlin output). All hand-written - // code here is Kotlin, so the javac output (purely Hilt/Dagger/BuildConfig generated) is omitted. - val debugKotlinClasses = layout.buildDirectory.dir( - "intermediates/built_in_kotlinc/debug/compileDebugKotlin/classes", - ) - classDirectories.setFrom( - fileTree(debugKotlinClasses) { exclude(generated) }, - ) +// No-regression coverage gate (closes #251; scoping from #290/#292). Fails `check` / CI when the +// overall LINE coverage of the scoped surface above drops below `jacocoLineCoverageFloor`. This is a +// FLOOR, not an absolute 95% target — the maintainer chose a ratchet over a fixed goal. The floor is +// set a hair (~0.5–1%) below the measured baseline so ordinary run-to-run noise doesn't red-flag it, +// while a real regression still fails the build. Manual ratchet FOR NOW: when coverage rises +// materially, bump this number up in the SAME PR so the floor tracks reality (there is no auto-ratchet +// yet). Baseline measured when this floor was set: 80.21% line (4838/6032), floor 0.79 (~1.2% headroom). +val jacocoLineCoverageFloor = "0.79" +tasks.register("jacocoTestCoverageVerification") { + // Same inputs as jacocoTestReport (shared vals above) so the gate enforces exactly what the + // report shows. Depend on the unit tests so the exec data exists before verifying. + dependsOn("testDebugUnitTest") + group = "verification" + description = "Fails the build if scoped JVM unit-test LINE coverage regresses below the floor." - // Sources = hand-written main Kotlin. - sourceDirectories.setFrom(files("src/main/kotlin")) + classDirectories.setFrom(jacocoClassDirectories) + sourceDirectories.setFrom(jacocoSourceDirectories) + executionData.setFrom(jacocoExecutionData) - // Exec data written by the instrumented testDebugUnitTest task. Accept the base `jacoco` plugin's - // default location and AGP's enableUnitTestCoverage location so the wiring is robust either way. - executionData.setFrom( - fileTree(layout.buildDirectory) { - include( - "jacoco/testDebugUnitTest.exec", - "outputs/unit_test_code_coverage/debugUnitTest/testDebugUnitTest.exec", - ) - }, - ) + violationRules { + rule { + element = "BUNDLE" + limit { + counter = "LINE" + value = "COVEREDRATIO" + minimum = jacocoLineCoverageFloor.toBigDecimal() + } + } + } +} + +// Make the aggregate `check` lifecycle task enforce the no-regression floor locally too, so a +// coverage regression is caught by `./gradlew check` and not only in CI. +tasks.named("check") { + dependsOn("jacocoTestCoverageVerification") } dependencies {