summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorPeter Stone <thepeterstone@gmail.com>2026-09-26 10:04:46 +0000
committerPeter Stone <thepeterstone@gmail.com>2026-09-26 10:04:46 +0000
commit315fee32e64c75f641b7673092d1f3f42ea27e57 (patch)
treeb58f7171b9efe0681d66a0c1b189a7adb1265aa4
parent12a0f7f07179f4d2b98a5db3c2a6bca33b57644c (diff)
Exclude framework adapters from coverage
Peter's call. The gate had been overridden eight times running, every one a class whose whole body is a call into ExoPlayer, Media3, Room, WorkManager or Chaquopy -- none of which execute on the JVM. The last few overrides were the argument for doing this. Pulling completion decisions out of PlaybackService into tested classes lowered the number, because the tested part was already counted and the glue it left behind was counted too. Writing an instrumented test for a migration lowered it again, because jacocoTestReport measures unit tests only. A metric that punishes both of those is measuring how much Android glue the app contains, not how well its logic is tested. Same code, 65.04% -> 89.93%: 629 uncovered lines were adapters, 131 are real. What is left is honest -- page-fetch coroutines that need the actual host, a few data classes, and Room's Converters, which are plain Kotlin and genuinely untested. A drop from here will mean something. The list sits next to its reasoning in the build file, with the bar written down: "cannot execute off a device", not "awkward to test". QueueRepository, PlaybackAdvancer, EpisodeDownloader and ChapterFetcher all touch framework-adjacent things and all stay measured. Also added jacocoFullReport, which folds in instrumented coverage 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 including it would make the gate report different numbers for identical code depending on whether one happened recently -- worse than a narrow gate. One trap worth recording: Kotlin block comments nest. A KDoc mentioning a glob with `/*` in it opened a nested comment, so the closing `*/` only closed the inner one and the comment ran on until it hit a `*/` inside a later string literal -- producing syntax errors on innocent lines well below the real cause. No behaviour change. 425 unit tests green, detekt and lint clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018AwnALZL6bD4VBtATDVv3B
-rw-r--r--.coverage-baseline2
-rw-r--r--CLAUDE.md33
-rw-r--r--app/build.gradle.kts85
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
diff --git a/CLAUDE.md b/CLAUDE.md
index 623ba1b..3abde17 100644
--- a/CLAUDE.md
+++ b/CLAUDE.md
@@ -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") })
+}