diff options
| -rw-r--r-- | .coverage-baseline | 2 | ||||
| -rw-r--r-- | CLAUDE.md | 33 | ||||
| -rw-r--r-- | app/build.gradle.kts | 85 |
3 files changed, 105 insertions, 15 deletions
diff --git a/.coverage-baseline b/.coverage-baseline index b486fa4..33e28ab 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -65.04 +89.93 @@ -87,9 +87,11 @@ claims three separate times across these repos. 2. **Tests** — `./gradlew testDebugUnitTest` passes in full. 3. **Static analysis** — `./gradlew detekt lintDebug` clean: zero lint errors, zero new detekt findings beyond the baseline. -4. **Coverage must not decrease** against `.coverage-baseline` (line coverage over - `app/src/main/java` excluding `ui/**`). Set `COVERAGE_EXCEPTION="reason"` for justified drops, - e.g. deleting covered dead code. +4. **Coverage must not decrease** against `.coverage-baseline` — line coverage over + `app/src/main/java`, less Compose screens and the framework adapters listed in + `app/build.gradle.kts`. Set `COVERAGE_EXCEPTION="reason"` for justified drops, e.g. deleting + covered dead code. With the adapters excluded the number is about 90%, so a drop now means + something; before it hovered around 65% and mostly measured how much glue existed. 5. **Manifest permission diff** — if `<uses-permission>` changed, the commit message says why. 6. **Schema migrations** — if `AppDatabase.kt`'s `version` changed, a migration test must be touched in the same diff. Room tracks by version; a wrong migration corrupts a real, @@ -188,13 +190,24 @@ out of a possibly-partial MP3, and the JSON path covers the shows that mark chap ## Things a future session should know -- **The coverage gate has been overridden four times**, each recorded in the commit message. All - four are Android-framework glue that cannot execute off-device: the Chaquopy bridge, - PlaybackService, ChaquopyYouTubeSource, PlayerConnection and the two Workers. Each time the - gate fired it was a useful prompt — it drove PositionWriter onto a narrow Playhead interface - and pulled RetentionSweeper out of its Worker, both of which found or prevented real bugs. - If the exceptions keep accumulating, the honest fix is a jacoco exclusion for framework - adapters, which is a decision for Peter rather than something to slip in. +- **Framework adapters are excluded from coverage, as of 2026-09-26.** The gate had been + overridden eight times, every one a class whose whole body is a call into ExoPlayer, Media3, + Room, WorkManager or Chaquopy. The last few overrides were perverse: pulling decisions *out* + of a service into tested classes lowered the number, and writing an instrumented test for a + migration lowered it again, because `jacocoTestReport` measures unit tests only. A metric that + punishes both was measuring how much Android glue exists. + + Peter's call, taken 2026-09-26. The list lives in `app/build.gradle.kts` next to its reasoning, + and coverage went 65.04% → 89.93% on the same code. **Adding to that list needs a reason, not + just a name** — the bar is "cannot execute off a device", not "awkward to test". Earlier, the + gate firing was a useful prompt: it drove PositionWriter onto a narrow Playhead interface and + pulled RetentionSweeper out of its Worker, both of which found real bugs. That value only + survives if the list stays honest. + + `jacocoFullReport` adds instrumented coverage on top, for when you want the fuller picture + with a device attached. The commit gate deliberately does not read it: device data only exists + after a device run, so folding it in would make the gate report different numbers for + identical code. - **Detekt config has been relaxed five times**, all documented inline in `config/detekt/detekt.yml`. Twice the better answer was restructuring instead: `Screens.kt` was split per screen and diff --git a/app/build.gradle.kts b/app/build.gradle.kts index e525eea..eba61d1 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -99,6 +99,9 @@ android { buildTypes { debug { enableUnitTestCoverage = true + // Produces device coverage data for jacocoFullReport. The commit gate deliberately + // does not read it -- see that task. + enableAndroidTestCoverage = true } release { isMinifyEnabled = false @@ -230,21 +233,95 @@ jacoco { toolVersion = "0.8.12" } +/** + * What coverage does not count. + * + * Compose screens are excluded for the same reason as FeeDreader: they are impractical to + * unit-test and including them makes the metric meaningless. + * + * The rest are framework adapters -- classes whose whole body is a call into ExoPlayer, Media3, + * Room, WorkManager or Chaquopy, none of which can execute on the JVM. They were overridden + * eight times in a row with `COVERAGE_EXCEPTION`, and the last few were perverse: extracting + * decisions *out* of a service into tested classes lowered the number, and writing an + * instrumented test for a migration lowered it again. A metric that punishes both is measuring + * how much Android glue exists rather than how well the logic is tested. + * + * **Adding to this list needs a reason, not just a name.** The bar is "cannot execute off a + * device", not "awkward to test" -- QueueRepository, PlaybackAdvancer, EpisodeDownloader and + * ChapterFetcher all touch framework-adjacent things and all stay measured, because their + * logic runs fine against fakes. + */ +val coverageExclusions = listOf( + "**/ui/**", + "**/R.class", "**/R\$*.class", "**/BuildConfig.*", "**/*_Impl*", + + // ExoPlayer, MediaSession and the browse tree: no JVM equivalent to run against. + "**/playback/PlaybackService*", + "**/playback/PlayerConnection*", + "**/playback/ListenSessionCallback*", + "**/playback/LibraryTree*", + "**/playback/NotificationControls*", + "**/playback/PodcastPlayer*", + + // Room's own machinery, including the migrations -- proven on a device by MigrationTest. + "**/data/AppDatabase*", + + // WorkManager entry points; the decisions live in DownloadDrainer, RetentionSweeper and + // QueuedDownloads, which are measured. + "**/work/RefreshWorker*", + "**/work/DownloadWorker*", + "**/work/RetentionWorker*", + "**/work/Scheduling*", + + // The embedded CPython bridge, and the app/graph wiring that only constructs things. + "**/download/ChaquopyAudioSource*", + "**/youtube/YouTubeAudio*", + "**/data/Graph*", + "**/data/AppGraph*", + "**/ListenApp*", +) + tasks.register<JacocoReport>("jacocoTestReport") { dependsOn("testDebugUnitTest") reports { xml.required.set(true) html.required.set(true) } - // ui/** is excluded for the same reason as FeeDreader: Compose screens are impractical to - // unit-test for coverage and including them makes the metric meaningless. val kotlinDebugTree = fileTree("${layout.buildDirectory.get()}/tmp/kotlin-classes/debug") { - exclude("**/ui/**", "**/R.class", "**/R$*.class", "**/BuildConfig.*", "**/*_Impl*") + exclude(coverageExclusions) } val javaDebugTree = fileTree("${layout.buildDirectory.get()}/intermediates/javac/debug") { - exclude("**/ui/**", "**/R.class", "**/R$*.class", "**/BuildConfig.*", "**/*_Impl*") + exclude(coverageExclusions) } classDirectories.setFrom(files(kotlinDebugTree, javaDebugTree)) sourceDirectories.setFrom(files("src/main/java")) executionData.setFrom(fileTree(layout.buildDirectory) { include("**/*.exec") }) } + +/** + * Unit *and* instrumented coverage, for when you want the fuller picture. + * + * Deliberately not what the commit gate reads. Instrumented data only exists after a device run, + * so folding it into the gate would make the number depend on whether one happened recently and + * on stale .ec files lying around -- a gate that reports different figures for identical code is + * worse than a narrow one. Run this with a device attached: + * + * ./gradlew connectedDebugAndroidTest jacocoFullReport + */ +tasks.register<JacocoReport>("jacocoFullReport") { + dependsOn("testDebugUnitTest") + reports { + xml.required.set(true) + html.required.set(true) + } + val kotlinDebugTree = fileTree("${layout.buildDirectory.get()}/tmp/kotlin-classes/debug") { + exclude(coverageExclusions) + } + val javaDebugTree = fileTree("${layout.buildDirectory.get()}/intermediates/javac/debug") { + exclude(coverageExclusions) + } + classDirectories.setFrom(files(kotlinDebugTree, javaDebugTree)) + sourceDirectories.setFrom(files("src/main/java")) + // .ec is what a device run writes, alongside the JVM run's .exec. + executionData.setFrom(fileTree(layout.buildDirectory) { include("**/*.exec", "**/*.ec") }) +} |