Merge branch 'test/refused-jobs' into test/mediaprobe-track-seam
This commit is contained in:
@@ -0,0 +1,234 @@
|
||||
package org.libremediaconverter.convert
|
||||
|
||||
import android.content.ComponentName
|
||||
import android.content.ContentProvider
|
||||
import android.content.ContentValues
|
||||
import android.content.Context
|
||||
import android.content.IntentFilter
|
||||
import android.content.pm.ProviderInfo
|
||||
import android.database.Cursor
|
||||
import android.database.MatrixCursor
|
||||
import android.net.Uri
|
||||
import android.os.Bundle
|
||||
import android.provider.DocumentsContract
|
||||
import android.provider.OpenableColumns
|
||||
import org.robolectric.Robolectric
|
||||
import org.robolectric.Shadows.shadowOf
|
||||
import java.io.File
|
||||
|
||||
/**
|
||||
* Content providers more than one test needs, and the registration dance they all repeat.
|
||||
*
|
||||
* Only that. A stub that serves one test stays in that test, next to the assertion it exists for —
|
||||
* the rule `work/WorkerStubs.kt` states, and the reason `UnreliableOutputStream` is still private to
|
||||
* `OutputPublisherPublishTest`.
|
||||
*
|
||||
* These started life inside `OutputPublisherPublishTest`, which is the only thing that needed a
|
||||
* provider at all. They moved here when `InputQuery`'s cursor reads turned out to need the same
|
||||
* provider answering *badly* — see [RowShape].
|
||||
*/
|
||||
|
||||
internal const val DOCUMENTS_AUTHORITY = "org.libremediaconverter.test.documents"
|
||||
internal const val PLAIN_AUTHORITY = "org.libremediaconverter.test.plain"
|
||||
|
||||
/**
|
||||
* How [FakeSafProvider] answers a metadata query.
|
||||
*
|
||||
* A provider is another app. It can be uninstalled, revoke its grant, crash, or simply answer
|
||||
* something the caller did not expect — and "answered something unexpected" is not one case but
|
||||
* several, which is why this is an enum rather than a boolean.
|
||||
*
|
||||
* The distinction that matters most to callers is **null versus missing versus zero versus
|
||||
* negative**. `InputQuery` exists to stop the last three being conflated: `hasSpaceFor(0)` is only
|
||||
* "is there 128 MB free", so a size nobody could determine must not arrive as `0`, and
|
||||
* `OutputPublisher.destinationIsKnownEmpty` must answer `false` — never "empty, go ahead and
|
||||
* delete" — for every one of them.
|
||||
*
|
||||
* Column-level granularity is deliberate. `OutputPublisher` reads only `SIZE`; `InputQuery` reads
|
||||
* both, and reaches a different answer depending on which one is bad.
|
||||
*/
|
||||
internal enum class RowShape {
|
||||
/** What a healthy provider answers: the file's real name and real length. */
|
||||
NORMAL,
|
||||
|
||||
/** A row is present and its `DISPLAY_NAME` cell is null. */
|
||||
NULL_DISPLAY_NAME,
|
||||
|
||||
/** A row is present and its `SIZE` cell is null. */
|
||||
NULL_SIZE,
|
||||
|
||||
/** The cursor carries no `DISPLAY_NAME` column at all — `getColumnIndex` gives `-1`. */
|
||||
NO_DISPLAY_NAME_COLUMN,
|
||||
|
||||
/** The cursor carries no `SIZE` column at all — `getColumnIndex` gives `-1`. */
|
||||
NO_SIZE_COLUMN,
|
||||
|
||||
/**
|
||||
* A size of `-1`.
|
||||
*
|
||||
* Not a corrupt provider: it is what anything without a fixed length reports — a pipe, or a
|
||||
* provider streaming its answer — and it is a third way of saying "unknown", distinct from a
|
||||
* null cell and from a missing column.
|
||||
*/
|
||||
NEGATIVE_SIZE,
|
||||
|
||||
/**
|
||||
* A cursor with the right columns and no rows in it.
|
||||
*
|
||||
* Distinct from returning `null`, which is what a provider that does not recognise the URI
|
||||
* does. Both mean "no answer", and code that treats one as an answer and the other as an
|
||||
* absence is wrong about one of them.
|
||||
*/
|
||||
NO_ROWS,
|
||||
|
||||
/**
|
||||
* The query itself throws.
|
||||
*
|
||||
* A resolver call is a call into another app, and that app can have been uninstalled, revoked
|
||||
* its grant, or simply crashed. `InputQuery.firstRow`'s KDoc is explicit that "a file picker is
|
||||
* not a place to bring the process down from", so this is the shape that proves the guard is
|
||||
* one.
|
||||
*/
|
||||
QUERY_THROWS,
|
||||
}
|
||||
|
||||
/**
|
||||
* A stand-in for the provider behind a SAF destination.
|
||||
*
|
||||
* It answers only what its callers ask of a document -- how many bytes are already there, what it
|
||||
* is called, and delete it -- backed by a real file so the assertions are about the filesystem
|
||||
* rather than about a mock's call log alone. The rest of the `ContentProvider` surface is stubbed.
|
||||
*
|
||||
* Writing is deliberately NOT routed through it. Robolectric's `ShadowContentResolver`
|
||||
* consults its registered-stream map before it reaches any provider, which is what lets a
|
||||
* test hand out a stream that writes some bytes and then fails -- a condition a real provider
|
||||
* cannot be asked to produce on demand.
|
||||
*/
|
||||
internal open class FakeSafProvider : ContentProvider() {
|
||||
|
||||
override fun onCreate() = true
|
||||
|
||||
override fun query(
|
||||
uri: Uri,
|
||||
projection: Array<out String>?,
|
||||
selection: String?,
|
||||
selectionArgs: Array<out String>?,
|
||||
sortOrder: String?,
|
||||
): Cursor? {
|
||||
if (rowShape == RowShape.QUERY_THROWS) throw SecurityException("provider revoked the grant")
|
||||
val file = backingFile(uri)
|
||||
if (!file.exists()) return null
|
||||
return MatrixCursor(columnsFor(rowShape)).apply {
|
||||
if (rowShape != RowShape.NO_ROWS) addRow(cellsFor(rowShape, file))
|
||||
}
|
||||
}
|
||||
|
||||
override fun call(method: String, arg: String?, extras: Bundle?): Bundle? {
|
||||
if (method != METHOD_DELETE_DOCUMENT) return null
|
||||
val target = extras?.getParcelable(EXTRA_URI, Uri::class.java) ?: return null
|
||||
deleteRequests += target
|
||||
deleteFailure?.let { throw it }
|
||||
backingFile(target).delete()
|
||||
return Bundle()
|
||||
}
|
||||
|
||||
override fun getType(uri: Uri) = "video/mp4"
|
||||
|
||||
override fun insert(uri: Uri, values: ContentValues?): Uri? = null
|
||||
|
||||
override fun delete(uri: Uri, selection: String?, selectionArgs: Array<out String>?) = 0
|
||||
|
||||
override fun update(uri: Uri, values: ContentValues?, selection: String?, selectionArgs: Array<out String>?) = 0
|
||||
|
||||
companion object {
|
||||
// DocumentsContract.METHOD_DELETE_DOCUMENT and EXTRA_URI are hidden from the public
|
||||
// SDK, so they cannot be referenced. These are the wire names
|
||||
// DocumentsContract.deleteDocument() actually sends, which is what a provider sees.
|
||||
const val METHOD_DELETE_DOCUMENT = "android:deleteDocument"
|
||||
const val EXTRA_URI = "uri"
|
||||
|
||||
/** Where the "documents" really live. Set per test to a Robolectric temp path. */
|
||||
lateinit var root: File
|
||||
|
||||
/** Every delete this provider was asked for, in order. Empty is an assertion too. */
|
||||
val deleteRequests = mutableListOf<Uri>()
|
||||
|
||||
/** Armed by the test that needs the cleanup itself to fail. */
|
||||
var deleteFailure: RuntimeException? = null
|
||||
|
||||
/**
|
||||
* How the next query answers. [reset] puts it back to [RowShape.NORMAL], so a test that
|
||||
* does not care never has to think about it.
|
||||
*/
|
||||
var rowShape: RowShape = RowShape.NORMAL
|
||||
|
||||
fun backingFile(uri: Uri) = File(root, uri.lastPathSegment.orEmpty())
|
||||
|
||||
fun reset(directory: File) {
|
||||
root = directory
|
||||
deleteRequests.clear()
|
||||
deleteFailure = null
|
||||
rowShape = RowShape.NORMAL
|
||||
}
|
||||
|
||||
private fun columnsFor(shape: RowShape): Array<String> = when (shape) {
|
||||
RowShape.NO_DISPLAY_NAME_COLUMN -> arrayOf(OpenableColumns.SIZE)
|
||||
RowShape.NO_SIZE_COLUMN -> arrayOf(OpenableColumns.DISPLAY_NAME)
|
||||
else -> arrayOf(OpenableColumns.DISPLAY_NAME, OpenableColumns.SIZE)
|
||||
}
|
||||
|
||||
private fun cellsFor(shape: RowShape, file: File): Array<Any?> = when (shape) {
|
||||
RowShape.NO_DISPLAY_NAME_COLUMN -> arrayOf(file.length())
|
||||
RowShape.NO_SIZE_COLUMN -> arrayOf<Any?>(file.name)
|
||||
RowShape.NULL_DISPLAY_NAME -> arrayOf(null, file.length())
|
||||
RowShape.NULL_SIZE -> arrayOf(file.name, null)
|
||||
RowShape.NEGATIVE_SIZE -> arrayOf(file.name, UNKNOWN_LENGTH)
|
||||
else -> arrayOf(file.name, file.length())
|
||||
}
|
||||
|
||||
/** What `statSize` reports for anything without a fixed length. See [RowShape.NEGATIVE_SIZE]. */
|
||||
private const val UNKNOWN_LENGTH = -1L
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The same provider, registered WITHOUT the documents-provider intent filter.
|
||||
*
|
||||
* A separate class because the package manager keys providers by component name, so two
|
||||
* authorities need two components. It exists to prove the guard is a guard: a content URI
|
||||
* from something that is not a documents provider must not be handed to `deleteDocument`.
|
||||
*/
|
||||
internal class FakePlainProvider : FakeSafProvider()
|
||||
|
||||
/**
|
||||
* Stands [provider] up on [authority] so `contentResolver` and the package manager both know it.
|
||||
*
|
||||
* `isDocumentUri()` does not look at the URI alone: it asks the package manager whether anything
|
||||
* answers `ACTION_DOCUMENTS_PROVIDER` for that authority. Registering the provider with the
|
||||
* resolver is not enough, which is the whole reason [asDocumentsProvider] is a parameter rather
|
||||
* than always true — the negative case is a test.
|
||||
*/
|
||||
internal fun registerProvider(
|
||||
context: Context,
|
||||
provider: Class<out FakeSafProvider>,
|
||||
authority: String,
|
||||
asDocumentsProvider: Boolean,
|
||||
) {
|
||||
val info = ProviderInfo().apply {
|
||||
this.authority = authority
|
||||
packageName = context.packageName
|
||||
name = provider.name
|
||||
exported = true
|
||||
grantUriPermissions = true
|
||||
}
|
||||
Robolectric.buildContentProvider(provider).create(info)
|
||||
|
||||
val packageManager = shadowOf(context.packageManager)
|
||||
packageManager.addOrUpdateProvider(info)
|
||||
if (asDocumentsProvider) {
|
||||
packageManager.addIntentFilterForProvider(
|
||||
ComponentName(context.packageName, provider.name),
|
||||
IntentFilter(DocumentsContract.PROVIDER_INTERFACE),
|
||||
)
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,187 @@
|
||||
package org.libremediaconverter.convert
|
||||
|
||||
import android.content.Context
|
||||
import android.net.Uri
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertNull
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
import org.robolectric.RobolectricTestRunner
|
||||
import org.robolectric.RuntimeEnvironment
|
||||
import java.io.File
|
||||
|
||||
/**
|
||||
* What [InputQuery] makes of a metadata row.
|
||||
*
|
||||
* ## Why this is a separate file from `UnknownInputSizeTest`
|
||||
*
|
||||
* That test drives the case where **no provider is registered** — the query returns null and
|
||||
* `measure()` answers instead — and it drives it thoroughly. What it never does is hand `InputQuery`
|
||||
* a row. Before this file, nothing did: `firstRow`'s body, `displayNameOrNull` and `sizeOrNull` had
|
||||
* never executed in the JVM suite, so every branch inside them was untested.
|
||||
*
|
||||
* ## What is actually being pinned
|
||||
*
|
||||
* Not "does it read a cursor" — that would pass against almost any implementation. The rule is that
|
||||
* **a size nobody could determine must not arrive as a number**, and there are four separate ways a
|
||||
* provider fails to determine one: a null cell, a missing column, a negative value, and no row at
|
||||
* all. `InputQuery`'s KDoc states the stake:
|
||||
*
|
||||
* > a worker's input `Data` carries the size the *picker* found … `hasSpaceFor(0)` is only "is there
|
||||
* > 128 MB free".
|
||||
*
|
||||
* So each of those four must produce `null`, and `null` specifically — not `0`, not `-1`. A test
|
||||
* that asserted only "not the file's length" would pass on `0`, which is the exact conflation the
|
||||
* class exists to end.
|
||||
*
|
||||
* ## Why every fall-through lands on null here
|
||||
*
|
||||
* [FakeSafProvider] does not implement `openFile`, so `measure()` cannot answer for these URIs
|
||||
* either. That is deliberate: it isolates the cursor half. The other direction — the cursor says
|
||||
* nothing and `measure()` succeeds — is `UnknownInputSizeTest`'s
|
||||
* `a picked file no provider describes is measured rather than reported as empty`, and is not
|
||||
* repeated here.
|
||||
*
|
||||
* ## What the mutations say, including the one that does not bite
|
||||
*
|
||||
* Measured against `MatrixCursor`, which is what these tests drive:
|
||||
*
|
||||
* | call on a null cell | result |
|
||||
* |---|---|
|
||||
* | `getString` | returns `null` |
|
||||
* | `getLong` | returns **`0`** |
|
||||
*
|
||||
* That second row is why `sizeOrNull`'s `!isNull(it)` guard is load-bearing and why these tests
|
||||
* bite: remove it and a null size arrives as `0`, a real number indistinguishable from an empty
|
||||
* file, which is the precise conflation this class exists to end. Removing it reddens
|
||||
* `a null size is unknown rather than zero`. Removing the trailing `takeIf { it >= 0 }` reddens
|
||||
* `a negative size is unknown rather than reported`.
|
||||
*
|
||||
* **Named exemption: `displayNameOrNull`'s `!isNull(it)` guard is not pinned by anything here, and
|
||||
* cannot be.** `getString` returns null for a null cell, so the fallback applies with or without
|
||||
* the guard — removing it leaves every test in this file green. The guard is not redundant in
|
||||
* production: `Cursor.getString`'s contract states that whether it throws on a null column is
|
||||
* *implementation-defined*, and a real `ContentProvider` is free to throw where `MatrixCursor`
|
||||
* returns null. It should stay. It simply cannot be falsified with this cursor, and saying so is
|
||||
* better than implying `a null display name falls back without disturbing the size` covers it —
|
||||
* that test pins the behaviour, not the guard.
|
||||
*/
|
||||
@RunWith(RobolectricTestRunner::class)
|
||||
class InputQueryCursorTest {
|
||||
|
||||
private lateinit var context: Context
|
||||
private lateinit var uri: Uri
|
||||
|
||||
@Before
|
||||
fun setUp() {
|
||||
context = RuntimeEnvironment.getApplication()
|
||||
FakeSafProvider.reset(File(context.cacheDir, "picked").apply { mkdirs() })
|
||||
registerProvider(context, FakeSafProvider::class.java, DOCUMENTS_AUTHORITY, asDocumentsProvider = true)
|
||||
uri = Uri.parse("content://$DOCUMENTS_AUTHORITY/document/holiday.mp4")
|
||||
FakeSafProvider.backingFile(uri).writeBytes(ByteArray(PAYLOAD_BYTES))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a provider that answers properly supplies both the name and the size`() {
|
||||
val described = InputQuery.describe(context, uri)
|
||||
|
||||
assertEquals("holiday.mp4", described.displayName)
|
||||
assertEquals(PAYLOAD_BYTES.toLong(), described.sizeBytes)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a null display name falls back without disturbing the size`() {
|
||||
FakeSafProvider.rowShape = RowShape.NULL_DISPLAY_NAME
|
||||
|
||||
val described = InputQuery.describe(context, uri)
|
||||
|
||||
assertEquals(InputQuery.FALLBACK_DISPLAY_NAME, described.displayName)
|
||||
// The two columns are read independently. A provider that cannot name the file can still
|
||||
// size it, and losing the size here would be a bug the name assertion alone would miss.
|
||||
assertEquals(PAYLOAD_BYTES.toLong(), described.sizeBytes)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a cursor with no display name column falls back rather than throwing`() {
|
||||
// getColumnIndex returns -1 rather than throwing, so the `it >= 0` guard is the only thing
|
||||
// between this and an IllegalArgumentException out of getString.
|
||||
FakeSafProvider.rowShape = RowShape.NO_DISPLAY_NAME_COLUMN
|
||||
|
||||
val described = InputQuery.describe(context, uri)
|
||||
|
||||
assertEquals(InputQuery.FALLBACK_DISPLAY_NAME, described.displayName)
|
||||
assertEquals(PAYLOAD_BYTES.toLong(), described.sizeBytes)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a null size is unknown rather than zero`() {
|
||||
FakeSafProvider.rowShape = RowShape.NULL_SIZE
|
||||
|
||||
assertNull(unknownSizeMessage("a null cell"), InputQuery.sizeOf(context, uri))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a cursor with no size column is unknown rather than zero`() {
|
||||
FakeSafProvider.rowShape = RowShape.NO_SIZE_COLUMN
|
||||
|
||||
assertNull(unknownSizeMessage("a missing column"), InputQuery.sizeOf(context, uri))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a negative size is unknown rather than reported`() {
|
||||
// What anything without a fixed length reports -- a pipe, or a provider streaming its
|
||||
// answer. Passing -1 through would be worse than passing 0: hasSpaceFor compares it
|
||||
// against free space, so it would read as "needs less than nothing".
|
||||
FakeSafProvider.rowShape = RowShape.NEGATIVE_SIZE
|
||||
|
||||
assertNull(unknownSizeMessage("a negative size"), InputQuery.sizeOf(context, uri))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a cursor with no rows is unknown rather than zero`() {
|
||||
// Distinct from the provider returning null, which UnknownInputSizeTest covers. A cursor
|
||||
// that exists and holds nothing still has to reach the same answer.
|
||||
FakeSafProvider.rowShape = RowShape.NO_ROWS
|
||||
|
||||
val described = InputQuery.describe(context, uri)
|
||||
|
||||
assertEquals(InputQuery.FALLBACK_DISPLAY_NAME, described.displayName)
|
||||
assertNull(unknownSizeMessage("an empty cursor"), described.sizeBytes)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a provider that throws is survived rather than propagated`() {
|
||||
// The guard firstRow's KDoc exists for: "a resolver call is a call into another app ... and
|
||||
// a file picker is not a place to bring the process down from". Without the runCatching,
|
||||
// this SecurityException reaches the caller and takes the pick with it.
|
||||
FakeSafProvider.rowShape = RowShape.QUERY_THROWS
|
||||
|
||||
val described = InputQuery.describe(context, uri)
|
||||
|
||||
assertEquals(InputQuery.FALLBACK_DISPLAY_NAME, described.displayName)
|
||||
assertNull(unknownSizeMessage("a provider that threw"), described.sizeBytes)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a join total is unknown when any one input could not be sized`() {
|
||||
// The consequence the four cases above exist for, asserted once at the place it lands.
|
||||
// Summing the inputs that did answer would produce a lower bound indistinguishable from a
|
||||
// real total, which is what the space check cannot tell apart.
|
||||
FakeSafProvider.rowShape = RowShape.NULL_SIZE
|
||||
val unsizable = InputQuery.sizeOf(context, uri)
|
||||
FakeSafProvider.rowShape = RowShape.NORMAL
|
||||
val sizable = InputQuery.sizeOf(context, uri)
|
||||
|
||||
assertEquals(PAYLOAD_BYTES.toLong(), sizable)
|
||||
assertNull(unsizable)
|
||||
assertNull("one unknown input makes the whole total unknown", InputQuery.total(listOf(sizable, unsizable)))
|
||||
}
|
||||
|
||||
private fun unknownSizeMessage(cause: String) =
|
||||
"$cause means nobody could size the file; that must be null, not 0 -- hasSpaceFor(0) is only a headroom check"
|
||||
|
||||
private companion object {
|
||||
const val PAYLOAD_BYTES = 4096
|
||||
}
|
||||
}
|
||||
@@ -1,17 +1,7 @@
|
||||
package org.libremediaconverter.convert
|
||||
|
||||
import android.content.ComponentName
|
||||
import android.content.ContentProvider
|
||||
import android.content.ContentValues
|
||||
import android.content.Context
|
||||
import android.content.IntentFilter
|
||||
import android.content.pm.ProviderInfo
|
||||
import android.database.Cursor
|
||||
import android.database.MatrixCursor
|
||||
import android.net.Uri
|
||||
import android.os.Bundle
|
||||
import android.provider.DocumentsContract
|
||||
import android.provider.OpenableColumns
|
||||
import org.junit.Assert.assertArrayEquals
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertFalse
|
||||
@@ -20,7 +10,6 @@ import org.junit.Assert.assertTrue
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
import org.robolectric.Robolectric
|
||||
import org.robolectric.RobolectricTestRunner
|
||||
import org.robolectric.RuntimeEnvironment
|
||||
import org.robolectric.Shadows.shadowOf
|
||||
@@ -31,91 +20,6 @@ import java.io.OutputStream
|
||||
/** What a destination volume says when it fills up mid-write. */
|
||||
private const val NO_SPACE = "No space left on device"
|
||||
|
||||
private const val DOCUMENTS_AUTHORITY = "org.libremediaconverter.test.documents"
|
||||
private const val PLAIN_AUTHORITY = "org.libremediaconverter.test.plain"
|
||||
|
||||
/**
|
||||
* A stand-in for the provider behind a SAF destination.
|
||||
*
|
||||
* It answers only what `publish()` asks of a destination -- how many bytes are already there,
|
||||
* and delete it -- backed by a real file so the assertions are about the filesystem rather
|
||||
* than about a mock's call log alone. The rest of the `ContentProvider` surface is stubbed.
|
||||
*
|
||||
* Writing is deliberately NOT routed through it. Robolectric's `ShadowContentResolver`
|
||||
* consults its registered-stream map before it reaches any provider, which is what lets a
|
||||
* test hand out a stream that writes some bytes and then fails -- the condition this whole
|
||||
* file exists for, and one a real provider cannot be asked to produce on demand.
|
||||
*/
|
||||
internal open class FakeSafProvider : ContentProvider() {
|
||||
|
||||
override fun onCreate() = true
|
||||
|
||||
override fun query(
|
||||
uri: Uri,
|
||||
projection: Array<out String>?,
|
||||
selection: String?,
|
||||
selectionArgs: Array<out String>?,
|
||||
sortOrder: String?,
|
||||
): Cursor? {
|
||||
val file = backingFile(uri)
|
||||
if (!file.exists()) return null
|
||||
return MatrixCursor(arrayOf(OpenableColumns.DISPLAY_NAME, OpenableColumns.SIZE)).apply {
|
||||
addRow(arrayOf<Any?>(file.name, file.length()))
|
||||
}
|
||||
}
|
||||
|
||||
override fun call(method: String, arg: String?, extras: Bundle?): Bundle? {
|
||||
if (method != METHOD_DELETE_DOCUMENT) return null
|
||||
val target = extras?.getParcelable(EXTRA_URI, Uri::class.java) ?: return null
|
||||
deleteRequests += target
|
||||
deleteFailure?.let { throw it }
|
||||
backingFile(target).delete()
|
||||
return Bundle()
|
||||
}
|
||||
|
||||
override fun getType(uri: Uri) = "video/mp4"
|
||||
|
||||
override fun insert(uri: Uri, values: ContentValues?): Uri? = null
|
||||
|
||||
override fun delete(uri: Uri, selection: String?, selectionArgs: Array<out String>?) = 0
|
||||
|
||||
override fun update(uri: Uri, values: ContentValues?, selection: String?, selectionArgs: Array<out String>?) = 0
|
||||
|
||||
companion object {
|
||||
// DocumentsContract.METHOD_DELETE_DOCUMENT and EXTRA_URI are hidden from the public
|
||||
// SDK, so they cannot be referenced. These are the wire names
|
||||
// DocumentsContract.deleteDocument() actually sends, which is what a provider sees.
|
||||
const val METHOD_DELETE_DOCUMENT = "android:deleteDocument"
|
||||
const val EXTRA_URI = "uri"
|
||||
|
||||
/** Where the "documents" really live. Set per test to a Robolectric temp path. */
|
||||
lateinit var root: File
|
||||
|
||||
/** Every delete this provider was asked for, in order. Empty is an assertion too. */
|
||||
val deleteRequests = mutableListOf<Uri>()
|
||||
|
||||
/** Armed by the test that needs the cleanup itself to fail. */
|
||||
var deleteFailure: RuntimeException? = null
|
||||
|
||||
fun backingFile(uri: Uri) = File(root, uri.lastPathSegment.orEmpty())
|
||||
|
||||
fun reset(directory: File) {
|
||||
root = directory
|
||||
deleteRequests.clear()
|
||||
deleteFailure = null
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The same provider, registered WITHOUT the documents-provider intent filter.
|
||||
*
|
||||
* A separate class because the package manager keys providers by component name, so two
|
||||
* authorities need two components. It exists to prove the guard is a guard: a content URI
|
||||
* from something that is not a documents provider must not be handed to `deleteDocument`.
|
||||
*/
|
||||
internal class FakePlainProvider : FakeSafProvider()
|
||||
|
||||
/**
|
||||
* A sink that behaves like a volume filling up.
|
||||
*
|
||||
@@ -179,8 +83,8 @@ class OutputPublisherPublishTest {
|
||||
fun setUp() {
|
||||
context = RuntimeEnvironment.getApplication()
|
||||
FakeSafProvider.reset(File(context.cacheDir, "destinations").apply { mkdirs() })
|
||||
register(FakeSafProvider::class.java, DOCUMENTS_AUTHORITY, asDocumentsProvider = true)
|
||||
register(FakePlainProvider::class.java, PLAIN_AUTHORITY, asDocumentsProvider = false)
|
||||
registerProvider(context, FakeSafProvider::class.java, DOCUMENTS_AUTHORITY, asDocumentsProvider = true)
|
||||
registerProvider(context, FakePlainProvider::class.java, PLAIN_AUTHORITY, asDocumentsProvider = false)
|
||||
|
||||
// SAF's CreateDocument contract hands back a document that already exists and is
|
||||
// empty, so that is the state every destination starts in here.
|
||||
@@ -326,28 +230,4 @@ class OutputPublisherPublishTest {
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
private fun register(provider: Class<out FakeSafProvider>, authority: String, asDocumentsProvider: Boolean) {
|
||||
val info = ProviderInfo().apply {
|
||||
this.authority = authority
|
||||
packageName = context.packageName
|
||||
name = provider.name
|
||||
exported = true
|
||||
grantUriPermissions = true
|
||||
}
|
||||
Robolectric.buildContentProvider(provider).create(info)
|
||||
|
||||
// isDocumentUri() does not look at the URI alone: it asks the package manager whether
|
||||
// anything answers ACTION_DOCUMENTS_PROVIDER for that authority. Registering the
|
||||
// provider with the resolver is not enough, which is the whole reason the negative
|
||||
// case above can exist.
|
||||
val packageManager = shadowOf(context.packageManager)
|
||||
packageManager.addOrUpdateProvider(info)
|
||||
if (asDocumentsProvider) {
|
||||
packageManager.addIntentFilterForProvider(
|
||||
ComponentName(context.packageName, provider.name),
|
||||
IntentFilter(DocumentsContract.PROVIDER_INTERFACE),
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -358,4 +358,120 @@ class ContainerCapabilitiesTest {
|
||||
assertEquals(emptyList<VideoCodec>(), 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,
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<String>(),
|
||||
stagedNames(),
|
||||
)
|
||||
}
|
||||
|
||||
private fun conversionWorker(runAttemptCount: Int = 0): ConversionWorker =
|
||||
TestListenableWorkerBuilder<ConversionWorker>(
|
||||
context = app,
|
||||
@@ -141,18 +172,22 @@ class DeniedForegroundStartTest {
|
||||
.setForegroundUpdater(DenyingForegroundUpdater)
|
||||
.build()
|
||||
|
||||
private fun concatWorker(): ConcatWorker = TestListenableWorkerBuilder<ConcatWorker>(
|
||||
private fun concatWorker(runAttemptCount: Int = 0): ConcatWorker = TestListenableWorkerBuilder<ConcatWorker>(
|
||||
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<Void> {
|
||||
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)
|
||||
}
|
||||
|
||||
@@ -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<String>(), 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<Uri>` 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<ConversionWorker>(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<ConcatWorker>(
|
||||
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<String> =
|
||||
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<OutputSpec>()
|
||||
|
||||
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
|
||||
}
|
||||
}
|
||||
@@ -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<String>(), 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<String>(), 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<ConcatWorker>(
|
||||
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<Void> = FailedFuture(CancellationException("cancelled while going foreground"))
|
||||
}
|
||||
|
||||
@@ -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<ConversionWorker>(
|
||||
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<QualityTier>()
|
||||
|
||||
/** The spec each run was asked for. Which one ran is what the three readSpec tests assert. */
|
||||
val specs = mutableListOf<OutputSpec>()
|
||||
|
||||
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))
|
||||
}
|
||||
|
||||
|
||||
@@ -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<Void> {
|
||||
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)
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
Reference in New Issue
Block a user