diff --git a/app/src/test/java/org/libremediaconverter/convert/FakeProviders.kt b/app/src/test/java/org/libremediaconverter/convert/FakeProviders.kt new file mode 100644 index 0000000..2d6b125 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/FakeProviders.kt @@ -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?, + selection: String?, + selectionArgs: Array?, + 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?) = 0 + + override fun update(uri: Uri, values: ContentValues?, selection: String?, selectionArgs: Array?) = 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() + + /** 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 = 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 = when (shape) { + RowShape.NO_DISPLAY_NAME_COLUMN -> arrayOf(file.length()) + RowShape.NO_SIZE_COLUMN -> arrayOf(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, + 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), + ) + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/InputQueryCursorTest.kt b/app/src/test/java/org/libremediaconverter/convert/InputQueryCursorTest.kt new file mode 100644 index 0000000..1b81616 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/InputQueryCursorTest.kt @@ -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 + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt index 88431f6..c670a67 100644 --- a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt @@ -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?, - selection: String?, - selectionArgs: Array?, - sortOrder: String?, - ): Cursor? { - val file = backingFile(uri) - if (!file.exists()) return null - return MatrixCursor(arrayOf(OpenableColumns.DISPLAY_NAME, OpenableColumns.SIZE)).apply { - addRow(arrayOf(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?) = 0 - - override fun update(uri: Uri, values: ContentValues?, selection: String?, selectionArgs: Array?) = 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() - - /** 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, 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), - ) - } - } } diff --git a/docs/coverage-read-findings.md b/docs/coverage-read-findings.md new file mode 100644 index 0000000..5a54e45 --- /dev/null +++ b/docs/coverage-read-findings.md @@ -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.