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), - ) - } - } }