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/app/src/test/java/org/libremediaconverter/model/ContainerCapabilitiesTest.kt b/app/src/test/java/org/libremediaconverter/model/ContainerCapabilitiesTest.kt index daae30b..30a44e5 100644 --- a/app/src/test/java/org/libremediaconverter/model/ContainerCapabilitiesTest.kt +++ b/app/src/test/java/org/libremediaconverter/model/ContainerCapabilitiesTest.kt @@ -358,4 +358,120 @@ class ContainerCapabilitiesTest { assertEquals(emptyList(), ContainerCapabilities.encodableVideo(container)) } } + + // --- the audio axis ----------------------------------------------------- + // + // Every rule below has a video twin already tested above. The two halves of `validate` were + // written together and only one of them was ever checked, so these are deliberately shaped like + // their twins rather than as a fresh idea about what to assert. + + @Test + fun `an unidentifiable source audio codec cannot be copied`() { + // The audio twin of `an unidentifiable source codec cannot be copied`. Never guess: a copy + // of an unidentified codec is how you ship a file that does not play. + val unknownAudio = InputProbe(videoCodec = "h264", audioCodec = null, container = Container.MP4) + val spec = OutputSpec(Container.MP4, VideoCodec.H264, AudioCodec.COPY) + + val invalid = ContainerCapabilities.validate(spec, unknownAudio) as? Validation.Invalid + ?: throw AssertionError("copying an unidentified audio codec must be refused") + + assertTrue(invalid.message, invalid.message.contains("could not be identified")) + assertEverySuggestionValid(invalid, unknownAudio) + } + + @Test + fun `copying an audio codec the container cannot hold is refused`() { + // MP4 carries AAC, MP3, Opus and FLAC. Vorbis lives in Ogg and Matroska, so a stream copy + // out of a Vorbis source into MP4 has nowhere to put the track. + val vorbisAudio = InputProbe(videoCodec = "h264", audioCodec = "vorbis", container = Container.MKV) + val spec = OutputSpec(Container.MP4, VideoCodec.H264, AudioCodec.COPY) + + val invalid = ContainerCapabilities.validate(spec, vorbisAudio) as? Validation.Invalid + ?: throw AssertionError("Vorbis copied into MP4 must be refused") + + assertEquals("MP4 cannot hold Vorbis audio.", invalid.message) + assertEverySuggestionValid(invalid, vorbisAudio) + } + + @Test + fun `an audio codec the container cannot hold is refused on the encode path too`() { + // WAV carries PCM and nothing else. The twin is `H265 in AVI is refused`. + val spec = OutputSpec(Container.WAV, VideoCodec.NONE, AudioCodec.AAC) + + val invalid = ContainerCapabilities.validate(spec, mp3Source) as? Validation.Invalid + ?: throw AssertionError("AAC in WAV must be refused") + + assertEquals("WAV cannot hold AAC audio.", invalid.message) + assertEverySuggestionValid(invalid, mp3Source) + } + + @Test + fun `an audio codec this app cannot encode is refused, and copying is offered instead`() { + // Matroska carries Vorbis; nothing here encodes it. The refusal has to say so *and* say + // what would work, which is the audio twin of `copying is offered as the fix when the codec + // is right but unencodable`. + val spec = OutputSpec(Container.MKV, VideoCodec.H264, AudioCodec.VORBIS) + + val invalid = ContainerCapabilities.validate(spec, h264Source) as? Validation.Invalid + ?: throw AssertionError("encoding Vorbis must be refused") + + assertEquals( + "This app cannot encode Vorbis audio. It can still be copied from a Vorbis source.", + invalid.message, + ) + assertEverySuggestionValid(invalid, h264Source) + } + + @Test + fun `copying a video codec the container cannot hold is refused`() { + // Not the audio axis, but the one video refusal with no test: AVI predates H.265, so a + // stream copy out of an HEVC source into AVI has nowhere to put the track. `H265 in AVI is + // refused` covers the matrix; this covers what validate() does with it. + val h265Source = InputProbe(videoCodec = "hevc", audioCodec = "mp3", container = Container.MP4) + val spec = OutputSpec(Container.AVI, VideoCodec.COPY, AudioCodec.MP3) + + val invalid = ContainerCapabilities.validate(spec, h265Source) as? Validation.Invalid + ?: throw AssertionError("H.265 copied into AVI must be refused") + + assertEquals("AVI cannot hold H.265 video.", invalid.message) + assertEverySuggestionValid(invalid, h265Source) + } + + @Test + fun `no audio track is accepted by every container in both modes`() { + // The audio twin of VideoCodec.NONE -> true. A container that refused "no audio" would make + // every video-only output invalid. + Container.entries.forEach { container -> + listOf(CodecMode.COPY, CodecMode.ENCODE).forEach { mode -> + assertTrue( + "$container should accept no audio track ($mode)", + ContainerCapabilities.accepts(container, AudioCodec.NONE, mode), + ) + } + } + } + + @Test + fun `resolving audio COPY before asking the matrix is required`() { + // The audio twin of `resolving COPY before asking the matrix is required`, and the reason is + // identical: silently answering "false" would refuse a perfectly good remux. + runCatching { ContainerCapabilities.accepts(Container.MP4, AudioCodec.COPY, CodecMode.COPY) } + .onSuccess { throw AssertionError("expected audio COPY to be rejected by the matrix") } + } + + /** + * Every alternative a refusal offers has to be one the same input could actually take. + * + * `Validation.Invalid` promises exactly this and names this class as the proof. The global + * property test walks the presets; these paths reach `suggestions()` through `validateAudio`, + * which no preset does. + */ + private fun assertEverySuggestionValid(invalid: Validation.Invalid, probe: InputProbe) { + invalid.suggestions.forEach { + assertTrue( + "suggestion $it is itself invalid, so the chip leads to a second error", + ContainerCapabilities.validate(it, probe).isValid, + ) + } + } } diff --git a/app/src/test/java/org/libremediaconverter/work/DeniedForegroundStartTest.kt b/app/src/test/java/org/libremediaconverter/work/DeniedForegroundStartTest.kt index 2308a44..a8e4c9b 100644 --- a/app/src/test/java/org/libremediaconverter/work/DeniedForegroundStartTest.kt +++ b/app/src/test/java/org/libremediaconverter/work/DeniedForegroundStartTest.kt @@ -27,9 +27,6 @@ import org.robolectric.RobolectricTestRunner import org.robolectric.RuntimeEnvironment import java.io.File import java.util.UUID -import java.util.concurrent.ExecutionException -import java.util.concurrent.Executor -import java.util.concurrent.TimeUnit /** * That a refused foreground-service start does not end the job. @@ -125,6 +122,40 @@ class DeniedForegroundStartTest { ) } + @Test + fun `a join denied past the attempt bound fails with a message the user can act on`() { + // The join twin of the conversion case above. ConcatWorker reaches the same FailureOutcome + // through its own `when`, and that arm was the only one of its three with no test -- so a + // join that gave up silently, or gave up with an empty Data, would have looked identical to + // one that retried. + val worker = concatWorker(runAttemptCount = FailureOutcome.MAX_FOREGROUND_START_ATTEMPTS) + + val result = runBlocking { worker.doWork() } + + assertEquals( + ListenableWorker.Result.failure( + workDataOf(ConcatWorker.KEY_ERROR to FailureOutcome.FOREGROUND_DENIED_MESSAGE), + ), + result, + ) + } + + @Test + fun `a join that gives up collects the partial it had already staged`() { + // The delete lives on ConcatWorker's `catch (e: Throwable)` path, which every give-up goes + // through. Written first so a missing delete cannot pass by asking whether a file nobody + // wrote is absent. + concatStagedFile().writeBytes(ByteArray(PARTIAL_BYTES)) + + runBlocking { concatWorker(runAttemptCount = FailureOutcome.MAX_FOREGROUND_START_ATTEMPTS).doWork() } + + assertEquals( + "a join that gave up must not orphan what it staged", + emptyList(), + stagedNames(), + ) + } + private fun conversionWorker(runAttemptCount: Int = 0): ConversionWorker = TestListenableWorkerBuilder( context = app, @@ -141,18 +172,22 @@ class DeniedForegroundStartTest { .setForegroundUpdater(DenyingForegroundUpdater) .build() - private fun concatWorker(): ConcatWorker = TestListenableWorkerBuilder( + private fun concatWorker(runAttemptCount: Int = 0): ConcatWorker = TestListenableWorkerBuilder( context = app, inputData = workDataOf( ConcatWorker.KEY_INPUT_URIS to arrayOf(INPUT.toString(), "content://test/second.mp4"), ConcatWorker.KEY_TOTAL_BYTES to INPUT_BYTES, - ConcatWorker.KEY_FORMAT to OutputFormat.MP4_H264.name, + ConcatWorker.KEY_FORMAT to CONCAT_FORMAT.name, ), - runAttemptCount = 0, + runAttemptCount = runAttemptCount, ).setId(CONCAT_ID) .setForegroundUpdater(DenyingForegroundUpdater) .build() + /** The staging path the join will compute, asked for rather than spelled out here. */ + private fun concatStagedFile(): File = + publisher.createStagingFile(StagingNames.forJob(CONCAT_ID, CONCAT_FORMAT.extension)) + /** The staging path the worker will compute, asked for rather than spelled out here. */ private fun stagedFile(): File = publisher.createStagingFile(StagingNames.forJob(CONVERSION_ID, SPEC.extension)) @@ -164,6 +199,7 @@ class DeniedForegroundStartTest { const val INPUT_BYTES = 1024L const val PARTIAL_BYTES = 2048 val SPEC = OutputFormat.MP4_H265.spec + val CONCAT_FORMAT = OutputFormat.MP4_H264 val CONVERSION_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000001") val CONCAT_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000002") } @@ -182,18 +218,3 @@ private object DenyingForegroundUpdater : ForegroundUpdater { ), ) } - -/** - * An already-failed future, written out rather than pulled from a futures library. - * - * `await()` takes the `isDone` fast path and unwraps the `ExecutionException`, which is what puts - * the platform's own exception in front of the worker's catch rather than a wrapper. - */ -private class FailedFuture(private val failure: Throwable) : ListenableFuture { - override fun addListener(listener: Runnable, executor: Executor): Unit = executor.execute(listener) - override fun cancel(mayInterruptIfRunning: Boolean): Boolean = false - override fun isCancelled(): Boolean = false - override fun isDone(): Boolean = true - override fun get(): Void = throw ExecutionException(failure) - override fun get(timeout: Long, unit: TimeUnit): Void = throw ExecutionException(failure) -} diff --git a/app/src/test/java/org/libremediaconverter/work/RefusedJobTest.kt b/app/src/test/java/org/libremediaconverter/work/RefusedJobTest.kt new file mode 100644 index 0000000..f988c06 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/RefusedJobTest.kt @@ -0,0 +1,276 @@ +package org.libremediaconverter.work + +import android.app.Application +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.Data +import androidx.work.ListenableWorker +import androidx.work.testing.TestListenableWorkerBuilder +import androidx.work.workDataOf +import kotlinx.coroutines.runBlocking +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.convert.ConversionDependencies +import org.libremediaconverter.convert.OutputPublisher +import org.libremediaconverter.convert.SoftwareTranscoder +import org.libremediaconverter.convert.installTestWorkManager +import org.libremediaconverter.model.AudioCodec +import org.libremediaconverter.model.ContainerCapabilities +import org.libremediaconverter.model.ConversionRequest +import org.libremediaconverter.model.DeviceCodecs +import org.libremediaconverter.model.EnginePreference +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.model.OutputFormat +import org.libremediaconverter.model.OutputSpec +import org.libremediaconverter.model.Validation +import org.libremediaconverter.model.VideoCodec +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File +import java.util.UUID + +/** + * Jobs the worker refuses before it converts anything, and what it says about them. + * + * Two exits, both cold before this file, and both reachable for the same underlying reason: **a job + * does not have to come from the picker.** WorkManager keeps queued and finished work for about a + * week, so a downgrade or a rollback hands this build a job enqueued by another one — the premise + * `WorkerEnumFallbackTest` and `JobTags` are both written on — and `ConversionWorker.request(...)` + * is callable directly. + * + * What makes these worth their own file rather than another case in an existing one is that both + * are about the *message*. A refusal that fails with empty output `Data` renders the UI's generic + * "Conversion failed." with nothing else to say, which is the defect shape `DeniedForegroundStartTest` + * records from the device pass. Asserting the verdict alone would pass against exactly that. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class RefusedJobTest { + + private lateinit var app: Application + private lateinit var publisher: OutputPublisher + private lateinit var engine: RefusingTranscoder + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = AlwaysRoomPublisher(app) + engine = RefusingTranscoder() + ConversionDependencies.publisher = { publisher } + ConversionDependencies.software = { engine } + // Neither test is about probing or about this machine's codecs; both would otherwise decide + // the outcome for reasons no assertion mentions. See WorkerCancellationTest's setUp. + ConversionDependencies.probe = { _, _ -> InputProbe() } + ConversionDependencies.deviceCodecs = { DeviceCodecs.PERMISSIVE } + installTestWorkManager(app, Data.EMPTY) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `a job with no input URI fails with a message rather than a bare failure`() { + val result = runBlocking { workerWithout(ConversionWorker.KEY_INPUT_URI).doWork() } + + // `Failure.equals` compares output data, so this pins the message and the verdict together. + assertEquals( + ListenableWorker.Result.failure(workDataOf(ConversionWorker.KEY_ERROR to "No input file.")), + result, + ) + } + + @Test + fun `a job with no input URI stages nothing`() { + // The URI read is the first thing doWork does -- above the space check, above the staging + // name, above the try. A refusal there must not have reserved anything. + runBlocking { workerWithout(ConversionWorker.KEY_INPUT_URI).doWork() } + + assertEquals("a job refused for having no input must not stage a file", emptyList(), stagedNames()) + } + + @Test + fun `a spec the picker would never have allowed is refused with the reason`() { + // WAV carries PCM and nothing else. The picker cannot produce this combination today, which + // is exactly why the worker checks: the job can arrive from a queue written before the + // settings changed, or from a direct request(...) call. + val expected = ContainerCapabilities.validate(REFUSED_SPEC, InputProbe()) as? Validation.Invalid + ?: throw AssertionError("the fixture spec is supposed to be invalid; ContainerCapabilities disagrees") + + val result = runBlocking { worker(REFUSED_SPEC).doWork() } + + assertEquals( + ListenableWorker.Result.failure(workDataOf(ConversionWorker.KEY_ERROR to expected.message)), + result, + ) + } + + @Test + fun `a refused spec never reaches an engine`() { + // The half that says it failed *before* converting rather than during. Without this, a + // worker that ran the job and then reported the validation message would pass the test + // above -- and would have spent the user's battery on a file it was going to refuse. + runBlocking { worker(REFUSED_SPEC).doWork() } + + assertTrue("a refused spec must be refused before any engine runs", engine.invocations.isEmpty()) + } + + @Test + fun `a valid spec is not refused`() { + // The control. Every assertion above is about a refusal, so without this they would all + // still pass against a worker that refused everything. + val result = runBlocking { worker(OutputFormat.MP4_H265.spec).doWork() } + + assertEquals(ListenableWorker.Result.success(), stripOutput(result)) + assertEquals(listOf(OutputFormat.MP4_H265.spec), engine.invocations) + } + + // --- the same refusal, on the join side ---------------------------------- + + @Test + fun `a join of a single file is refused with a message rather than joined`() { + // The arm beside it -- a job with no URI array at all -- is covered on the device by + // `UnopenableUriTest.aJoinWithNoInputArrayFailsWithAMessage`. This one was covered by + // nothing in either source set, which a coverage report cannot say because it cannot see + // androidTest: the two arms are adjacent lines and only one of them had a test. + // + // Reachable for the reason this file's header gives, plus one of its own: `request(...)` + // takes a `List` and checks nothing about its length, so a single-item join is a + // well-formed call, not a corrupted queue entry. + val result = runBlocking { joinWorker(INPUT).doWork() } + + assertEquals( + ListenableWorker.Result.failure( + workDataOf(ConcatWorker.KEY_ERROR to "Pick at least two files to join."), + ), + result, + ) + } + + @Test + fun `a join of two files is not refused for its count`() { + // The control, and the half that makes the test above bite on the boundary rather than on + // the message: without it, `uris.size < 3` passes everything here. + // + // It refuses the space instead of letting the job run, because the next thing past the + // count guard is `ConcatEngine`, which is native -- `NamingPublisher`'s KDoc records that + // no JVM test gets past it. A refusal with the *space* message is proof that execution + // reached line 57, which is proof it got past line 42, and it costs no engine to say so. + val noRoom = NamingPublisher(app).apply { refuseSpace = true } + ConversionDependencies.publisher = { noRoom } + + val result = runBlocking { joinWorker(INPUT, SECOND_INPUT).doWork() } + + assertEquals( + ListenableWorker.Result.failure( + workDataOf(ConcatWorker.KEY_ERROR to "Not enough free space to join these files."), + ), + result, + ) + } + + /** [ListenableWorker.Result.Success] compares its output data, which these tests do not pin. */ + private fun stripOutput(result: ListenableWorker.Result): ListenableWorker.Result = + if (result is ListenableWorker.Result.Success) ListenableWorker.Result.success() else result + + private fun worker(spec: OutputSpec): ConversionWorker = build( + workDataOf( + ConversionWorker.KEY_INPUT_URI to INPUT.toString(), + ConversionWorker.KEY_DISPLAY_NAME to DISPLAY_NAME, + ConversionWorker.KEY_SIZE_BYTES to INPUT_BYTES, + ConversionWorker.KEY_CONTAINER to spec.container.name, + ConversionWorker.KEY_VIDEO_CODEC to spec.videoCodec.name, + ConversionWorker.KEY_AUDIO_CODEC to spec.audioCodec.name, + ConversionWorker.KEY_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name, + ), + ) + + /** + * The ordinary input `Data`, less one key. + * + * Built by removal rather than by spelling out a shorter map, so the test cannot drift into + * omitting something else as well and passing for a reason it does not name. + */ + private fun workerWithout(key: String): ConversionWorker { + val full = OutputFormat.MP4_H265.spec + val entries = mapOf( + ConversionWorker.KEY_INPUT_URI to INPUT.toString(), + ConversionWorker.KEY_DISPLAY_NAME to DISPLAY_NAME, + ConversionWorker.KEY_SIZE_BYTES to INPUT_BYTES, + ConversionWorker.KEY_CONTAINER to full.container.name, + ConversionWorker.KEY_VIDEO_CODEC to full.videoCodec.name, + ConversionWorker.KEY_AUDIO_CODEC to full.audioCodec.name, + ConversionWorker.KEY_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name, + ) - key + return build(Data.Builder().putAll(entries).build()) + } + + private fun build(data: Data): ConversionWorker = + TestListenableWorkerBuilder(context = app, inputData = data, runAttemptCount = 0) + .setId(JOB_ID) + .build() + + /** + * A join job carrying [inputs], a declared total, and a format. + * + * The total is declared so `hasRoomFor` takes its `hasSpaceFor` branch: the other branch is + * `hasSpaceForUnknownSize`, which `NamingPublisher` does not override and which would measure + * this machine's real disk. + */ + private fun joinWorker(vararg inputs: Uri): ConcatWorker = TestListenableWorkerBuilder( + context = app, + inputData = workDataOf( + ConcatWorker.KEY_INPUT_URIS to inputs.map(Uri::toString).toTypedArray(), + ConcatWorker.KEY_TOTAL_BYTES to INPUT_BYTES * inputs.size, + ConcatWorker.KEY_FORMAT to OutputFormat.MP4_H264.name, + ), + runAttemptCount = 0, + ).setId(JOB_ID).build() + + private fun stagedNames(): List = + publisher.createStagingFile("anything").parentFile?.listFiles().orEmpty().map { it.name }.sorted() + + private companion object { + val INPUT: Uri = Uri.parse("file:///tmp/holiday.mp4") + const val DISPLAY_NAME = "holiday.mp4" + const val INPUT_BYTES = 1024L + + /** A join needs two, and "two" is the boundary the count guard is about. */ + val SECOND_INPUT: Uri = Uri.parse("file:///tmp/holiday-2.mp4") + + /** WAV carries PCM and nothing else, so AAC in WAV has nowhere to go. */ + val REFUSED_SPEC = OutputSpec( + org.libremediaconverter.model.Container.WAV, + VideoCodec.NONE, + AudioCodec.AAC, + ) + val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000005") + } +} + +/** An engine that records what it was asked for and writes an output, so a success is a success. */ +private class RefusingTranscoder : SoftwareTranscoder { + + /** Every spec that actually reached an engine. Empty is the assertion for a refused job. */ + val invocations = mutableListOf() + + override suspend fun run( + request: ConversionRequest, + inputPath: String, + output: File, + durationMs: Long, + onProgress: (Int) -> Unit, + ) { + invocations += request.spec + output.writeBytes(ByteArray(OUTPUT_BYTES)) + } + + private companion object { + const val OUTPUT_BYTES = 512 + } +} diff --git a/app/src/test/java/org/libremediaconverter/work/WorkerCancellationTest.kt b/app/src/test/java/org/libremediaconverter/work/WorkerCancellationTest.kt index a04f345..0a79eb8 100644 --- a/app/src/test/java/org/libremediaconverter/work/WorkerCancellationTest.kt +++ b/app/src/test/java/org/libremediaconverter/work/WorkerCancellationTest.kt @@ -1,12 +1,16 @@ package org.libremediaconverter.work import android.app.Application +import android.content.Context import android.net.Uri import androidx.media3.common.util.UnstableApi import androidx.work.Data +import androidx.work.ForegroundInfo +import androidx.work.ForegroundUpdater import androidx.work.ListenableWorker import androidx.work.testing.TestListenableWorkerBuilder import androidx.work.workDataOf +import com.google.common.util.concurrent.ListenableFuture import kotlinx.coroutines.CancellationException import kotlinx.coroutines.runBlocking import org.junit.After @@ -18,6 +22,7 @@ import org.junit.runner.RunWith import org.libremediaconverter.convert.ConversionDependencies import org.libremediaconverter.convert.OutputPublisher import org.libremediaconverter.convert.SoftwareTranscoder +import org.libremediaconverter.convert.StagingNames import org.libremediaconverter.convert.installTestWorkManager import org.libremediaconverter.model.ConversionRequest import org.libremediaconverter.model.DeviceCodecs @@ -108,6 +113,54 @@ class WorkerCancellationTest { assertEquals("a failed attempt must not leave its partial behind", emptyList(), stagedNames()) } + @Test + fun `a cancelled join propagates instead of being turned into a Result`() { + val thrown = runCatching { runBlocking { concatWorker().doWork() } }.exceptionOrNull() + + assertTrue( + "cancellation must leave doWork as cancellation, not as a Result; got $thrown", + thrown is CancellationException, + ) + } + + @Test + fun `a cancelled join still deletes the partial it had already staged`() { + // Written first, so a missing delete cannot pass by asking whether a file nobody wrote is + // absent -- the same reason PartialThenFailingTranscoder writes before it throws. + concatStagedFile().writeBytes(ByteArray(PARTIAL_STAGED_BYTES)) + + runCatching { runBlocking { concatWorker().doWork() } } + + assertEquals("a cancelled join must not leave its partial behind", emptyList(), stagedNames()) + } + + /** + * A join whose foreground start is cancelled rather than denied. + * + * The conversion twin cancels *inside the engine*, which is the honest shape there because + * `ConversionDependencies` has a seam for it. `ConcatWorker` calls `ConcatEngine` directly and + * has no such seam -- it is native, and nothing here gets past it -- so the cancellation is + * injected at the only other point inside the `try`: `setForeground`. That is not a contrivance. + * A job cancelled while WorkManager is promoting it to the foreground is precisely when the + * window is open, and what is being tested is the `catch` arm, which cannot tell where in the + * `try` the cancellation came from. + */ + private fun concatWorker(): ConcatWorker = TestListenableWorkerBuilder( + context = app, + inputData = workDataOf( + ConcatWorker.KEY_INPUT_URIS to arrayOf(INPUT.toString(), "file:///tmp/second.mp4"), + ConcatWorker.KEY_TOTAL_BYTES to INPUT_BYTES, + ConcatWorker.KEY_FORMAT to CONCAT_FORMAT.name, + ), + runAttemptCount = 0, + ).setId(CONCAT_ID) + .setForegroundUpdater(CancellingForegroundUpdater) + .build() + + /** The staging path the join will compute, asked for rather than spelled out here. */ + private fun concatStagedFile(): File = + publisher.createStagingFile(StagingNames.forJob(CONCAT_ID, CONCAT_FORMAT.extension)) + /** * A worker routed to the software engine, which is [failure] and nothing else. * @@ -142,7 +195,10 @@ class WorkerCancellationTest { const val DISPLAY_NAME = "holiday.mp4" const val INPUT_BYTES = 1024L val SPEC = OutputFormat.MP4_H265.spec + val CONCAT_FORMAT = OutputFormat.MP4_H264 + const val PARTIAL_STAGED_BYTES = 2048 val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000003") + val CONCAT_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000004") } } @@ -167,3 +223,19 @@ private class PartialThenFailingTranscoder(private val failure: () -> Nothing) : const val PARTIAL_BYTES = 2048 } } + +/** + * Stands in for a job cancelled while WorkManager is promoting it to the foreground. + * + * The mechanism `DeniedForegroundStartTest` documents, carrying a different exception: + * `WorkForegroundUpdater` propagates whatever the future failed with, and + * `ListenableFuture.await()` unwraps the `ExecutionException`, so the worker meets a bare + * `CancellationException` exactly where a real cancellation would put one. + */ +private object CancellingForegroundUpdater : ForegroundUpdater { + override fun setForegroundAsync( + context: Context, + id: UUID, + foregroundInfo: ForegroundInfo, + ): ListenableFuture = FailedFuture(CancellationException("cancelled while going foreground")) +} diff --git a/app/src/test/java/org/libremediaconverter/work/WorkerEnumFallbackTest.kt b/app/src/test/java/org/libremediaconverter/work/WorkerEnumFallbackTest.kt index 32cea02..ce32a1e 100644 --- a/app/src/test/java/org/libremediaconverter/work/WorkerEnumFallbackTest.kt +++ b/app/src/test/java/org/libremediaconverter/work/WorkerEnumFallbackTest.kt @@ -22,6 +22,7 @@ import org.libremediaconverter.model.DeviceCodecs import org.libremediaconverter.model.EnginePreference import org.libremediaconverter.model.InputProbe import org.libremediaconverter.model.OutputFormat +import org.libremediaconverter.model.OutputSpec import org.libremediaconverter.model.QualityTier import org.robolectric.RobolectricTestRunner import org.robolectric.RuntimeEnvironment @@ -109,6 +110,50 @@ class WorkerEnumFallbackTest { ) } + @Test + fun `a container this build does not define falls back to the default spec`() { + assertFallsBackToDefault(container = "HOLOTAPE") + } + + @Test + fun `a video codec this build does not define falls back to the default spec`() { + assertFallsBackToDefault(video = "H267") + } + + @Test + fun `an audio codec this build does not define falls back to the default spec`() { + assertFallsBackToDefault(audio = "SUPER_AAC") + } + + /** + * Drives a job whose spec is [NOT_THE_FALLBACK] on every axis but the one named, and asserts the + * whole spec came back as [DEFAULT_SPEC]. + * + * **The baseline is the point.** `readSpec` returns the *entire* fallback spec the moment any + * one axis fails to resolve, so a test starting from `MP4_H265` -- which is itself the fallback + * -- could not tell a worker that read the spec correctly from one that gave up on it. Starting + * from MKV/H.264 makes the difference visible on two axes at once. + * + * Asserting the spec that *ran*, rather than only that a `Result` came back, is the other half: + * the defect these three are written for threw out of `doWork` entirely, so "a Result at all" + * would pass against a fallback to something arbitrary. + */ + private fun assertFallsBackToDefault( + container: String = NOT_THE_FALLBACK.container.name, + video: String = NOT_THE_FALLBACK.videoCodec.name, + audio: String = NOT_THE_FALLBACK.audioCodec.name, + ) { + val transcoder = RequestRecordingTranscoder() + ConversionDependencies.software = { transcoder } + + val result = runBlocking { + conversionWorker(container = container, video = video, audio = audio).doWork() + } + + assertEquals(ListenableWorker.Result.success(), stripOutput(result)) + assertEquals(listOf(DEFAULT_SPEC), transcoder.specs) + } + /** [ListenableWorker.Result.Success] compares its output data, which these tests do not pin. */ private fun stripOutput(result: ListenableWorker.Result): ListenableWorker.Result = if (result is ListenableWorker.Result.Success) ListenableWorker.Result.success() else result @@ -116,15 +161,18 @@ class WorkerEnumFallbackTest { private fun conversionWorker( quality: String = QualityTier.FAST.name, preference: String = EnginePreference.FORCE_SOFTWARE.name, + container: String = SPEC.container.name, + video: String = SPEC.videoCodec.name, + audio: String = SPEC.audioCodec.name, ): ConversionWorker = TestListenableWorkerBuilder( context = app, inputData = workDataOf( ConversionWorker.KEY_INPUT_URI to INPUT.toString(), ConversionWorker.KEY_DISPLAY_NAME to DISPLAY_NAME, ConversionWorker.KEY_SIZE_BYTES to INPUT_BYTES, - ConversionWorker.KEY_CONTAINER to SPEC.container.name, - ConversionWorker.KEY_VIDEO_CODEC to SPEC.videoCodec.name, - ConversionWorker.KEY_AUDIO_CODEC to SPEC.audioCodec.name, + ConversionWorker.KEY_CONTAINER to container, + ConversionWorker.KEY_VIDEO_CODEC to video, + ConversionWorker.KEY_AUDIO_CODEC to audio, ConversionWorker.KEY_QUALITY to quality, ConversionWorker.KEY_ENGINE_PREFERENCE to preference, ), @@ -146,6 +194,12 @@ class WorkerEnumFallbackTest { const val DISPLAY_NAME = "holiday.mp4" const val INPUT_BYTES = 1024L val SPEC = OutputFormat.MP4_H265.spec + + /** What `readSpec` returns when any axis fails to resolve. */ + val DEFAULT_SPEC = OutputFormat.MP4_H265.spec + + /** A spec that differs from [DEFAULT_SPEC] on container *and* video codec. See the helper. */ + val NOT_THE_FALLBACK = OutputFormat.MKV_H264.spec val CONVERSION_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000021") val CONCAT_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000022") } @@ -156,6 +210,9 @@ private class RequestRecordingTranscoder : SoftwareTranscoder { val qualities = mutableListOf() + /** The spec each run was asked for. Which one ran is what the three readSpec tests assert. */ + val specs = mutableListOf() + override suspend fun run( request: ConversionRequest, inputPath: String, @@ -164,6 +221,7 @@ private class RequestRecordingTranscoder : SoftwareTranscoder { onProgress: (Int) -> Unit, ) { qualities += request.quality + specs += request.spec output.writeBytes(ByteArray(OUTPUT_BYTES)) } diff --git a/app/src/test/java/org/libremediaconverter/work/WorkerStubs.kt b/app/src/test/java/org/libremediaconverter/work/WorkerStubs.kt index 848c04b..14f3abe 100644 --- a/app/src/test/java/org/libremediaconverter/work/WorkerStubs.kt +++ b/app/src/test/java/org/libremediaconverter/work/WorkerStubs.kt @@ -1,10 +1,14 @@ package org.libremediaconverter.work import android.content.Context +import com.google.common.util.concurrent.ListenableFuture import org.libremediaconverter.convert.OutputPublisher import org.libremediaconverter.convert.SoftwareTranscoder import org.libremediaconverter.model.ConversionRequest import java.io.File +import java.util.concurrent.ExecutionException +import java.util.concurrent.Executor +import java.util.concurrent.TimeUnit /** * Scaffolding more than one worker test needs. @@ -68,3 +72,25 @@ object WritingTranscoder : SoftwareTranscoder { private const val OUTPUT_BYTES = 512 } + +/** + * An already-failed future, written out rather than pulled from a futures library. + * + * `await()` takes the `isDone` fast path and unwraps the `ExecutionException`, which is what puts + * the original exception in front of the worker's `catch` rather than a wrapper. That is the whole + * mechanism behind driving a `ForegroundUpdater` to fail: `WorkForegroundUpdater` propagates + * whatever the future failed with rather than swallowing it, so `setForeground()` throws exactly + * what is handed here. + * + * Shared because two tests inject two different failures through it -- a denied foreground start + * and a cancellation -- and Kotlin will not take two file-private top-level classes of one name in + * one package. + */ +internal class FailedFuture(private val failure: Throwable) : ListenableFuture { + override fun addListener(listener: Runnable, executor: Executor): Unit = executor.execute(listener) + override fun cancel(mayInterruptIfRunning: Boolean): Boolean = false + override fun isCancelled(): Boolean = false + override fun isDone(): Boolean = true + override fun get(): Void = throw ExecutionException(failure) + override fun get(timeout: Long, unit: TimeUnit): Void = throw ExecutionException(failure) +} 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.