Compare commits
1
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
  JMR-devandClaude Opus 5
|
da344b0fe4 |
@@ -184,29 +184,11 @@ 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:
|
||||
@@ -254,35 +236,6 @@ 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.
|
||||
|
||||
@@ -1,180 +0,0 @@
|
||||
package org.libremediaconverter.convert
|
||||
|
||||
import android.app.Application
|
||||
import androidx.media3.common.util.UnstableApi
|
||||
import androidx.work.OneTimeWorkRequestBuilder
|
||||
import androidx.work.WorkInfo
|
||||
import androidx.work.WorkManager
|
||||
import androidx.work.workDataOf
|
||||
import kotlinx.coroutines.Dispatchers
|
||||
import org.junit.After
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
import org.libremediaconverter.join.JoinState
|
||||
import org.libremediaconverter.join.JoinViewModel
|
||||
import org.libremediaconverter.model.InputProbe
|
||||
import org.libremediaconverter.work.ConcatWorker
|
||||
import org.libremediaconverter.work.ConversionWorker
|
||||
import org.libremediaconverter.work.JobTags
|
||||
import org.robolectric.RobolectricTestRunner
|
||||
import org.robolectric.RuntimeEnvironment
|
||||
import java.util.UUID
|
||||
import java.util.concurrent.TimeUnit
|
||||
|
||||
/**
|
||||
* That `cancel()` cancels the job, on both screens.
|
||||
*
|
||||
* ## Why this was missing, which is the interesting part
|
||||
*
|
||||
* Both `cancel()` methods are one line — `activeWorkId?.let(workManager::cancelWorkById)` — and
|
||||
* **JaCoCo reports every line of both as covered**. `SettingsEditsTest`'s
|
||||
* `cancelling with no active job does nothing rather than throwing` runs the method, and its own
|
||||
* comment names which half it drives: "`activeWorkId?.let(...)` -- the null side". The other side
|
||||
* had never been entered, and `JoinViewModel.cancel()` had no test at all.
|
||||
*
|
||||
* So no line-level coverage filter could see this. What surfaces it is a method-level read —
|
||||
* `mi=11, ci=7, mb=1, cb=1` on both — a covered method with an arm nothing takes. That is the
|
||||
* second of the two filters #194 records, and this is the gap that argued for it.
|
||||
*
|
||||
* The affordance tests are not this. `ConverterStateAffordancesTest` and `JoinStateAffordancesTest`
|
||||
* click `TestTags.CANCEL` and assert the *action* fires into a stub; `ScreenWiringTest` asserts the
|
||||
* action calls `viewModel.cancel()`. Both halves were pinned and the join between them was not, so
|
||||
* nothing in 584 tests connected the button to WorkManager.
|
||||
*
|
||||
* ## Why the job is enqueued with a delay
|
||||
*
|
||||
* The test WorkManager runs on a `SynchronousExecutor`, so an ordinary request finishes inline —
|
||||
* which is exactly why only the null half was ever covered: by the time a test could call
|
||||
* `cancel()`, `convert()`'s job was already terminal. `setInitialDelay` is what `TestScheduler`
|
||||
* honours, so the job sits in `ENQUEUED` until the test lets it go, and it never does.
|
||||
*
|
||||
* **Production never sets a delay**, so the request is built here rather than through
|
||||
* `ConversionWorker.request`. The *state* is not synthetic: `ENQUEUED` at `runAttemptCount == 0` is
|
||||
* what every job passes through before the scheduler picks it up, `Reattachment.choose` ranks it
|
||||
* `QUEUED`, and `conversionStateFrom` maps it to `Converting(input, 0)`. The delay changes how long
|
||||
* the job stays in a real state, not which state it is in.
|
||||
*
|
||||
* ## What is asserted, and in which order
|
||||
*
|
||||
* WorkManager's own record first, then the screen. The screen alone would be a weaker claim than it
|
||||
* looks: `CANCELLED` maps to `Idle` for a reattached job, and `Idle` is also where a ViewModel that
|
||||
* did nothing at all would sit.
|
||||
*/
|
||||
@UnstableApi
|
||||
@RunWith(RobolectricTestRunner::class)
|
||||
class CancelReachesWorkManagerTest {
|
||||
|
||||
private lateinit var app: Application
|
||||
private lateinit var workManager: WorkManager
|
||||
|
||||
@Before
|
||||
fun setUp() {
|
||||
app = RuntimeEnvironment.getApplication()
|
||||
ConversionDependencies.publisher = { RecordingPublisher(app) }
|
||||
ConversionDependencies.probe = { _, _ -> InputProbe() }
|
||||
installTestWorkManager(app, workDataOf())
|
||||
workManager = WorkManager.getInstance(app)
|
||||
}
|
||||
|
||||
@After
|
||||
fun tearDown() {
|
||||
ConversionDependencies.reset()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `cancelling a queued conversion cancels that job`() {
|
||||
val id = enqueueQueuedConversion()
|
||||
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
|
||||
awaitState(viewModel.state, "Converting") { it is ConversionState.Converting }
|
||||
|
||||
viewModel.cancel()
|
||||
|
||||
assertEquals(
|
||||
"Cancel must reach WorkManager, not just the screen",
|
||||
WorkInfo.State.CANCELLED,
|
||||
stateOf(id),
|
||||
)
|
||||
awaitState(viewModel.state, "Idle") { it is ConversionState.Idle }
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `cancelling a queued join cancels that job`() {
|
||||
val id = enqueueQueuedJoin()
|
||||
val viewModel = JoinViewModel(app, Dispatchers.Unconfined)
|
||||
awaitState(viewModel.state, "Joining") { it is JoinState.Joining }
|
||||
|
||||
viewModel.cancel()
|
||||
|
||||
assertEquals(
|
||||
"Cancel must reach WorkManager, not just the screen",
|
||||
WorkInfo.State.CANCELLED,
|
||||
stateOf(id),
|
||||
)
|
||||
awaitState(viewModel.state, "Idle") { it is JoinState.Idle }
|
||||
}
|
||||
|
||||
/**
|
||||
* The negative that bounds both: cancelling must cancel the job the screen is showing, and only
|
||||
* that one.
|
||||
*
|
||||
* Without this, `cancel()` could cancel everything in the queue — `cancelAllWork()` in place of
|
||||
* `cancelWorkById(activeWorkId)` — and both tests above would still pass.
|
||||
*/
|
||||
@Test
|
||||
fun `cancelling one conversion leaves another queued job alone`() {
|
||||
val bystander = enqueueQueuedConversion(displayName = "beach.mp4")
|
||||
val id = enqueueQueuedConversion(displayName = "holiday.mp4")
|
||||
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
|
||||
val converting = awaitState(viewModel.state, "Converting") { it is ConversionState.Converting }
|
||||
val onScreen = (converting as ConversionState.Converting).input.displayName
|
||||
|
||||
viewModel.cancel()
|
||||
|
||||
// Which of the two the ViewModel reattached to is the query's business, not this test's --
|
||||
// the comparator leaves queued jobs tied deliberately, per Reattachment's ordering notes.
|
||||
// So assert the shape rather than the identity: exactly one is cancelled, and the other is
|
||||
// untouched.
|
||||
val cancelled = listOf(id, bystander).filter { stateOf(it) == WorkInfo.State.CANCELLED }
|
||||
assertEquals(
|
||||
"exactly one job may be cancelled, with $onScreen on screen",
|
||||
1,
|
||||
cancelled.size,
|
||||
)
|
||||
}
|
||||
|
||||
private fun stateOf(id: UUID): WorkInfo.State =
|
||||
requireNotNull(workManager.getWorkInfoById(id).get()) { "no WorkInfo for $id" }.state
|
||||
|
||||
/**
|
||||
* A conversion sitting in the queue, which is where every job starts.
|
||||
*
|
||||
* Built by hand rather than through `ConversionWorker.request` for the reason in the class
|
||||
* KDoc; the display-name tag is included because `reattach()` reads it for the file card, and a
|
||||
* job without one would exercise the `UNKNOWN_INPUT_NAME` fallback instead of this test's
|
||||
* subject.
|
||||
*/
|
||||
private fun enqueueQueuedConversion(displayName: String = "holiday.mp4"): UUID {
|
||||
val request = OneTimeWorkRequestBuilder<ConversionWorker>()
|
||||
.addTag(JobTags.displayName(displayName))
|
||||
.setInitialDelay(QUEUE_HOLD_HOURS, TimeUnit.HOURS)
|
||||
.build()
|
||||
workManager.enqueue(request).result.get()
|
||||
return request.id
|
||||
}
|
||||
|
||||
private fun enqueueQueuedJoin(inputCount: Int = 2): UUID {
|
||||
val request = OneTimeWorkRequestBuilder<ConcatWorker>()
|
||||
.addTag(JobTags.inputCount(inputCount))
|
||||
.setInitialDelay(QUEUE_HOLD_HOURS, TimeUnit.HOURS)
|
||||
.build()
|
||||
workManager.enqueue(request).result.get()
|
||||
return request.id
|
||||
}
|
||||
|
||||
private companion object {
|
||||
/** Long enough that `TestScheduler` never releases the job during a test run. */
|
||||
const val QUEUE_HOLD_HOURS = 1L
|
||||
}
|
||||
}
|
||||
+17
-233
@@ -1,27 +1,22 @@
|
||||
# Coverage-read findings
|
||||
|
||||
**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.
|
||||
**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.
|
||||
|
||||
## 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 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.
|
||||
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.
|
||||
|
||||
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:
|
||||
@@ -29,7 +24,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`–`F10` so they cannot be confused with `defect-audit.md`'s `D1`–`D16`.
|
||||
Entry ids are `F1`–`F5` so they cannot be confused with `defect-audit.md`'s `D1`–`D16`.
|
||||
|
||||
## How to read the confidence labels
|
||||
|
||||
@@ -267,179 +262,6 @@ 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 |
|
||||
@@ -449,23 +271,12 @@ the cheaper order.
|
||||
| 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, 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.
|
||||
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.
|
||||
|
||||
**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
|
||||
@@ -476,15 +287,7 @@ 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 **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.
|
||||
`AndroidDeviceCodecs.probe()` was considered and left out, so that spike is not run a third time.
|
||||
|
||||
**`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
|
||||
@@ -518,25 +321,6 @@ 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
|
||||
|
||||
Reference in New Issue
Block a user