diff --git a/CLAUDE.md b/CLAUDE.md index 530a8b0..bb2f592 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -184,11 +184,29 @@ install for code that can never run — and on API 37 the full APK does not fit - **coverage gaps** — the line never executes. Filtered to sites where JaCoCo reports `mi > 0`, a concrete instruction no test runs, which is what separates a real gap from a partial branch on a compound condition. That filter cut the candidate list roughly in half and was right to. + **Wave 4 found it wrong in both directions, though — use the two filters below instead.** - **assertion gaps** — JaCoCo is green and nothing checks the answer. `MainActivity`'s rail and bottom bar were both *executed* by `AppRootRestorationTest` and **transposing them passed the entire suite**; so did swapping the two progress-notification strings, and swapping `Content`'s two destinations. No coverage number would ever have found any of the three. + **Wave 4 (2026-09-02) corrected that first filter, and the correction is the reusable part.** + `mi > 0` fails in both directions. It *over-reports* on Compose: `JoinScreen.kt:222` reads + `mi=10` and also `ci=38`, and `JoinStateAffordancesTest` already clicks that Save button and + asserts `save:joined.mp4` — the missed instructions are the synthesized `$changed`/`$dirty` + recomposition-skip path, the same codegen this file already warns about for *branch* counts, + showing up in the instruction count too. And it *under-reports* on warm methods with cold arms: + `ConversionViewModel.cancel()` misses no line, yet `activeWorkId?.let(...)` had only ever been + entered on the null side in 584 tests. Use two filters together instead: + + - **`ci == 0`** — the line never executed. This is JaCoCo's own missed-line definition, so it + totals exactly the reported missed-line count and needs no judgement. + - **`ci > 0 && mb > 0` at method level** — a covered method with an arm nothing takes. This is + the only one that finds the `cancel()` shape. + + Of wave 4's 251 missed branches, just **18** sat on lines that do execute, so the branch gap and + the line gap are largely the same gap; the second filter is about which of them are reachable. + So **every ticket named the mutation that had to go red, and that was its acceptance criterion rather than a coverage delta**. It caught **two vacuous tests written in the same session**, before either shipped: @@ -236,6 +254,35 @@ install for code that can never run — and on API 37 the full APK does not fit And **re-measure before quoting**: this entry was once written quoting 81.4%, measured four hours earlier, and was already three points stale by the time it was ready to merge. + + **Wave 4's read (2026-09-02) moved no number at all, and that is its result.** It was a triage + rather than a test push: twelve tickets (**#192-#203**), four deferred candidates (**#204**), and + five findings (**F6-F10** in `docs/coverage-read-findings.md`). What it establishes is the shape + of what is left, which is different again from wave 3's: + + - Of 169 never-executed lines, **81 are native or device edges and stay that way** — + `FFmpegEngine` 33, `Media3Engine` 24, `ConcatEngine` 14, `MainActivity.onCreate` 10 — their + zeroes being the `testDebugUnitTest`-only measurement boundary that #84, #85, #86 and #88 each + recorded before. A further **34 are device-bound only until a seam moves them**: + `AndroidDeviceCodecs` 20 (#194) and the 14 of `MediaProbe`'s 26 that are `readMediaInformation` + (#195). Do not read that second group as exempt — the two tickets exist because it is not. + - Most of the rest is **already closed with a reason on record**, or compiler-generated: default-arg + bridges, DI factory lambdas, synthetic `NoWhenBranchMatchedException` arms, coroutine completion. + - Six of the ten findings in that document are now "no action" or "not a test gap". By this point + the report's remaining red is mostly arms nothing can reach, members nothing calls, and arms a + test *can* reach but cannot pin — and a coverage number tells none of them apart. + + **The biggest single gap it found was not a missed line.** `ConversionViewModel.cancel()` and + `JoinViewModel.cancel()` report every line covered; only the null arm of + `activeWorkId?.let(workManager::cancelWorkById)` had ever been entered, so nothing in 584 tests + connected the Cancel button to WorkManager (#192). That is what the second filter above is for. + + It also re-opened a mechanism, not a close: #86 and #133 ruled `AndroidDeviceCodecs.probe()` out + **through `ShadowMediaCodecList`**, on the grounds that the builder cannot set `isAlias` or + `canonicalName`. A pure seam does not have that constraint, and #133 did not evaluate one. Read + #194 before re-arguing either way — and note the reason it is worth cutting is not coverage but + that the `runCatching` fallback logs "assuming permissive" while returning empty sets, which makes + `canEncode` and `canDecode` answer *no* for everything. - **Testable code is not done until it is tested.** If a piece is unit testable, it gets unit tests before it counts as done. If it is e2e testable, it gets e2e tests. Both clauses apply — a change that is both needs both. diff --git a/docs/coverage-read-findings.md b/docs/coverage-read-findings.md index 5a54e45..eecf91a 100644 --- a/docs/coverage-read-findings.md +++ b/docs/coverage-read-findings.md @@ -1,22 +1,27 @@ # Coverage-read findings -**Status:** five findings, none fixed, none urgent. F5 was added on 2026-08-27, found while decomposing #132 into children — it had been listed there as a test gap, and is not one. Every entry here is a *code* observation — -something a test would document rather than repair. The test gaps found in the same read are -tickets #132 and #133, not entries here; see [Not covered here](#not-covered-here). -**Scope:** what a JaCoCo read on 2026-08-26 turned up that writing a test would not fix. This is -a survey, not a work order. Acting on any entry is a separate decision and would be its own commit. -**Last verified:** `main` at `dc8b7c3`, 2026-08-26. Coverage re-measured that day with -`./gradlew :app:jacocoTestReport`: **84.9% line (1971/2321), 63.8% branch (900/1410)**, against -**456 JVM tests in 68 classes**. `CLAUDE.md` quotes 454 in 67 from four hours earlier; the -percentages are unchanged, so no figure there is stale. +**Status:** ten findings, none fixed, none urgent. F1-F4 came from the 2026-08-26 read; F5 was added +on 2026-08-27 while decomposing #132; **F6-F10 were added on 2026-09-02 from the wave-4 read**. Every +entry here is a *code* observation — something a test would document rather than repair. The test +gaps found in the same reads are tickets, not entries here; see [Not covered here](#not-covered-here). +**Scope:** what a JaCoCo read turned up that writing a test would not fix. This is a survey, not a +work order. Acting on any entry is a separate decision and would be its own commit. +**Last verified:** `main` at `54ca2dd`, 2026-09-02. Coverage measured that day with +`./gradlew :app:jacocoTestReport`: **92.8% line (2183/2352), 81.3% branch (1091/1342)**, against +**584 JVM tests in 87 classes**, matching what `CLAUDE.md` quotes. + +The wave-4 read that produced F6-F10 also produced twelve test tickets, **#192-#203**, plus **#204** +for four candidates whose cost was not obviously worth paying. The split between them is the same one +this document has always drawn: a ticket is where a test goes, an entry here is where a test would not +help. ## Why this document is separate from `defect-audit.md` `defect-audit.md` is the record of the 2026-08-22 defect sweep: sixteen entries, each a thing that is *wrong at runtime*. Nothing here is wrong at runtime today. These are arms that cannot be -reached, accessors nobody calls, and one KDoc that contradicts the code beside it — the category -`defect-audit.md` calls **latent**, plus one that is not a defect at all and is recorded so the -next coverage read does not re-file it. +reached, accessors nobody calls, and two KDocs that contradict the code beside them — the category +`defect-audit.md` calls **latent**, plus several that are not defects at all and are recorded so the +next coverage read does not re-file them. They are here rather than in that document because folding them in would inflate a sixteen-entry audit whose status metadata has already gone stale once, and because they share a provenance: @@ -24,7 +29,7 @@ every one fell out of reading a coverage report, and every one is the kind of th report is *good* at surfacing and a test is bad at fixing. F5 is the clearest case — it was filed as a test gap first, and only stopped being one when someone went looking for its callers. -Entry ids are `F1`–`F5` so they cannot be confused with `defect-audit.md`'s `D1`–`D16`. +Entry ids are `F1`–`F10` so they cannot be confused with `defect-audit.md`'s `D1`–`D16`. ## How to read the confidence labels @@ -262,6 +267,179 @@ no way to make it happen now. --- +## F6 — Four more arms that cannot be reached, and one KDoc among them that is false + +**Severity: low · Confirmed by inspection · F4's family, found in the wave-4 read** + +``` +app/src/main/java/org/libremediaconverter/model/ConversionRouter.kt:178-179 +app/src/main/java/org/libremediaconverter/model/ConversionRouter.kt:221 +app/src/main/java/org/libremediaconverter/model/ContainerCapabilities.kt:297 +app/src/main/java/org/libremediaconverter/model/ContainerCapabilities.kt:324, :340 +``` + +Four sites that a coverage report flags and that no test can reach. Each is recorded with the +upstream guard that makes it unreachable, because that guard is what would have to change first. + +- **`ConversionRouter:178-179`** — the missed branch is `orEmpty()`'s absent-key arm on + `MEDIA3_MUXABLE_VIDEO[plan.container]`. `MEDIA3_CONTAINERS` is `setOf(MP4)` and `route()` returns at + `:104` for anything else, so `media3CanMux` only ever sees MP4, which both maps key. Same function + as F4's second pair, one line below it. +- **`ConversionRouter:221`** — `DeviceCodecs.PERMISSIVE.canDecode` returning **false** for + `InputProbe.UNPARSEABLE`. `PERMISSIVE` has no production caller at all (tests only), and the + router's one `canDecode` call at `:128` is already preceded by `:117` returning FFMPEG for + `UNPARSEABLE`. **Its KDoc at `:214-217` is false as written:** + + > That exception matters: a device double that claims it can decode an unparseable file would let + > the router send a doomed job to Media3. + + It would not — `:117` already caught it. This is F2's shape: a comment that describes a hazard the + code upstream has removed. Correcting it is a one-line change and should not be bundled with + anything. +- **`ContainerCapabilities:297`** — `if (container == GIF || container == IMAGE_SEQUENCE) return null` + in `repair`. `repair`'s only caller is `suggestions` (`:281`); `validate` returns at `:121` for + `isImageOutput` (which is exactly GIF ∥ IMAGE_SEQUENCE) before `suggestions` is reached, and + `firstContainerHolding` filters on `CARRIES_VIDEO`, which is empty for both. +- **`ContainerCapabilities:324` and `:340`** — the `else ->` arms themselves are exercised; what is + missed is the elvis tail, `firstOrNull() ?: VideoCodec.NONE` / `?: AudioCodec.NONE`. Reaching it + needs a container with no encodable codec on that axis. Audio-only containers return early at + `:307`, and the only containers with an empty audio set are GIF and IMAGE_SEQUENCE, excluded at + `:297` above. + +**Recorded so the next read does not re-file them.** F4's rule applies unchanged: a second line of +defence that can be provoked is not a second line of defence, and widening a private function to make +one reachable buys a test that asserts a fallback fires when called in a way production cannot call +it. + +--- + +## F7 — `probeWithExtractor`'s catch is unreachable for the same measured reason `probeForConcat`'s is + +**Severity: n/a · No action · completes a measurement already on record** + +``` +app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt:180-182 +``` + +```kotlin +} catch (e: Exception) { + Log.i(TAG, "Platform extractor could not read $uri.", e) + null +} +``` + +`CLAUDE.md` records the measurement for the *other* extractor site: Robolectric's `MediaExtractor` +never throws from `setDataSource`, checked across an unregistered `content://` authority, a missing +`file://`, a file of garbage bytes and an `http://` URL — all four returned with `trackCount = 0`. + +`probeWithExtractor` calls the same overload, three lines apart in the same file, and the measurement +covers it identically. It was simply not written down for this site, so a future read would re-derive +it. It stays device-only, alongside `probeForConcat`'s. + +**Two neighbouring line counts are artifacts of this, not separate gaps.** `MediaProbe:184` and +`:331` each report 27 missed instructions and are the `finally` block's synthetic exception-path copy +— JaCoCo duplicates a `finally` per exit path, and the exceptional one is unreachable for the reason +above. Do not read them as a third and fourth site. + +--- + +## F8 — Three more dead members, and six unused defaults + +**Severity: low · Confirmed by inspection · F3's family** + +``` +app/src/main/java/org/libremediaconverter/model/CopyPlanner.kt:28 ConversionPlan.hasVideo +app/src/main/java/org/libremediaconverter/codec/AndroidDeviceCodecs.kt:39 hardwareEncoders() +app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt:30 Result.output +app/src/main/java/org/libremediaconverter/convert/Transcoders.kt:28, :29, :40, :61 +app/src/main/java/org/libremediaconverter/work/Reattachment.kt:28, :30 +``` + +- **`ConversionPlan.hasVideo`** — zero callers in `main`, `test` or `androidTest`. Every `hasVideo` + hit in the tree is `InputProbe.hasVideo`, `OutputSpec.hasVideo` or `Container.extensionFor(hasVideo)`, + which are different properties on different types. A test asserting + `plan.hasVideo == (plan.video != VideoPlan.Drop)` is vacuous by construction. +- **`AndroidDeviceCodecs.hardwareEncoders()`** — its only caller is `RealMediaBenchmark`, in + `androidTest`. Production reads capabilities through `DeviceCodecs`, never the raw set. +- **`ConcatEngine.Result.output`** — `ConcatWorker` reads `result.strategy` and uses the `staged` + file it passed in, never `.output`. +- **`Transcoders.kt`'s default arguments** — `request` and `onProgress` on + `HardwareTranscoder.transcode` (`:28`, `:29`), `onProgress` on `SoftwareTranscoder.run` (`:40`), + and `format` on `ConcatJoiner.join` (`:61`). All three production call sites + (`ConversionWorker.kt:208`, `:234`, `ConcatWorker.kt:79`) pass every argument, so the synthesised + `$default` bridges and `$DefaultImpls` copies are never entered. The + `request: ConversionRequest = ConversionRequest(OutputFormat.MP4_H265.spec)` default is the one + worth a second look: nothing anywhere omits it, so an interface silently promises H.265 to a + caller that does not exist. +- **`JobSnapshot`'s `outputModifiedAt` and `tags` defaults** — `JobSnapshots.kt:32-42` passes all + seven fields, so the synthesised `$default` constructor (20 missed instructions at + `Reattachment.kt:14`) is never entered. + +**Not a test gap, for F3's reason.** Delete them, or keep them and know they are unused; either is a +decision, and a test restating the compiler is not. + +--- + +## F9 — Both workers' `getForegroundInfo` overrides are dead, and this is why + +**Severity: n/a · No action · sharpens #88 rather than reopening it** + +``` +app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt:342-346 +app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt:132-136 +``` + +**#88 already closed on these**, after reading both and finding no decision worth a seam — the +correct call, and it stands. What #88 did not name is the reason they are cold in the first place, +which is stronger than "the JVM cannot reach them": + +WorkManager calls `getForegroundInfoAsync()` **only for expedited work**. `ConversionWorker`'s own +KDoc says expedited is deliberately not used, and `grep -rn 'setExpedited\|OutOfQuotaPolicy' app/src` +returns nothing. So both overrides are dead in production today, not merely untested — a test would +assert the shape of something nothing invokes. + +They are still correct to keep: `ForegroundInfo` is required by the `CoroutineWorker` contract and +`setForeground` is called explicitly elsewhere. **What would reopen this** is the same trigger #88 +named — a `getForegroundInfo` that starts branching — plus one more: the day anything calls +`setExpedited`. + +--- + +## F10 — Three arms that are reachable, uncovered, and cannot be made to bite + +**Severity: n/a · No action · the shape a coverage number cannot distinguish** + +``` +app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt:550, :553 +app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt:349, :352, :278 +``` + +F4 and F6 hold arms that cannot be *reached*. These can — and a test written against them would still +pass under the mutation that ought to redden it, which is the harder case to spot and the more +expensive one to discover halfway through writing the test. + +- **`observer?.cancel()`'s non-null arm** (`ConversionViewModel:550`, `JoinViewModel:349`). Reachable + by calling `convert()` twice. But `ScreenOwnership`'s token is what actually blocks the superseded + write — the ViewModel's own KDoc at `reset()` says the cancel is "a request honoured at the next + suspension point" and "the claim is what actually stops that write". Delete `observer?.cancel()` + and the suite stays green, correctly. +- **`if (info == null) return@collect`** (`ConversionViewModel:553`, `JoinViewModel:352`). Reachable + through `pruneWork()`. But when the null arrives the state is already terminal, so removing the + guard crashes the collector and **leaves the state unchanged** — a state assertion is green under + the mutation. The only observable is an escaped coroutine exception, which the ViewModel's own KDoc + documents as unreliable on the JVM: kotlinx-coroutines-test's process-wide collector hands it to + whichever `runTest` starts next. +- **`JoinViewModel:278`'s `Ambiguous` arm.** Looks like the twin of `ReattachGuardsTest`'s "a result + two jobs both claim", and is not. An `Ambiguous` requires a shared `outputPath`, so it can only be a + *finished* job — which maps to `Joined`, a state that reads nothing from `inputs`. **The Convert-side + twin does bite**, because `displayNameOf(tags)` reaches the file card; the asymmetry is the point. + +**Recorded because each of these was picked up as a candidate and put down again.** The wave-4 read +lost time to all three before the mutation test was run in the head rather than the editor, which is +the cheaper order. + +--- + ## Summary | ID | Finding | Severity | Evidence | Action | @@ -271,12 +449,23 @@ no way to make it happen now. | F3 | `ConversionRequest.videoCodec` / `.audioCodec` have no callers | low | confirmed by inspection | delete, or keep for symmetry — **not** a test gap | | F4 | Two private guards reachable only by direct call | n/a | confirmed by inspection | **no action** — named exemption, per #88 | | F5 | `ConversionNotifications.areEnabled()` is never called | low | confirmed by inspection; grep returns the declaration only | **decide**: act on it or delete it — **not** a test gap | +| F6 | Four more unreachable arms; `ConversionRouter:214-217`'s KDoc is false | low | confirmed by inspection; each traced to its upstream guard | **no action**, except the one-line KDoc fix | +| F7 | `probeWithExtractor`'s catch is unreachable, as `probeForConcat`'s is | n/a | measured across four URI shapes (recorded in `CLAUDE.md`) | **no action** — device-only, now written down for both sites | +| F8 | Three more dead members and six unused defaults | low | confirmed by inspection; grep per member | delete or keep knowingly — **not** a test gap | +| F9 | Both `getForegroundInfo` overrides are dead: expedited work is never used | n/a | confirmed by inspection; `grep setExpedited` returns nothing | **no action** — sharpens #88's close | +| F10 | Three reachable arms where no mutation bites | n/a | confirmed by inspection; each mutation traced to its masking guard | **no action** — recorded to stop the next read re-picking them | Order, if these are acted on: **F1 and F5 first, separately.** They are the two with a possible user-visible answer — a format the app can produce and does not offer, and a warning the app -documents and does not give — and either answer changes what the tidying should look like. F2 and F3 -are tidying and belong in one commit with each other, not with F1 or F5. F4 is finished by being -written down. +documents and does not give — and either answer changes what the tidying should look like. F2, F3 and +F8 are tidying and belong in one commit with each other, not with F1 or F5. F6's KDoc correction is a +third kind: one line, no decision, and it should not wait on the tidying. F4, F7, F9 and F10 are +finished by being written down. + +**Six of the ten are now "no action" or "not a test gap", and that is the useful shape.** By wave 4 +the report's remaining red is mostly this: arms nothing can reach, members nothing calls, and arms a +test can reach but not pin. A coverage number cannot tell any of them from a real gap, which is why +this document exists and why it grows faster than the percentage moves. **F1 and F5 share a shape worth naming:** both are places where a comment describes behaviour the code does not have, and in both the tempting fix (delete the dead arm, test the dead method) would @@ -287,7 +476,15 @@ freeze the wrong answer in place. The decision comes first. **The test gaps from the same read.** Seven JVM-side gaps (**#132**) and three seam questions (**#133**) came out of this coverage read and are tracked there, because they are work rather than observations. This document holds only what a test would not fix. #133 also records why -`AndroidDeviceCodecs.probe()` was considered and left out, so that spike is not run a third time. +`AndroidDeviceCodecs.probe()` was considered and left out **through `ShadowMediaCodecList`**, so that +spike is not run a third time. + +**Updated 2026-09-02:** #194 proposes reaching the same code through a *pure seam* instead, which is a +different mechanism and one #133 did not evaluate — the builder objection it turns on (no +`setIsAlias`, no `setCanonicalName`) does not apply to a function taking its own entry type. #133's +close stands for the shadow; it is not a close on the seam. #194 also carries the reason the seam is +worth cutting at all, which is not coverage: the `runCatching` fallback logs "assuming permissive" and +returns empty sets, which makes `canEncode` and `canDecode` answer *no* for everything. **`ConversionForegroundType.current()`**, which looked like the sharpest gap in the read and is not. Its API 33 and 34 arms are cold on the JVM, but issue **#88** already established that the class is @@ -321,6 +518,25 @@ the real ones — **34 of 383** and **20 of 143** missed — and the screens are better-covered files in the repo, which is what #52, #57 and #61 were for. **Do not chase the branch number here.** If a future read wants a screen metric, use lines. +**Updated 2026-09-02: the same codegen inflates the *instruction* count, which wave 3's filter did +not allow for.** Wave 3 selected candidates on `mi > 0` — at least one missed instruction — which was +right to prefer over a bare branch count and is still wrong on these files. `JoinScreen.kt:222` reads +`mi=10` and looks uncovered; it also reads `ci=38`, and `JoinStateAffordancesTest` already clicks that +Save button and asserts `save:joined.mp4`. Every `onClick` lambda body flagged this way turned out to +be covered at method level, the missed instructions being the recomposition-skip path again. + +Use `ci == 0` — the line never executed, which is JaCoCo's own missed-line definition — and pair it +with a method-level `ci > 0 && mb > 0` pass for covered methods with cold arms. Neither filter alone +is enough: `ConversionViewModel.cancel()` misses no line at all, yet its non-null arm had never been +entered in 584 tests (#192). `CLAUDE.md`'s coverage entry carries the same correction. + +**Also codegen, also not gaps**, recorded once so they are not re-derived: the synthetic +`NoWhenBranchMatchedException` closing an exhaustive `when` (`ConverterScreen:399`, `:686`, +`JoinScreen:278`, `MainActivity:160`); the inner `is Idle -> Unit` arms at `ConverterScreen:253-254` +and `JoinScreen:158-159`, which are structurally unreachable because the outer `when` already routed +`Idle`; and the closing brace of a `launch` block whose `collect` never terminates +(`ConversionViewModel:578`, `JoinViewModel:371`). + **Anything requiring a device.** `MediaProbe`'s FFprobe half (`MediaProbe.kt:151, 156-158, 173-188`) and `FFmpegEngine` in full report 0% on the JVM and are covered by `androidTest`. JaCoCo measures `testDebugUnitTest` only; their zeroes are a boundary, as #84, #85, #86 and #88 each recorded