Merge remote-tracking branch 'origin/main' into test/readspec-enum-fallbacks

This commit is contained in:
2026-08-27 07:18:24 -05:00
4 changed files with 750 additions and 122 deletions
@@ -0,0 +1,234 @@
package org.libremediaconverter.convert
import android.content.ComponentName
import android.content.ContentProvider
import android.content.ContentValues
import android.content.Context
import android.content.IntentFilter
import android.content.pm.ProviderInfo
import android.database.Cursor
import android.database.MatrixCursor
import android.net.Uri
import android.os.Bundle
import android.provider.DocumentsContract
import android.provider.OpenableColumns
import org.robolectric.Robolectric
import org.robolectric.Shadows.shadowOf
import java.io.File
/**
* Content providers more than one test needs, and the registration dance they all repeat.
*
* Only that. A stub that serves one test stays in that test, next to the assertion it exists for —
* the rule `work/WorkerStubs.kt` states, and the reason `UnreliableOutputStream` is still private to
* `OutputPublisherPublishTest`.
*
* These started life inside `OutputPublisherPublishTest`, which is the only thing that needed a
* provider at all. They moved here when `InputQuery`'s cursor reads turned out to need the same
* provider answering *badly* — see [RowShape].
*/
internal const val DOCUMENTS_AUTHORITY = "org.libremediaconverter.test.documents"
internal const val PLAIN_AUTHORITY = "org.libremediaconverter.test.plain"
/**
* How [FakeSafProvider] answers a metadata query.
*
* A provider is another app. It can be uninstalled, revoke its grant, crash, or simply answer
* something the caller did not expect — and "answered something unexpected" is not one case but
* several, which is why this is an enum rather than a boolean.
*
* The distinction that matters most to callers is **null versus missing versus zero versus
* negative**. `InputQuery` exists to stop the last three being conflated: `hasSpaceFor(0)` is only
* "is there 128 MB free", so a size nobody could determine must not arrive as `0`, and
* `OutputPublisher.destinationIsKnownEmpty` must answer `false` — never "empty, go ahead and
* delete" — for every one of them.
*
* Column-level granularity is deliberate. `OutputPublisher` reads only `SIZE`; `InputQuery` reads
* both, and reaches a different answer depending on which one is bad.
*/
internal enum class RowShape {
/** What a healthy provider answers: the file's real name and real length. */
NORMAL,
/** A row is present and its `DISPLAY_NAME` cell is null. */
NULL_DISPLAY_NAME,
/** A row is present and its `SIZE` cell is null. */
NULL_SIZE,
/** The cursor carries no `DISPLAY_NAME` column at all — `getColumnIndex` gives `-1`. */
NO_DISPLAY_NAME_COLUMN,
/** The cursor carries no `SIZE` column at all — `getColumnIndex` gives `-1`. */
NO_SIZE_COLUMN,
/**
* A size of `-1`.
*
* Not a corrupt provider: it is what anything without a fixed length reports — a pipe, or a
* provider streaming its answer — and it is a third way of saying "unknown", distinct from a
* null cell and from a missing column.
*/
NEGATIVE_SIZE,
/**
* A cursor with the right columns and no rows in it.
*
* Distinct from returning `null`, which is what a provider that does not recognise the URI
* does. Both mean "no answer", and code that treats one as an answer and the other as an
* absence is wrong about one of them.
*/
NO_ROWS,
/**
* The query itself throws.
*
* A resolver call is a call into another app, and that app can have been uninstalled, revoked
* its grant, or simply crashed. `InputQuery.firstRow`'s KDoc is explicit that "a file picker is
* not a place to bring the process down from", so this is the shape that proves the guard is
* one.
*/
QUERY_THROWS,
}
/**
* A stand-in for the provider behind a SAF destination.
*
* It answers only what its callers ask of a document -- how many bytes are already there, what it
* is called, and delete it -- backed by a real file so the assertions are about the filesystem
* rather than about a mock's call log alone. The rest of the `ContentProvider` surface is stubbed.
*
* Writing is deliberately NOT routed through it. Robolectric's `ShadowContentResolver`
* consults its registered-stream map before it reaches any provider, which is what lets a
* test hand out a stream that writes some bytes and then fails -- a condition a real provider
* cannot be asked to produce on demand.
*/
internal open class FakeSafProvider : ContentProvider() {
override fun onCreate() = true
override fun query(
uri: Uri,
projection: Array<out String>?,
selection: String?,
selectionArgs: Array<out String>?,
sortOrder: String?,
): Cursor? {
if (rowShape == RowShape.QUERY_THROWS) throw SecurityException("provider revoked the grant")
val file = backingFile(uri)
if (!file.exists()) return null
return MatrixCursor(columnsFor(rowShape)).apply {
if (rowShape != RowShape.NO_ROWS) addRow(cellsFor(rowShape, file))
}
}
override fun call(method: String, arg: String?, extras: Bundle?): Bundle? {
if (method != METHOD_DELETE_DOCUMENT) return null
val target = extras?.getParcelable(EXTRA_URI, Uri::class.java) ?: return null
deleteRequests += target
deleteFailure?.let { throw it }
backingFile(target).delete()
return Bundle()
}
override fun getType(uri: Uri) = "video/mp4"
override fun insert(uri: Uri, values: ContentValues?): Uri? = null
override fun delete(uri: Uri, selection: String?, selectionArgs: Array<out String>?) = 0
override fun update(uri: Uri, values: ContentValues?, selection: String?, selectionArgs: Array<out String>?) = 0
companion object {
// DocumentsContract.METHOD_DELETE_DOCUMENT and EXTRA_URI are hidden from the public
// SDK, so they cannot be referenced. These are the wire names
// DocumentsContract.deleteDocument() actually sends, which is what a provider sees.
const val METHOD_DELETE_DOCUMENT = "android:deleteDocument"
const val EXTRA_URI = "uri"
/** Where the "documents" really live. Set per test to a Robolectric temp path. */
lateinit var root: File
/** Every delete this provider was asked for, in order. Empty is an assertion too. */
val deleteRequests = mutableListOf<Uri>()
/** Armed by the test that needs the cleanup itself to fail. */
var deleteFailure: RuntimeException? = null
/**
* How the next query answers. [reset] puts it back to [RowShape.NORMAL], so a test that
* does not care never has to think about it.
*/
var rowShape: RowShape = RowShape.NORMAL
fun backingFile(uri: Uri) = File(root, uri.lastPathSegment.orEmpty())
fun reset(directory: File) {
root = directory
deleteRequests.clear()
deleteFailure = null
rowShape = RowShape.NORMAL
}
private fun columnsFor(shape: RowShape): Array<String> = when (shape) {
RowShape.NO_DISPLAY_NAME_COLUMN -> arrayOf(OpenableColumns.SIZE)
RowShape.NO_SIZE_COLUMN -> arrayOf(OpenableColumns.DISPLAY_NAME)
else -> arrayOf(OpenableColumns.DISPLAY_NAME, OpenableColumns.SIZE)
}
private fun cellsFor(shape: RowShape, file: File): Array<Any?> = when (shape) {
RowShape.NO_DISPLAY_NAME_COLUMN -> arrayOf(file.length())
RowShape.NO_SIZE_COLUMN -> arrayOf<Any?>(file.name)
RowShape.NULL_DISPLAY_NAME -> arrayOf(null, file.length())
RowShape.NULL_SIZE -> arrayOf(file.name, null)
RowShape.NEGATIVE_SIZE -> arrayOf(file.name, UNKNOWN_LENGTH)
else -> arrayOf(file.name, file.length())
}
/** What `statSize` reports for anything without a fixed length. See [RowShape.NEGATIVE_SIZE]. */
private const val UNKNOWN_LENGTH = -1L
}
}
/**
* The same provider, registered WITHOUT the documents-provider intent filter.
*
* A separate class because the package manager keys providers by component name, so two
* authorities need two components. It exists to prove the guard is a guard: a content URI
* from something that is not a documents provider must not be handed to `deleteDocument`.
*/
internal class FakePlainProvider : FakeSafProvider()
/**
* Stands [provider] up on [authority] so `contentResolver` and the package manager both know it.
*
* `isDocumentUri()` does not look at the URI alone: it asks the package manager whether anything
* answers `ACTION_DOCUMENTS_PROVIDER` for that authority. Registering the provider with the
* resolver is not enough, which is the whole reason [asDocumentsProvider] is a parameter rather
* than always true — the negative case is a test.
*/
internal fun registerProvider(
context: Context,
provider: Class<out FakeSafProvider>,
authority: String,
asDocumentsProvider: Boolean,
) {
val info = ProviderInfo().apply {
this.authority = authority
packageName = context.packageName
name = provider.name
exported = true
grantUriPermissions = true
}
Robolectric.buildContentProvider(provider).create(info)
val packageManager = shadowOf(context.packageManager)
packageManager.addOrUpdateProvider(info)
if (asDocumentsProvider) {
packageManager.addIntentFilterForProvider(
ComponentName(context.packageName, provider.name),
IntentFilter(DocumentsContract.PROVIDER_INTERFACE),
)
}
}
@@ -0,0 +1,187 @@
package org.libremediaconverter.convert
import android.content.Context
import android.net.Uri
import org.junit.Assert.assertEquals
import org.junit.Assert.assertNull
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment
import java.io.File
/**
* What [InputQuery] makes of a metadata row.
*
* ## Why this is a separate file from `UnknownInputSizeTest`
*
* That test drives the case where **no provider is registered** — the query returns null and
* `measure()` answers instead — and it drives it thoroughly. What it never does is hand `InputQuery`
* a row. Before this file, nothing did: `firstRow`'s body, `displayNameOrNull` and `sizeOrNull` had
* never executed in the JVM suite, so every branch inside them was untested.
*
* ## What is actually being pinned
*
* Not "does it read a cursor" — that would pass against almost any implementation. The rule is that
* **a size nobody could determine must not arrive as a number**, and there are four separate ways a
* provider fails to determine one: a null cell, a missing column, a negative value, and no row at
* all. `InputQuery`'s KDoc states the stake:
*
* > a worker's input `Data` carries the size the *picker* found … `hasSpaceFor(0)` is only "is there
* > 128 MB free".
*
* So each of those four must produce `null`, and `null` specifically — not `0`, not `-1`. A test
* that asserted only "not the file's length" would pass on `0`, which is the exact conflation the
* class exists to end.
*
* ## Why every fall-through lands on null here
*
* [FakeSafProvider] does not implement `openFile`, so `measure()` cannot answer for these URIs
* either. That is deliberate: it isolates the cursor half. The other direction — the cursor says
* nothing and `measure()` succeeds — is `UnknownInputSizeTest`'s
* `a picked file no provider describes is measured rather than reported as empty`, and is not
* repeated here.
*
* ## What the mutations say, including the one that does not bite
*
* Measured against `MatrixCursor`, which is what these tests drive:
*
* | call on a null cell | result |
* |---|---|
* | `getString` | returns `null` |
* | `getLong` | returns **`0`** |
*
* That second row is why `sizeOrNull`'s `!isNull(it)` guard is load-bearing and why these tests
* bite: remove it and a null size arrives as `0`, a real number indistinguishable from an empty
* file, which is the precise conflation this class exists to end. Removing it reddens
* `a null size is unknown rather than zero`. Removing the trailing `takeIf { it >= 0 }` reddens
* `a negative size is unknown rather than reported`.
*
* **Named exemption: `displayNameOrNull`'s `!isNull(it)` guard is not pinned by anything here, and
* cannot be.** `getString` returns null for a null cell, so the fallback applies with or without
* the guard — removing it leaves every test in this file green. The guard is not redundant in
* production: `Cursor.getString`'s contract states that whether it throws on a null column is
* *implementation-defined*, and a real `ContentProvider` is free to throw where `MatrixCursor`
* returns null. It should stay. It simply cannot be falsified with this cursor, and saying so is
* better than implying `a null display name falls back without disturbing the size` covers it —
* that test pins the behaviour, not the guard.
*/
@RunWith(RobolectricTestRunner::class)
class InputQueryCursorTest {
private lateinit var context: Context
private lateinit var uri: Uri
@Before
fun setUp() {
context = RuntimeEnvironment.getApplication()
FakeSafProvider.reset(File(context.cacheDir, "picked").apply { mkdirs() })
registerProvider(context, FakeSafProvider::class.java, DOCUMENTS_AUTHORITY, asDocumentsProvider = true)
uri = Uri.parse("content://$DOCUMENTS_AUTHORITY/document/holiday.mp4")
FakeSafProvider.backingFile(uri).writeBytes(ByteArray(PAYLOAD_BYTES))
}
@Test
fun `a provider that answers properly supplies both the name and the size`() {
val described = InputQuery.describe(context, uri)
assertEquals("holiday.mp4", described.displayName)
assertEquals(PAYLOAD_BYTES.toLong(), described.sizeBytes)
}
@Test
fun `a null display name falls back without disturbing the size`() {
FakeSafProvider.rowShape = RowShape.NULL_DISPLAY_NAME
val described = InputQuery.describe(context, uri)
assertEquals(InputQuery.FALLBACK_DISPLAY_NAME, described.displayName)
// The two columns are read independently. A provider that cannot name the file can still
// size it, and losing the size here would be a bug the name assertion alone would miss.
assertEquals(PAYLOAD_BYTES.toLong(), described.sizeBytes)
}
@Test
fun `a cursor with no display name column falls back rather than throwing`() {
// getColumnIndex returns -1 rather than throwing, so the `it >= 0` guard is the only thing
// between this and an IllegalArgumentException out of getString.
FakeSafProvider.rowShape = RowShape.NO_DISPLAY_NAME_COLUMN
val described = InputQuery.describe(context, uri)
assertEquals(InputQuery.FALLBACK_DISPLAY_NAME, described.displayName)
assertEquals(PAYLOAD_BYTES.toLong(), described.sizeBytes)
}
@Test
fun `a null size is unknown rather than zero`() {
FakeSafProvider.rowShape = RowShape.NULL_SIZE
assertNull(unknownSizeMessage("a null cell"), InputQuery.sizeOf(context, uri))
}
@Test
fun `a cursor with no size column is unknown rather than zero`() {
FakeSafProvider.rowShape = RowShape.NO_SIZE_COLUMN
assertNull(unknownSizeMessage("a missing column"), InputQuery.sizeOf(context, uri))
}
@Test
fun `a negative size is unknown rather than reported`() {
// What anything without a fixed length reports -- a pipe, or a provider streaming its
// answer. Passing -1 through would be worse than passing 0: hasSpaceFor compares it
// against free space, so it would read as "needs less than nothing".
FakeSafProvider.rowShape = RowShape.NEGATIVE_SIZE
assertNull(unknownSizeMessage("a negative size"), InputQuery.sizeOf(context, uri))
}
@Test
fun `a cursor with no rows is unknown rather than zero`() {
// Distinct from the provider returning null, which UnknownInputSizeTest covers. A cursor
// that exists and holds nothing still has to reach the same answer.
FakeSafProvider.rowShape = RowShape.NO_ROWS
val described = InputQuery.describe(context, uri)
assertEquals(InputQuery.FALLBACK_DISPLAY_NAME, described.displayName)
assertNull(unknownSizeMessage("an empty cursor"), described.sizeBytes)
}
@Test
fun `a provider that throws is survived rather than propagated`() {
// The guard firstRow's KDoc exists for: "a resolver call is a call into another app ... and
// a file picker is not a place to bring the process down from". Without the runCatching,
// this SecurityException reaches the caller and takes the pick with it.
FakeSafProvider.rowShape = RowShape.QUERY_THROWS
val described = InputQuery.describe(context, uri)
assertEquals(InputQuery.FALLBACK_DISPLAY_NAME, described.displayName)
assertNull(unknownSizeMessage("a provider that threw"), described.sizeBytes)
}
@Test
fun `a join total is unknown when any one input could not be sized`() {
// The consequence the four cases above exist for, asserted once at the place it lands.
// Summing the inputs that did answer would produce a lower bound indistinguishable from a
// real total, which is what the space check cannot tell apart.
FakeSafProvider.rowShape = RowShape.NULL_SIZE
val unsizable = InputQuery.sizeOf(context, uri)
FakeSafProvider.rowShape = RowShape.NORMAL
val sizable = InputQuery.sizeOf(context, uri)
assertEquals(PAYLOAD_BYTES.toLong(), sizable)
assertNull(unsizable)
assertNull("one unknown input makes the whole total unknown", InputQuery.total(listOf(sizable, unsizable)))
}
private fun unknownSizeMessage(cause: String) =
"$cause means nobody could size the file; that must be null, not 0 -- hasSpaceFor(0) is only a headroom check"
private companion object {
const val PAYLOAD_BYTES = 4096
}
}
@@ -1,17 +1,7 @@
package org.libremediaconverter.convert
import android.content.ComponentName
import android.content.ContentProvider
import android.content.ContentValues
import android.content.Context
import android.content.IntentFilter
import android.content.pm.ProviderInfo
import android.database.Cursor
import android.database.MatrixCursor
import android.net.Uri
import android.os.Bundle
import android.provider.DocumentsContract
import android.provider.OpenableColumns
import org.junit.Assert.assertArrayEquals
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
@@ -20,7 +10,6 @@ import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.robolectric.Robolectric
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment
import org.robolectric.Shadows.shadowOf
@@ -31,91 +20,6 @@ import java.io.OutputStream
/** What a destination volume says when it fills up mid-write. */
private const val NO_SPACE = "No space left on device"
private const val DOCUMENTS_AUTHORITY = "org.libremediaconverter.test.documents"
private const val PLAIN_AUTHORITY = "org.libremediaconverter.test.plain"
/**
* A stand-in for the provider behind a SAF destination.
*
* It answers only what `publish()` asks of a destination -- how many bytes are already there,
* and delete it -- backed by a real file so the assertions are about the filesystem rather
* than about a mock's call log alone. The rest of the `ContentProvider` surface is stubbed.
*
* Writing is deliberately NOT routed through it. Robolectric's `ShadowContentResolver`
* consults its registered-stream map before it reaches any provider, which is what lets a
* test hand out a stream that writes some bytes and then fails -- the condition this whole
* file exists for, and one a real provider cannot be asked to produce on demand.
*/
internal open class FakeSafProvider : ContentProvider() {
override fun onCreate() = true
override fun query(
uri: Uri,
projection: Array<out String>?,
selection: String?,
selectionArgs: Array<out String>?,
sortOrder: String?,
): Cursor? {
val file = backingFile(uri)
if (!file.exists()) return null
return MatrixCursor(arrayOf(OpenableColumns.DISPLAY_NAME, OpenableColumns.SIZE)).apply {
addRow(arrayOf<Any?>(file.name, file.length()))
}
}
override fun call(method: String, arg: String?, extras: Bundle?): Bundle? {
if (method != METHOD_DELETE_DOCUMENT) return null
val target = extras?.getParcelable(EXTRA_URI, Uri::class.java) ?: return null
deleteRequests += target
deleteFailure?.let { throw it }
backingFile(target).delete()
return Bundle()
}
override fun getType(uri: Uri) = "video/mp4"
override fun insert(uri: Uri, values: ContentValues?): Uri? = null
override fun delete(uri: Uri, selection: String?, selectionArgs: Array<out String>?) = 0
override fun update(uri: Uri, values: ContentValues?, selection: String?, selectionArgs: Array<out String>?) = 0
companion object {
// DocumentsContract.METHOD_DELETE_DOCUMENT and EXTRA_URI are hidden from the public
// SDK, so they cannot be referenced. These are the wire names
// DocumentsContract.deleteDocument() actually sends, which is what a provider sees.
const val METHOD_DELETE_DOCUMENT = "android:deleteDocument"
const val EXTRA_URI = "uri"
/** Where the "documents" really live. Set per test to a Robolectric temp path. */
lateinit var root: File
/** Every delete this provider was asked for, in order. Empty is an assertion too. */
val deleteRequests = mutableListOf<Uri>()
/** Armed by the test that needs the cleanup itself to fail. */
var deleteFailure: RuntimeException? = null
fun backingFile(uri: Uri) = File(root, uri.lastPathSegment.orEmpty())
fun reset(directory: File) {
root = directory
deleteRequests.clear()
deleteFailure = null
}
}
}
/**
* The same provider, registered WITHOUT the documents-provider intent filter.
*
* A separate class because the package manager keys providers by component name, so two
* authorities need two components. It exists to prove the guard is a guard: a content URI
* from something that is not a documents provider must not be handed to `deleteDocument`.
*/
internal class FakePlainProvider : FakeSafProvider()
/**
* A sink that behaves like a volume filling up.
*
@@ -179,8 +83,8 @@ class OutputPublisherPublishTest {
fun setUp() {
context = RuntimeEnvironment.getApplication()
FakeSafProvider.reset(File(context.cacheDir, "destinations").apply { mkdirs() })
register(FakeSafProvider::class.java, DOCUMENTS_AUTHORITY, asDocumentsProvider = true)
register(FakePlainProvider::class.java, PLAIN_AUTHORITY, asDocumentsProvider = false)
registerProvider(context, FakeSafProvider::class.java, DOCUMENTS_AUTHORITY, asDocumentsProvider = true)
registerProvider(context, FakePlainProvider::class.java, PLAIN_AUTHORITY, asDocumentsProvider = false)
// SAF's CreateDocument contract hands back a document that already exists and is
// empty, so that is the state every destination starts in here.
@@ -326,28 +230,4 @@ class OutputPublisherPublishTest {
)
}
}
private fun register(provider: Class<out FakeSafProvider>, authority: String, asDocumentsProvider: Boolean) {
val info = ProviderInfo().apply {
this.authority = authority
packageName = context.packageName
name = provider.name
exported = true
grantUriPermissions = true
}
Robolectric.buildContentProvider(provider).create(info)
// isDocumentUri() does not look at the URI alone: it asks the package manager whether
// anything answers ACTION_DOCUMENTS_PROVIDER for that authority. Registering the
// provider with the resolver is not enough, which is the whole reason the negative
// case above can exist.
val packageManager = shadowOf(context.packageManager)
packageManager.addOrUpdateProvider(info)
if (asDocumentsProvider) {
packageManager.addIntentFilterForProvider(
ComponentName(context.packageName, provider.name),
IntentFilter(DocumentsContract.PROVIDER_INTERFACE),
)
}
}
}
+327
View File
@@ -0,0 +1,327 @@
# 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.
## 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.
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:
every one fell out of reading a coverage report, and every one is the kind of thing a coverage
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`.
## How to read the confidence labels
Same vocabulary as `defect-audit.md`, deliberately, so the two read alike:
- **Confirmed by inspection** — the control flow is fully readable and the finding follows from it.
- **Latent** — not reachable through today's UI, but wrong, and one change away from being live.
- **No action** — recorded because it looks like a finding and is not.
Nothing below was observed on a device, and nothing below needs to be: every entry is a claim about
what the code says, checkable by reading it.
---
## F1 — `FFmpegCommandBuilder` emits a Vorbis encoder that `ContainerCapabilities` says does not exist
**Severity: low · Latent · the more interesting reading is a missing feature, not dead code**
```
app/src/main/java/org/libremediaconverter/ffmpeg/FFmpegCommandBuilder.kt:188
app/src/main/java/org/libremediaconverter/model/ContainerCapabilities.kt:84-91
```
`FFmpegCommandBuilder.audioArgs` carries a live Vorbis arm:
```kotlin
AudioCodec.VORBIS -> listOf("-c:a", "libvorbis", "-q:a", "5")
```
`ContainerCapabilities` states, immediately above the set that governs it, that no such thing
exists:
> `/** Vorbis is absent for the same reason: nothing here emits a Vorbis encoder. */`
> `private val ENCODABLE_AUDIO = setOf(AAC, OPUS, MP3, FLAC, PCM)`
One of those two is wrong. The comment is the one that is wrong as written — something here does
emit a Vorbis encoder, twelve lines of `FFmpegCommandBuilder`.
### Why the arm is unreachable today
Traced, not assumed:
| step | where | effect |
|---|---|---|
| `validate` runs before routing | `ConversionWorker.kt:123` | a spec is checked on every job, however it was enqueued |
| `validateAudio` refuses non-encodable | `ContainerCapabilities.kt:246-251` | `VORBIS !in ENCODABLE_AUDIO` → `Invalid("This app cannot encode Vorbis audio.")` |
| the only spec→plan encode path | `CopyPlanner.kt:104` | `AudioPlan.Encode(requested)` — but `requested` cannot be Vorbis by the row above |
| the fallback encode path | `CopyPlanner.kt:112-115` | draws from `encodableAudio(container)`, itself filtered by `ENCODABLE_AUDIO` |
So `AudioPlan.Encode(VORBIS)` is not constructible through the app, and line 188 is dead.
### The reading that matters more
`CARRIES_AUDIO` lists Vorbis for WebM (`ContainerCapabilities.kt:62`) and OGG (`:67`). Because
`encodableAudio` filters through `ENCODABLE_AUDIO`, the picker offers **Opus and nothing else** for
WebM, and Opus/FLAC for OGG. FFmpeg on this device can encode Vorbis — the command is written and
correct — and the app declines to offer it.
So the honest framing is not "delete a dead arm". It is: **is `ENCODABLE_AUDIO`'s omission of
Vorbis a deliberate product call, or an accident that has been costing WebM/OGG users a format the
app already supports?** Nothing in the repo records that decision.
### The precedent for whichever way it goes
`Media3Engine.audioMimeTypeFor` has the *same* Vorbis arm, and handles it exactly right
(`Media3Engine.kt:221-233`): the KDoc names it dead, says why the arm stays anyway ("deleting a
right answer out of unreachable code buys nothing"), and points at `Media3EngineMimeTypesTest`,
which asserts which three of six codecs actually arrive — so the set moving fails a test rather
than surprising someone.
`FFmpegCommandBuilder`'s arm has none of that. Whatever is decided, the fix is to make the two
files agree and to say so in one place.
### What a fix has to decide
1. Whether Vorbis belongs in `ENCODABLE_AUDIO`. If yes, this is a feature and needs an e2e test
that produces a playable Vorbis file; if no, go to 2.
2. Correct the `ContainerCapabilities.kt:84` comment, which is false as written, and give the
`FFmpegCommandBuilder` arm the treatment `Media3Engine.kt:221-233` already models.
---
## F2 — `ConversionRequest.hardwareEncodeAvailable` is written, read by nothing, and its KDoc describes behaviour that was removed
**Severity: low · Confirmed by inspection**
```
app/src/main/java/org/libremediaconverter/model/OutputFormat.kt:211-219
app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt:117
```
The property is set on every request:
```kotlin
hardwareEncodeAvailable = devices.canEncode(spec.videoCodec),
```
`grep -rn 'hardwareEncodeAvailable' app/src/main` returns **that line and nothing else**. No
production code reads it. Its getter is one of three uncovered methods in `OutputFormat.kt`, which
is what surfaced it.
Its KDoc (`OutputFormat.kt:211-218`) explains at length what it is for:
> Knowing this lets the Fast tier choose a genuinely fast software preset instead of a mislabelled
> slow one.
`FFmpegCommandBuilder` no longer does that, and its own test says so —
`FFmpegCommandBuilderTest.kt:132`, `the encoder choice no longer depends on hardware availability`:
> Once FFmpeg stopped selecting MediaCodec encoders, this flag only affects whether the router sends
> the job to Media3 at all — not what FFmpeg does.
That second clause is also not true. `ConversionRouter` decides hardware encodability by calling
`device.canEncode(videoEncode)` itself (`ConversionRouter.kt:153`); it never reads
`request.hardwareEncodeAvailable`. The flag is computed from the same source the router
independently consults, carried through the request, and dropped.
This is the shape of open issue **#68** — a KDoc promising a switch that does not exist.
**Not harmful.** It costs one `canEncode` call per job and a field on a data class. It is recorded
because the KDoc actively misleads: a reader changing the Fast-tier preset logic would look here
first, and this is not where that decision lives.
### What a fix has to decide
Whether to delete the property (and the constructor parameter, and the four
`FFmpegCommandBuilderTest` call sites that pass it) or to keep it and rewrite the KDoc to say it is
vestigial. Deleting is cleaner; the test at `:132` is worth keeping either way, since it pins the
"FFmpeg does not select MediaCodec encoders" rule that the deletion would otherwise erase.
---
## F3 — `ConversionRequest.videoCodec` and `.audioCodec` have no callers anywhere
**Severity: low · Confirmed by inspection**
```
app/src/main/java/org/libremediaconverter/model/OutputFormat.kt:222-223
```
```kotlin
val container: Container get() = spec.container // used: FFmpegConcatCommand.kt:42, :80
val videoCodec: VideoCodec get() = spec.videoCodec // no callers
val audioCodec: AudioCodec get() = spec.audioCodec // no callers
```
Three delegating accessors on `ConversionRequest`; the first is used twice, the other two are used
nowhere in `main`, `test` or `androidTest`. Everything that wants those values reads
`request.spec.videoCodec` or takes the `OutputSpec` directly.
**This is not a test gap and must not be filed as one.** A test asserting
`request.videoCodec == request.spec.videoCodec` is vacuous by construction — it restates the
implementation and would pass against any delegation, right or wrong. That is precisely the failure
mode `CLAUDE.md` records from the mutation review (9 of 46 mutations vacuous, five over completely
unguarded paths).
The two accessors are either convenience worth keeping for symmetry with `container`, or two lines
to delete. Deleting them costs nothing and removes two uncovered methods that will otherwise be
re-found by every future coverage read.
---
## F4 — Two guards are reachable only by direct call, and that is correct
**Severity: n/a · No action**
```
app/src/main/java/org/libremediaconverter/ffmpeg/FFmpegCommandBuilder.kt:167-168
app/src/main/java/org/libremediaconverter/model/ConversionRouter.kt:175-176
```
```kotlin
VideoCodec.COPY, VideoCodec.NONE -> error("encodeVideo called for $codec, which is not an encode")
```
```kotlin
if (plan.video == VideoPlan.Copy && video == null) return false
if (plan.audio == AudioPlan.Copy && audio == null) return false
```
Both sit in private functions (`encodeVideo`, `media3CanMux`), and both are unreachable because a
caller upstream already excluded the case — which each says in its own comment. `ConversionRouter`'s
is labelled "the second line of defence"; `CopyPlanner` is the first.
**Recorded so the next coverage read does not treat them as gaps.** A second line of defence that
can be provoked is not a second line of defence. Making these reachable from a test would mean
widening the functions to `internal`, which buys a test that asserts an `error()` fires when called
in a way production cannot call it. This is the same judgement issue **#88** reached about
`getForegroundInfo` and closed on: naming the exemption rather than covering it.
Neither should change unless the upstream guard does. If `CopyPlanner` ever stops resolving `COPY`
before the builder sees it, `FFmpegCommandBuilder.kt:167` becomes live and wants a test that day.
---
## F5 — `ConversionNotifications.areEnabled()` is never called
**Severity: low · Confirmed by inspection · found while decomposing the test-gap ticket**
```
app/src/main/java/org/libremediaconverter/work/ConversionNotifications.kt:60-62
```
```kotlin
fun areEnabled(): Boolean = context.getSystemService(NotificationManager::class.java)
.areNotificationsEnabled()
.also { if (!it) Log.i(TAG, "Notifications disabled; progress will not be visible.") }
```
`grep -rn 'areEnabled' app/src` returns **that declaration and nothing else**. `ConversionNotifications`
is constructed in both workers (`ConversionWorker.kt:55`, `ConcatWorker.kt:35`) and only `build()` is
ever called on it.
**This entry exists because it was very nearly filed as a test gap.** Its three lines are cold on the
JVM, it has a KDoc explaining real user-visible stakes — a foreground service without
`POST_NOTIFICATIONS` shows only in the Task Manager, so progress silently vanishes — and Robolectric
can flip that permission in one line. Everything about it reads like a cheap, worthwhile test.
It is not, because **the behaviour the KDoc describes does not happen**. Nothing consults
`areEnabled()`, so nothing warns, degrades, or logs when notifications are off. A test would assert
that a function nobody calls returns what the platform told it — green, vacuous, and actively
misleading, since it would imply the app handles the disabled-notification case. That is the failure
mode `CLAUDE.md` records from the mutation review, reached from the opposite direction: not a test
that fails to bite, but a test with nothing to bite.
### What a fix has to decide
Whether the app should act on this at all. The KDoc argues it should — a conversion whose progress is
invisible is a real complaint, and `ConversionViewModel` or the worker's foreground start is where a
check would go. If yes, that is a **feature** with a test; if no, delete the method and the KDoc's
claim with it. What must not happen is a test that makes the current state look handled.
Related: **#16** is open on an adjacent gap — a user who *can* unblock a foreground-denied retry has
no way to make it happen now.
---
## Summary
| ID | Finding | Severity | Evidence | Action |
|---|---|---|---|---|
| F1 | `FFmpegCommandBuilder` emits a Vorbis encoder `ContainerCapabilities` says does not exist | low | confirmed by inspection; unreachability traced through four call sites | **decide**: feature or dead arm — the comment is false either way |
| F2 | `hardwareEncodeAvailable` written, never read; KDoc describes removed behaviour | low | confirmed by inspection; `FFmpegCommandBuilderTest:132` corroborates | **decide**: delete or mark vestigial |
| 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 |
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.
**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
freeze the wrong answer in place. The decision comes first.
## Not covered here
**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.
**`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
covered by `ConversionWorkerTest.foregroundTypeMatchesTheRunningApiLevel` across the CI matrix, and
that its 0% is the `testDebugUnitTest`-only measurement boundary.
The premise worth re-checking was whether the 33/34 legs still complete, given #122's wedge.
**They mostly do, and #122 is not resolved** — this entry said "they do" on first writing, from a
single green run, and the PR carrying this very document proved that wrong:
| run | API 33 leg | shape |
|---|---|---|
| `32933262839` (#127) | success, 7m16s | `expected 60, received 60, failed 0, completed cleanly: yes` |
| `33033036857` (PR #131, docs-only) | **failure, 23m08s** | `expected 60, received 60, failed unknown, wedged: yes — gradle killed after 1200s` |
Five of the last six completed API 33 legs passed in about seven minutes, so the wedge is
intermittent rather than systematic. **What it costs is the verdict, not the execution**: `received:
60` on the wedged run means all sixty tests still reported, so the API 33 regime *was* exercised —
but `failed:` reads `unknown`, so that leg could not have told anyone if it had broken.
That is why this stays a note and not a ticket, and also why it is not simply deleted: #88's
reasoning holds, but the leg it rests on cannot be relied on to report a failure. A
`@Config(sdk = 33)` / `@Config(sdk = 34)` JVM test would pin all three arms deterministically in one
run for about three lines. Small, and worth doing the next time this file is opened — but it is
insurance against a flaky leg, not the uncovered behaviour it first looked like.
**The Compose screens' branch coverage.** `ConverterScreenKt` reports 110 of 200 branches missed and
`JoinScreenKt` 60 of 82, which looks alarming and is not a signal: the Compose compiler synthesises
`$changed`/`$dirty` recomposition-skip tests that JaCoCo counts as branches. The line figures are
the real ones — **34 of 383** and **20 of 143** missed — and the screens are among the
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.
**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
before this.