diff --git a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt index 3cc9352..de587e3 100644 --- a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt +++ b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt @@ -400,8 +400,21 @@ class ConversionViewModel @JvmOverloads constructor( activeWorkId?.let(workManager::cancelWorkById) } + /** + * Copies the staged result out to the destination the user picked. + * + * The existence check is not redundant with the one reattachment already made. That one ran + * inside a tag query which, for a result offered on launch, can be hours older than the tap — + * and `cacheDir` is exactly the directory the OS empties when it wants space, which is also + * what the sweep does to anything a day old. Without it the file's absence arrived as + * `staged.inputStream()` throwing, and `e.message` put a raw ENOENT path on screen. + */ fun save(destination: Uri) { val converted = _state.value as? ConversionState.Converted ?: return + if (!converted.staged.isFile) { + _state.value = ConversionState.Failed(STAGED_FILE_GONE_MESSAGE) + return + } viewModelScope.launch { runCatching { withContext(Dispatchers.IO) { diff --git a/app/src/main/java/org/libremediaconverter/convert/InputQuery.kt b/app/src/main/java/org/libremediaconverter/convert/InputQuery.kt index 0dd1730..8d8cbd3 100644 --- a/app/src/main/java/org/libremediaconverter/convert/InputQuery.kt +++ b/app/src/main/java/org/libremediaconverter/convert/InputQuery.kt @@ -64,9 +64,18 @@ object InputQuery { * A join's total is only as good as its worst-known part. Adding up the ones that answered * would produce a lower bound that reads exactly like a real total, and the space check has * no way to tell the two apart — which is the same conflation this whole file exists to end. + * + * The sum saturates rather than wrapping. Sizes reach here non-negative — both of the ways one + * is found reject a negative answer — but nothing bounds their *sum*, and a total that wrapped + * negative would not be harmless nonsense: `OutputPublisher.hasSpaceFor` compares it against + * free space, so the largest join representable would come back as the one with the most room. */ fun total(sizes: List): Long? = sizes.fold(0L as Long?) { running, size -> - if (running == null || size == null) null else running + size + when { + running == null || size == null -> null + size > Long.MAX_VALUE - running -> Long.MAX_VALUE + else -> running + size + } } /** diff --git a/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt index ebb8355..430b135 100644 --- a/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt +++ b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt @@ -6,6 +6,23 @@ import android.provider.DocumentsContract import android.provider.OpenableColumns import java.io.File +/** + * What a save has to say when the staged file is not there any more. + * + * Reachable without anything going wrong: staging lives in `cacheDir`, which the OS reclaims + * whenever it wants the space, and [sweepStaging] collects anything a day old. A result offered by + * reattachment is the likeliest to meet it — the check that decided the file existed ran during a + * tag query that can be hours old by the time the Save button is tapped. + * + * A written sentence rather than the exception's message, which is what used to reach the screen: + * `/data/user/0/org.libremediaconverter/cache/conversions/4b4882….mp4: open failed: ENOENT (No such + * file or directory)` is a true statement about a path the user has never seen and cannot act on. + * Kept next to [OutputPublisher] because both ViewModels need it and staging is what it is about. + */ +const val STAGED_FILE_GONE_MESSAGE: String = + "The finished file is no longer in the cache, so there is nothing left to save. " + + "Start over to make it again." + /** * Staging and publication of conversion output. * @@ -50,8 +67,18 @@ open class OutputPublisher(private val context: Context) { * through its engine, with a message of its own. * * Open so a test can force a full disk; see `FakeFailures` in the instrumented source set. + * + * Written as `free - headroom > required` rather than the equivalent-looking + * `free > required + headroom`. The second overflows: a request within 128 MiB of + * [Long.MAX_VALUE] wraps the sum negative, every free-space measurement beats a negative + * number, and the check answers "plenty of room" to the largest request it can be given. That + * is reachable rather than theoretical — [InputQuery.total] sums a join's inputs, so the number + * arriving here is not bounded by any single file. Both operands are clamped at zero first, so + * the subtraction cannot underflow and a nonsense negative size decides exactly as zero does + * instead of buying slack. */ - open fun hasSpaceFor(bytes: Long): Boolean = stagingDir.usableSpace > bytes + SPACE_HEADROOM_BYTES + open fun hasSpaceFor(bytes: Long): Boolean = + stagingDir.usableSpace.coerceAtLeast(0L) - SPACE_HEADROOM_BYTES > bytes.coerceAtLeast(0L) /** * The same check for a job whose input size nobody could determine — see [InputQuery]. @@ -111,14 +138,22 @@ open class OutputPublisher(private val context: Context) { * which is the right way round, since a flush that failed means the bytes are not * durably there to begin with. * - * A failure from `openOutputStream` itself is deliberately outside the guard. Nothing - * has been written at that point, so there is nothing of ours to remove. + * `openOutputStream` is inside the guard as well, and the reasoning that used to keep it + * out -- "nothing has been written at that point, so there is nothing of ours to remove" -- + * was wrong about what exists. SAF's `CreateDocument` contract creates the document *before* + * this is called, which is why every fixture in `OutputPublisherPublishTest` starts as an + * existing empty file. So a provider that hands out no stream at all -- gone between the + * picker and the write, or simply returning null -- left a zero-byte file at the name the + * user chose while the UI said "Could not save the file". The two bounds above are what make + * removing it safe, and they apply to this case exactly as they do to a failed copy. A + * provider that will not open its own empty document may well refuse to delete it too, which + * is already [deletePartialOutput]'s documented no-op path. */ open fun publish(staged: File, destination: Uri) { val destinationWasEmpty = destinationIsKnownEmpty(destination) - val out = context.contentResolver.openOutputStream(destination) - ?: error("Could not open destination for writing: $destination") try { + val out = context.contentResolver.openOutputStream(destination) + ?: error("Could not open destination for writing: $destination") out.use { sink -> staged.inputStream().use { source -> source.copyTo(sink) } } } catch (failure: Throwable) { if (destinationWasEmpty) deletePartialOutput(destination, failure) diff --git a/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt b/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt index 7b0dc7b..849adeb 100644 --- a/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt +++ b/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt @@ -18,6 +18,7 @@ import kotlinx.coroutines.withContext import org.libremediaconverter.convert.ConversionDependencies import org.libremediaconverter.convert.InputFile import org.libremediaconverter.convert.InputQuery +import org.libremediaconverter.convert.STAGED_FILE_GONE_MESSAGE import org.libremediaconverter.model.ConcatStrategy import org.libremediaconverter.work.ConcatWorker import org.libremediaconverter.work.JobTags @@ -221,8 +222,20 @@ class JoinViewModel @JvmOverloads constructor( activeWorkId?.let(workManager::cancelWorkById) } + /** + * Copies the staged result out to the destination the user picked. + * + * The existence check is the same one `ConversionViewModel.save` makes, for the same reason: a + * join offered by reattachment was last seen during a tag query that may be hours old, and + * `cacheDir` is reclaimed by the OS and swept by this app. Without it the file's absence + * reached the screen as a raw ENOENT path. + */ fun save(destination: Uri) { val joined = _state.value as? JoinState.Joined ?: return + if (!joined.staged.isFile) { + _state.value = JoinState.Failed(STAGED_FILE_GONE_MESSAGE) + return + } viewModelScope.launch { runCatching { withContext(Dispatchers.IO) { diff --git a/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt b/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt index 553310b..23d95d2 100644 --- a/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt +++ b/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt @@ -46,9 +46,12 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker val declaredTotal = inputData .takeIf { it.hasKeyWithValueOfType(KEY_TOTAL_BYTES) } ?.getLong(KEY_TOTAL_BYTES, 0L) - val format = OutputFormat.valueOf( - inputData.getString(KEY_FORMAT) ?: DEFAULT_FORMAT.name, - ) + // Looked up rather than `valueOf` -- see the same three reads in ConversionWorker. This one + // is above the try as well, so a format name this build does not define used to throw past + // the catch: FAILED with no error in the output Data, and no staged.delete(). + val format = inputData.getString(KEY_FORMAT) + ?.let { name -> OutputFormat.entries.firstOrNull { it.name == name } } + ?: DEFAULT_FORMAT if (!hasRoomFor(declaredTotal, uris)) { return Result.failure(workDataOf(KEY_ERROR to "Not enough free space to join these files.")) diff --git a/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt b/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt index a31249f..40e37cc 100644 --- a/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt +++ b/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt @@ -67,12 +67,18 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo .takeIf { it.hasKeyWithValueOfType(KEY_SIZE_BYTES) } ?.getLong(KEY_SIZE_BYTES, 0L) val spec = readSpec() - val quality = QualityTier.valueOf( - inputData.getString(KEY_QUALITY) ?: QualityTier.FAST.name, - ) - val preference = EnginePreference.valueOf( - inputData.getString(KEY_ENGINE_PREFERENCE) ?: EnginePreference.AUTO.name, - ) + // Looked up rather than `valueOf`, for the reason [readSpec] gives twelve lines below and + // for one more: these three reads sit ABOVE the try. A name this build does not define -- + // which is what a downgrade with work still queued produces, the same previous-version case + // JobTags is written for -- threw IllegalArgumentException straight past the catch, taking + // the retry, the error message and the staged file's delete with it. That is the escape + // shape setForeground was moved inside the try to end. + val quality = inputData.getString(KEY_QUALITY) + ?.let { name -> QualityTier.entries.firstOrNull { it.name == name } } + ?: QualityTier.FAST + val preference = inputData.getString(KEY_ENGINE_PREFERENCE) + ?.let { name -> EnginePreference.entries.firstOrNull { it.name == name } } + ?: EnginePreference.AUTO if (!hasRoomFor(declaredSize, inputUri)) { return Result.failure(workDataOf(KEY_ERROR to "Not enough free space to convert.")) diff --git a/app/src/main/java/org/libremediaconverter/work/Reattachment.kt b/app/src/main/java/org/libremediaconverter/work/Reattachment.kt index 106e3d3..3316012 100644 --- a/app/src/main/java/org/libremediaconverter/work/Reattachment.kt +++ b/app/src/main/java/org/libremediaconverter/work/Reattachment.kt @@ -104,10 +104,13 @@ sealed interface Reattachment { * result still worth offering from one already dealt with, which is why that check * carries the weight here. * - * It is also the seam for a neighbouring defect: a result the user dismissed with "Start - * over" currently keeps its staged file, so today it can be offered again on the next - * launch. Nothing here changes when that is fixed — the file stops existing and the job - * stops qualifying. + * It is also the seam a neighbouring fix acts through. "Start over" deletes the staged + * file, so a result the user dismissed stops qualifying here without this rule needing to + * know that happened — the file stops existing and the job falls out. What survives is the + * narrower gap that delete cannot close: `reset()` dispatches it to + * [kotlinx.coroutines.Dispatchers.IO] and it is cancelled with the Activity, so a + * dismissal on the way out of the app can leave the file behind. That is what + * `OutputPublisher.sweepStaging` is for, and its own KDoc names this case. * * Ranked, when more than one qualifies: * diff --git a/app/src/test/java/org/libremediaconverter/AppStartSweepTest.kt b/app/src/test/java/org/libremediaconverter/AppStartSweepTest.kt new file mode 100644 index 0000000..e5a8646 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/AppStartSweepTest.kt @@ -0,0 +1,98 @@ +package org.libremediaconverter + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Assert.fail +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.convert.StagingSweep +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File +import java.util.concurrent.TimeUnit + +/** + * That process start actually sweeps. + * + * [StagingSweepTest][org.libremediaconverter.convert.StagingSweepTest] pins the age rule and + * `OutputPublisherStagingTest` pins the sweep against a real filesystem; neither says anything + * about whether anything calls it, and deleting the one line that does left the whole suite green. + * That line is the only reason this Application class exists, and it is the backstop for every leak + * `discardStaged` cannot reach — a process reclaimed before a save, a worker that failed before its + * output ever became a `Converted` state, a `reset()` whose delete was cancelled with the Activity. + * + * `onCreate()` is called again rather than a second Application being built: it is what the + * framework calls at process start, the scope it launches on is already there, and the first test + * below is what pins that the framework calls it on *this* class. + */ +@RunWith(RobolectricTestRunner::class) +class AppStartSweepTest { + + private lateinit var app: LibreMediaConverterApp + private lateinit var stagingDir: File + + @Before + fun setUp() { + // The cast is an assertion in itself: Robolectric builds the Application named in the + // merged manifest, so this fails if `android:name` ever stops pointing here -- in which + // case the sweep below would be perfectly correct code that never runs. + app = RuntimeEnvironment.getApplication() as LibreMediaConverterApp + stagingDir = File(app.cacheDir, "conversions").apply { mkdirs() } + stagingDir.listFiles()?.forEach { it.delete() } + } + + @Test + fun `the application the manifest starts is the one that sweeps`() { + assertEquals(LibreMediaConverterApp::class.java, RuntimeEnvironment.getApplication().javaClass) + } + + @Test + fun `process start collects an abandoned staged file and leaves a live one alone`() { + val abandoned = stagedFile("abandoned.mp4") + val live = stagedFile("live.mp4") + // Set explicitly. Relying on a file being written "long enough ago" is not something a test + // can arrange, and the grace period is a day. + assertTrue( + abandoned.setLastModified(System.currentTimeMillis() - StagingSweep.GRACE_PERIOD_MS - ONE_MINUTE_MS), + ) + + // Both files are still here on the way in. The Application was already constructed once + // before this test ran, so without this the sweep that call started could be the one that + // collected the file, and the assertion below would be about the wrong process start. + assertTrue(abandoned.exists() && live.exists()) + + app.onCreate() + + awaitGone(abandoned) + // The other half, and the one that says the sweep is a sweep rather than a + // `clearStaging()`: the directory is shared by the convert tab, the join tab and + // ConcatEngine's list file, so deleting everything could take a file from a running job. + assertTrue("a file written moments ago belongs to a live job", live.exists()) + } + + /** + * Waits for [file] to be deleted. + * + * The sweep runs on `Dispatchers.IO`, deliberately: it lists a directory and stats every entry + * on the path that decides how long the launcher icon stays unresponsive. So there is nothing + * to join, and the wait is a bounded poll — long enough for a directory listing, short enough + * that a sweep which never happens fails rather than hangs. + */ + private fun awaitGone(file: File) { + val deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(AWAIT_TIMEOUT_SECONDS) + while (System.nanoTime() < deadline) { + if (!file.exists()) return + Thread.sleep(POLL_INTERVAL_MS) + } + fail("process start left ${file.name} in staging; nothing swept it") + } + + private fun stagedFile(name: String): File = File(stagingDir, name).apply { writeBytes(ByteArray(4096)) } + + private companion object { + const val ONE_MINUTE_MS = 60L * 1000 + const val AWAIT_TIMEOUT_SECONDS = 10L + const val POLL_INTERVAL_MS = 5L + } +} diff --git a/app/src/test/java/org/libremediaconverter/BackupExclusionsTest.kt b/app/src/test/java/org/libremediaconverter/BackupExclusionsTest.kt new file mode 100644 index 0000000..699ccd6 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/BackupExclusionsTest.kt @@ -0,0 +1,106 @@ +package org.libremediaconverter + +import android.content.res.XmlResourceParser +import org.junit.Assert.assertEquals +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import org.xmlpull.v1.XmlPullParser + +/** + * What the app lets leave the device. + * + * `data_extraction_rules.xml` is a resource rather than code, so nothing was checking it: reverting + * the whole file to the template's boilerplate left the unit tests green AND `lintDebug` green, and + * a future edit dropping the excludes would ship in silence. The failure it would cause is one + * nobody meets in development — a cloud restore or a device-to-device transfer. + * + * What is at stake is written in the file itself. WorkManager's queue is the app's entire backup + * payload, and every row in it references a `content://` URI granted to one install on one device + * and an output path under that install's `cacheDir`. Neither survives the transfer, and the rows + * are not inert when they arrive: reattachment queries WorkManager by tag on launch, so a fresh + * install would come up attached to a job the user never ran on it. + * + * Read out of the compiled resource table rather than off `src/main/res`, so what is asserted is + * what the APK actually carries. Note the limit of that: this pins the rules' content, not the + * `android:dataExtractionRules` attribute that points the system at them. + */ +@RunWith(RobolectricTestRunner::class) +class BackupExclusionsTest { + + @Test + fun `the work queue is excluded from cloud backup and from device transfer alike`() { + // Both sections, because they are separately honoured: `allowBackup` stays true and the + // exclusion is per-file, so an edit that dropped either half would leave the other looking + // like the whole answer. + assertEquals( + mapOf( + "cloud-backup" to WORK_MANAGER_STATE, + "device-transfer" to WORK_MANAGER_STATE, + ), + excludesBySection(), + ) + } + + /** + * Every `` in the rules, as `domain:path`, grouped by the section it sits in. + * + * Both halves of each entry, because an `` carrying no path is skipped unchecked by + * lint's own detector — so that spelling could protect nothing while still looking like a rule. + */ + private fun excludesBySection(): Map> { + // Both sections start present and empty, so a section deleted outright fails as an empty + // set rather than as a missing key -- the same finding either way, said the same way. + val found = SECTIONS.associateWith { mutableSetOf() } + var section: String? = null + RuntimeEnvironment.getApplication().resources.getXml(R.xml.data_extraction_rules).use { parser -> + while (parser.next() != XmlPullParser.END_DOCUMENT) { + section = parser.sectionAfter(section, found) + } + } + return found + } + + /** Folds one parse event into [found], and answers which section the parser is now inside. */ + private fun XmlResourceParser.sectionAfter(section: String?, found: Map>): String? = + when { + eventType == XmlPullParser.START_TAG && name in SECTIONS -> name + eventType == XmlPullParser.END_TAG && name == section -> null + eventType == XmlPullParser.START_TAG && name == "exclude" && section != null -> + section.also { found.getValue(it) += entry() } + + else -> section + } + + private fun XmlResourceParser.entry(): String = "${attribute("domain")}:${attribute("path")}" + + /** + * The value of the attribute called [name] on the current tag. + * + * Walked by index rather than looked up by namespace. These attributes carry the `android` + * namespace in the source file, but a parser over the *compiled* resource reports them with + * none, so `getAttributeValue(namespace, name)` answers null for every one of them. + */ + private fun XmlResourceParser.attribute(name: String): String? = + (0 until attributeCount).firstOrNull { getAttributeName(it) == name }?.let { getAttributeValue(it) } + + private companion object { + /** The two ways data leaves a device, both of which these rules have to answer. */ + val SECTIONS = setOf("cloud-backup", "device-transfer") + + /** + * WorkManager's own storage, spelled the way WorkManager spells it. + * + * The database is Room-backed and therefore in WAL mode, hence the two sidecars. Pinning + * the spelling is the point rather than a cost: a WorkManager release renaming its database + * would silently un-exclude the queue, and this failing is how anyone would find out. + */ + val WORK_MANAGER_STATE = setOf( + "database:androidx.work.workdb", + "database:androidx.work.workdb-wal", + "database:androidx.work.workdb-shm", + "sharedpref:androidx.work.util.preferences.xml", + ) + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/MissingStagedFileTest.kt b/app/src/test/java/org/libremediaconverter/convert/MissingStagedFileTest.kt new file mode 100644 index 0000000..1692b00 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/MissingStagedFileTest.kt @@ -0,0 +1,107 @@ +package org.libremediaconverter.convert + +import android.app.Application +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.workDataOf +import kotlinx.coroutines.Dispatchers +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.join.JoinState +import org.libremediaconverter.join.JoinViewModel +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.work.ConcatWorker +import org.libremediaconverter.work.ConversionWorker +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import org.robolectric.Shadows.shadowOf +import java.io.ByteArrayOutputStream +import java.io.File + +/** + * What the user is told when the file went away between being offered and being saved. + * + * Not a corner: staging is `cacheDir`, which is what the OS empties when it wants space, and the + * sweep collects anything a day old. Reattachment is where the two are furthest apart — the check + * that decided the file existed ran inside a tag query on launch, and the Save button may not be + * tapped for hours. + * + * The real [OutputPublisher] rather than the recording stub, because the defect is what the *real* + * publish does with a staged file that is not there: `staged.inputStream()` throws, and `save()` + * put `e.message` on screen — a `/data/user/0/…/4b4882….mp4: open failed: ENOENT` path the user has + * never seen and can do nothing with. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class MissingStagedFileTest { + + private lateinit var app: Application + private lateinit var publisher: OutputPublisher + private lateinit var staged: File + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = OutputPublisher(app) + ConversionDependencies.publisher = { publisher } + ConversionDependencies.probe = { _, _ -> InputProbe() } + + staged = publisher.createStagingFile("holiday_converted.mp4").apply { writeBytes(ByteArray(4096)) } + installTestWorkManager( + app, + workDataOf( + ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath, + ConcatWorker.KEY_OUTPUT_PATH to staged.absolutePath, + ), + ) + // A destination that really opens, so the save gets far enough to reach the staged file. + // Without this the failure would be about the destination and the test would pass while + // saying nothing. + shadowOf(app.contentResolver).registerOutputStreamSupplier(DESTINATION) { ByteArrayOutputStream() } + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `saving a conversion whose staged file has gone says so in a sentence`() { + val viewModel = ConversionViewModel(app, Dispatchers.Unconfined) + viewModel.onInputPicked(Uri.parse("content://test/holiday.mp4")) + awaitState(viewModel.state, "Ready") { it is ConversionState.Ready } + viewModel.convert() + awaitState(viewModel.state, "Converted") { it is ConversionState.Converted } + + assertTrue("the fixture must start with a real staged file", staged.delete()) + + viewModel.save(DESTINATION) + + val failed = awaitState(viewModel.state, "Failed") { it is ConversionState.Failed } + assertEquals(STAGED_FILE_GONE_MESSAGE, (failed as ConversionState.Failed).message) + } + + @Test + fun `saving a join whose staged file has gone says so in a sentence`() { + val viewModel = JoinViewModel(app, Dispatchers.Unconfined) + viewModel.onInputsPicked(listOf(Uri.parse("content://test/a.mp4"), Uri.parse("content://test/b.mp4"))) + awaitState(viewModel.state, "Ready") { it is JoinState.Ready } + viewModel.join() + awaitState(viewModel.state, "Joined") { it is JoinState.Joined } + + assertTrue("the fixture must start with a real staged file", staged.delete()) + + viewModel.save(DESTINATION) + + val failed = awaitState(viewModel.state, "Failed") { it is JoinState.Failed } + assertEquals(STAGED_FILE_GONE_MESSAGE, (failed as JoinState.Failed).message) + } + + private companion object { + val DESTINATION: Uri = Uri.parse("content://test/destination.mp4") + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt index 09f1adc..88431f6 100644 --- a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt @@ -265,10 +265,34 @@ class OutputPublisherPublishTest { ) } + @Test + fun `a destination the provider will not open does not stay behind as an empty file`() { + // No stream supplier is registered for this URI and the fake provider does not implement + // openFile, which is a provider that has gone away between the picker and the write. + // + // The document exists all the same: SAF's CreateDocument contract created it before + // publish() was ever called, so "nothing has been written yet" was never the same claim as + // "there is nothing of ours here". Leaving it means a zero-byte file at the name the user + // chose, while the screen says the save failed. + val destination = FakeSafProvider.backingFile(documentUri) + assertEquals("the fixture starts as the empty document SAF hands back", 0L, destination.length()) + + val failure = runCatching { publisher.publish(staged, documentUri) }.exceptionOrNull() + + assertTrue("a destination that will not open must not appear to succeed, got $failure", failure != null) + assertEquals(listOf(documentUri), FakeSafProvider.deleteRequests) + assertFalse( + "a zero-byte file must not be left at the name the user picked", + destination.exists(), + ) + } + @Test fun `a destination that cannot be opened at all fails without any cleanup`() { - // The JVM twin of UnopenableUriTest's unwritable-destination case. Nothing was - // written, so there is nothing of ours to remove. + // The JVM twin of UnopenableUriTest's unwritable-destination case. The open sits inside + // the guarded region now, so what keeps this one untouched is the guard rather than the + // placement: nothing answers for that authority, so no size can be read, and "I could not + // tell" must never authorise a delete. val failure = runCatching { publisher.publish(staged, deadUri) }.exceptionOrNull() assertTrue("publishing to a dead provider must not appear to succeed, got $failure", failure != null) diff --git a/app/src/test/java/org/libremediaconverter/convert/ReattachGuardsTest.kt b/app/src/test/java/org/libremediaconverter/convert/ReattachGuardsTest.kt new file mode 100644 index 0000000..68dc507 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/ReattachGuardsTest.kt @@ -0,0 +1,214 @@ +package org.libremediaconverter.convert + +import android.app.Application +import android.net.Uri +import android.os.Looper +import android.util.Log +import androidx.media3.common.util.UnstableApi +import androidx.work.Configuration +import androidx.work.WorkManager +import androidx.work.testing.SynchronousExecutor +import androidx.work.testing.WorkManagerTestInitHelper +import androidx.work.workDataOf +import kotlinx.coroutines.Dispatchers +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.model.InputProbe +import org.libremediaconverter.work.ConversionWorker +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import org.robolectric.Shadows.shadowOf +import java.io.File +import java.util.concurrent.CountDownLatch +import java.util.concurrent.Executor +import java.util.concurrent.TimeUnit + +/** + * The two decisions `reattach()` makes that [org.libremediaconverter.work.Reattachment] cannot. + * + * `Reattachment.choose` answers "which job", and twenty tests pin it. What it does not decide is + * whether the answer may still be used by the time it arrives, or how much of it the card is + * allowed to believe — and both of those live in the ViewModel, where nothing was asserting them. + * Deleting either guard left the whole suite green. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class ReattachGuardsTest { + + private lateinit var app: Application + private lateinit var publisher: RecordingPublisher + private lateinit var workManager: WorkManager + private lateinit var staged: File + private lateinit var queries: HoldableTaskExecutor + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = RecordingPublisher(app) + ConversionDependencies.publisher = { publisher } + ConversionDependencies.probe = { _, _ -> InputProbe() } + + staged = publisher.createStagingFile("holiday_converted.mp4").apply { writeBytes(ByteArray(4096)) } + queries = HoldableTaskExecutor() + WorkManagerTestInitHelper.initializeTestWorkManager( + app, + Configuration.Builder() + .setMinimumLoggingLevel(Log.ASSERT) + .setExecutor(SynchronousExecutor()) + .setTaskExecutor(queries) + .setWorkerFactory( + SucceedingWorkerFactory(workDataOf(ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath)), + ) + .build(), + ) + workManager = WorkManager.getInstance(app) + } + + @After + fun tearDown() { + queries.release() + ConversionDependencies.reset() + } + + /** + * The race the guard exists for: the tag query suspends, and while it is away the user picks a + * file of their own. Reattaching over that would throw away what they just did — and, worse, + * point the Save button at yesterday's file while the card named today's. + * + * Made deterministic by holding WorkManager's task executor rather than by hoping the pick wins: + * the query cannot complete until this test lets it, so the pick has landed before the guard is + * ever reached. + */ + @Test + fun `a file picked while the query was in flight is not reattached over`() { + finishAConversionWithNobodyWatching() + val picked = File(app.cacheDir, "beach.mp4").apply { writeBytes(ByteArray(2048)) } + + queries.hold() + val viewModel = ConversionViewModel(app, Dispatchers.Unconfined) + viewModel.onInputPicked(Uri.fromFile(picked)) + val ready = awaitState(viewModel.state, "Ready") { it is ConversionState.Ready } + // The URI, because that is what tells the two inputs apart: a reattached job's is + // Uri.EMPTY -- WorkManager never hands back the Data a request was enqueued with -- while a + // picked file's is the one the picker returned. + assertEquals(Uri.fromFile(picked), (ready as ConversionState.Ready).input.uri) + + queries.release() + settle() + + // The query really did run and really did reach the guard -- without this the assertion + // below would pass just as well against a reattachment that never arrived. + assertTrue("the reattach query should have been held, then run", queries.heldTasks > 0) + val current = viewModel.state.value + assertTrue("the user's pick must survive a late reattachment, got $current", current is ConversionState.Ready) + assertEquals(Uri.fromFile(picked), (current as ConversionState.Ready).input.uri) + assertEquals(2_048L, current.input.sizeBytes) + } + + /** + * Two finished jobs naming one staged file, which is exactly what the device produced before + * staging was keyed on the job id. + * + * The file is the user's either way, so it is still offered. Which job wrote it is not + * knowable, so the card must not borrow either job's input name: a card labelled with the other + * conversion's file is a confident lie, where a neutral label is merely thin. + */ + @Test + fun `a result two jobs both claim is offered without being attributed to either`() { + finishAConversionWithNobodyWatching(displayName = "holiday.mp4") + finishAConversionWithNobodyWatching(displayName = "beach.mp4") + + val viewModel = ConversionViewModel(app, Dispatchers.Unconfined) + val converted = awaitState(viewModel.state, "Converted") { it is ConversionState.Converted } + + converted as ConversionState.Converted + assertEquals( + "the bytes on disk are what the user gets back", + staged.absolutePath, + converted.staged.absolutePath, + ) + assertEquals( + "neither job's name may be claimed for the other's file", + "Media file", + converted.input.displayName, + ) + // The size travels in the same tags as the name, so it goes the same way rather than being + // reported as one job's number against the other job's file. + assertEquals(null, converted.input.sizeBytes) + } + + /** Pumps the main looper for long enough that anything already dispatched has run. */ + private fun settle() { + repeat(SETTLE_PUMPS) { + shadowOf(Looper.getMainLooper()).idle() + Thread.sleep(SETTLE_INTERVAL_MS) + } + } + + private fun finishAConversionWithNobodyWatching(displayName: String = "holiday.mp4") { + workManager.enqueue( + ConversionWorker.request( + inputUri = Uri.parse("content://test/$displayName"), + displayName = displayName, + sizeBytes = 4_096L, + ), + ).result.get() + } + + private companion object { + const val SETTLE_PUMPS = 60 + const val SETTLE_INTERVAL_MS = 5L + } +} + +/** + * WorkManager's task executor, with a brake the test can apply. + * + * The reattachment query is a suspending call the ViewModel makes in `init`, so a test that wants + * to act "while it is in flight" has to be able to stop it finishing. Holding the executor it runs + * on is the only seam for that: `jobSnapshots` takes no dispatcher, and racing it would make the + * assertion depend on which of two IO hops happened to return first. + * + * Never applied on the main thread. The test releases the brake from there, so a wait taken on that + * thread would deadlock the loop that was going to end it. The wait is bounded for the same class of + * reason: a wiring mistake should turn the test red, not hang the build. + */ +private class HoldableTaskExecutor : Executor { + + private val released = CountDownLatch(1) + + @Volatile + private var holding = false + + /** How many tasks were actually held. Zero means the brake never gripped anything. */ + @Volatile + var heldTasks = 0 + private set + + fun hold() { + holding = true + } + + fun release() { + holding = false + released.countDown() + } + + override fun execute(command: Runnable) { + if (holding && Looper.myLooper() != Looper.getMainLooper()) { + heldTasks++ + check(released.await(HOLD_TIMEOUT_SECONDS, TimeUnit.SECONDS)) { + "a held WorkManager task was never released" + } + } + command.run() + } + + private companion object { + const val HOLD_TIMEOUT_SECONDS = 10L + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/SpaceArithmeticTest.kt b/app/src/test/java/org/libremediaconverter/convert/SpaceArithmeticTest.kt new file mode 100644 index 0000000..da32743 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/SpaceArithmeticTest.kt @@ -0,0 +1,79 @@ +package org.libremediaconverter.convert + +import android.content.Context +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +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 + +/** + * The two sums the space check is made of, at the sizes where addition stops working. + * + * `SpaceCheckTest` pins which *question* each worker asks; this pins what the answer is once the + * number is large. Both halves were live on main: `hasSpaceFor` added the headroom to the request + * before comparing, and [InputQuery.total] folded a join's inputs with nothing stopping the sum + * from wrapping. A wrapped total is not merely nonsense — it is negative, and every free-space + * measurement beats a negative number, so the check that exists to refuse impossible jobs approved + * the most impossible one it can be handed. + * + * Nothing here is about the *allocatable-versus-usable* question, which is a separate decision + * still parked. This is the arithmetic on whichever number that decision ends up producing. + */ +@RunWith(RobolectricTestRunner::class) +class SpaceArithmeticTest { + + private lateinit var context: Context + private lateinit var publisher: OutputPublisher + + @Before + fun setUp() { + context = RuntimeEnvironment.getApplication() + publisher = OutputPublisher(context) + } + + @Test + fun `a request no disk could hold is refused rather than wrapping into plenty of room`() { + assertFalse("eight exabytes do not fit anywhere", publisher.hasSpaceFor(Long.MAX_VALUE)) + // Just inside the headroom of the maximum, which is the arithmetic's actual edge: this is + // the range where `bytes + headroom` goes negative while `bytes` alone still looks huge. + assertFalse(publisher.hasSpaceFor(Long.MAX_VALUE - ONE_HUNDRED_MIB)) + } + + // The clamp on a negative size is deliberately NOT asserted here. It only changes the answer + // when free space is below the headroom, which this test cannot arrange -- the publisher reads + // the host's real cache volume -- so any assertion available would pass against the unclamped + // arithmetic too, and a test that cannot fail is worse than the gap it appears to close. + + @Test + fun `an ordinary request is still allowed, so the refusals above are not vacuous`() { + val free = File(context.cacheDir, "conversions").usableSpace + assertTrue( + "a one-byte conversion must fit; the volume under the cache reports $free bytes free", + publisher.hasSpaceFor(1L), + ) + } + + @Test + fun `a join total too large to represent saturates instead of turning negative`() { + val enormous = listOf(FOUR_EXABYTES, FOUR_EXABYTES, FOUR_EXABYTES) + + val total = InputQuery.total(enormous) + + assertEquals(Long.MAX_VALUE, total) + // The whole point, in the shape the defect had: this total is handed straight to the space + // check by ConcatWorker, and before the clamp it arrived negative and was approved. + assertFalse("a join of three four-exabyte files does not fit", publisher.hasSpaceFor(total!!)) + } + + private companion object { + const val ONE_HUNDRED_MIB = 100L * 1024 * 1024 + + /** Big enough that three of them overflow, small enough to be a plausible `statSize`. */ + const val FOUR_EXABYTES = 4_000_000_000_000_000_000L + } +} diff --git a/app/src/test/java/org/libremediaconverter/work/FailureOutcomeTest.kt b/app/src/test/java/org/libremediaconverter/work/FailureOutcomeTest.kt index d342137..f064ded 100644 --- a/app/src/test/java/org/libremediaconverter/work/FailureOutcomeTest.kt +++ b/app/src/test/java/org/libremediaconverter/work/FailureOutcomeTest.kt @@ -2,7 +2,9 @@ package org.libremediaconverter.work import android.app.ForegroundServiceStartNotAllowedException import androidx.work.WorkInfo +import androidx.work.WorkRequest import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner @@ -144,8 +146,48 @@ class FailureOutcomeTest { ) } + @Test + fun `the attempt bound outlasts a night rather than being a round number`() { + // The KDoc argues the value rather than picking one: ten attempts against WorkManager's + // default backoff span about eight and a half hours, which is what turns "the user will + // have opened the app before this gives up" into a claim instead of a hope. + // + // Asserted as that span rather than as the literal 10, so a deliberate re-tune keeping the + // property passes while an accidental one fails. The accident is not hypothetical: at 2 the + // job gives up about ninety seconds after process death -- losing exactly the long + // conversion the retry exists to protect -- and every other test in this file still passes, + // because they are all written against the constant rather than against its value. + var delay = WorkRequest.DEFAULT_BACKOFF_DELAY_MILLIS + var span = 0L + repeat(FailureOutcome.MAX_FOREGROUND_START_ATTEMPTS) { + span += delay + // Doubling per attempt, clamped, exactly as WorkManager schedules it. Coerced each + // time round so the arithmetic cannot overflow whatever the bound is set to. + delay = (delay * 2).coerceAtMost(WorkRequest.MAX_BACKOFF_MILLIS) + } + + assertTrue( + "the retry budget must outlast a night; it spans ${span / MILLIS_PER_HOUR.toDouble()} hours", + span >= MINIMUM_RETRY_SPAN_MS, + ) + } + private fun denied() = ForegroundServiceStartNotAllowedException( "startForegroundService() not allowed: service " + "org.libremediaconverter/androidx.work.impl.foreground.SystemForegroundService", ) + + private companion object { + const val MILLIS_PER_HOUR = 60L * 60 * 1000 + + /** + * How long the denied-start retries have to keep going. + * + * Eight hours rather than the eight and a half the default backoff actually produces. The + * claim is about covering a night between one opening of the app and the next; pinning the + * exact arithmetic would instead fail on a WorkManager release that re-tuned its own + * constants without anything this app decides having changed. + */ + const val MINIMUM_RETRY_SPAN_MS = 8L * MILLIS_PER_HOUR + } } diff --git a/app/src/test/java/org/libremediaconverter/work/JobSnapshotsTest.kt b/app/src/test/java/org/libremediaconverter/work/JobSnapshotsTest.kt new file mode 100644 index 0000000..4af2fda --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/JobSnapshotsTest.kt @@ -0,0 +1,183 @@ +package org.libremediaconverter.work + +import android.app.Application +import android.content.Context +import android.util.Log +import androidx.media3.common.util.UnstableApi +import androidx.work.Configuration +import androidx.work.ListenableWorker +import androidx.work.OneTimeWorkRequestBuilder +import androidx.work.WorkManager +import androidx.work.Worker +import androidx.work.WorkerFactory +import androidx.work.WorkerParameters +import androidx.work.testing.SynchronousExecutor +import androidx.work.testing.WorkManagerTestInitHelper +import androidx.work.workDataOf +import kotlinx.coroutines.runBlocking +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.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File + +/** + * The edge that feeds the reattachment decision. + * + * [Reattachment.choose] is a pure function with twenty tests, and every input it reasons over is + * computed here — by the one part of reattachment that has to touch WorkManager and the + * filesystem. That asymmetry was the gap: the rule was pinned exhaustively while the values it + * ran on were pinned nowhere, so a regression in this file left the whole suite green. Two + * demonstrated ones: dropping the empty-file filter offered a zero-byte staged file as a savable + * result, and hardcoding [JobSnapshot.outputModifiedAt] to zero starved the newest-file tie-break + * of the only data it has. + * + * A real `WorkManager` and a real `cacheDir`, because both are what the code under test is for. + * The worker never runs: [EchoingWorkerFactory] stands in for a job that finished in a process + * that no longer exists, which is the only way a snapshot with an output path comes to exist at + * all. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class JobSnapshotsTest { + + private lateinit var app: Application + private lateinit var workManager: WorkManager + private lateinit var stagingDir: File + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + WorkManagerTestInitHelper.initializeTestWorkManager( + app, + Configuration.Builder() + .setMinimumLoggingLevel(Log.ASSERT) + .setExecutor(SynchronousExecutor()) + .setTaskExecutor(SynchronousExecutor()) + .setWorkerFactory(EchoingWorkerFactory) + .build(), + ) + workManager = WorkManager.getInstance(app) + stagingDir = File(app.cacheDir, "conversions").apply { mkdirs() } + stagingDir.listFiles()?.forEach { it.delete() } + } + + @Test + fun `a staged file with nothing in it is not an output`() { + // Zero bytes is what a job killed before its engine wrote anything leaves behind. Treating + // it as a result would publish it: the user taps Save and gets a zero-byte "conversion" + // rather than a message, which is worse than not being offered it. + val empty = stagedFile("empty.mp4", bytes = 0) + val real = stagedFile("real.mp4", bytes = 4096) + // Never created at all -- the OS reclaimed the cache, or the file was saved and deleted. + val reclaimed = File(stagingDir, "reclaimed.mp4") + listOf(empty, real, reclaimed).forEach(::finishedWithOutput) + + val snapshots = snapshots() + + assertEquals( + "only a file with bytes in it is a result", + mapOf( + empty.absolutePath to false, + real.absolutePath to true, + reclaimed.absolutePath to false, + ), + snapshots.associate { it.outputPath to it.outputExists }, + ) + // The path is still reported for all three. It is what the worker said; whether it still + // names anything is the separate question above. + assertEquals( + setOf(empty.absolutePath, real.absolutePath, reclaimed.absolutePath), + snapshots.mapNotNull { it.outputPath }.toSet(), + ) + // And a file that is not an output has no time either: an mtime read off a zero-byte + // leftover would feed the tie-break a moment nothing produced. + assertEquals(0L, snapshotFor(snapshots, empty).outputModifiedAt) + } + + @Test + fun `each result carries the time its own file was last written`() { + val older = stagedFile("older.mp4", bytes = 4096) + val newer = stagedFile("newer.mp4", bytes = 4096) + // Set explicitly rather than relying on the order the two were written: a filesystem is + // free to give both the same mtime, and then the fixture would be testing nothing. + assertTrue(older.setLastModified(OLDER_MS)) + assertTrue(newer.setLastModified(NEWER_MS)) + assertTrue( + "the two fixtures must really carry different times, got ${older.lastModified()}", + older.lastModified() < newer.lastModified(), + ) + listOf(older, newer).forEach(::finishedWithOutput) + + val snapshots = snapshots() + + // Compared against what the filesystem stored rather than against what was requested, + // because mtime granularity is the filesystem's business and not this test's claim. + assertEquals(older.lastModified(), snapshotFor(snapshots, older).outputModifiedAt) + assertEquals(newer.lastModified(), snapshotFor(snapshots, newer).outputModifiedAt) + + // Why the field exists, asserted through the rule that reads it: the tag query has no + // ORDER BY, so without a real time here an arbitrary winner would win every launch while + // the other result stayed unreachable for as long as its file existed. + assertEquals(newer.absolutePath, Reattachment.choose(snapshots)?.job?.outputPath) + } + + private fun snapshots(): List = runBlocking { + workManager.jobSnapshots( + tag = ConversionWorker::class.java.name, + outputPathKey = ConversionWorker.KEY_OUTPUT_PATH, + ) + } + + private fun snapshotFor(snapshots: List, output: File): JobSnapshot = + snapshots.single { it.outputPath == output.absolutePath } + + private fun stagedFile(name: String, bytes: Int): File = + File(stagingDir, name).apply { writeBytes(ByteArray(bytes)) } + + /** + * A conversion that finished with [output] as its result and nobody watching. + * + * Built rather than taken from `ConversionWorker.request`, because what has to reach + * `jobSnapshots` is the *output* `Data` of a finished job, and a request only carries input. + */ + private fun finishedWithOutput(output: File) { + workManager.enqueue( + OneTimeWorkRequestBuilder() + .setInputData(workDataOf(ConversionWorker.KEY_OUTPUT_PATH to output.absolutePath)) + .build(), + ).result.get() + } + + private companion object { + /** Two fixed moments a day apart, so the ordering is stated rather than raced for. */ + const val OLDER_MS = 1_700_000_000_000L + const val NEWER_MS = OLDER_MS + 24L * 60 * 60 * 1000 + } +} + +/** + * Stands in for whichever job finished before this process existed, reporting the output path it + * was handed. + * + * The real [ConversionWorker] cannot run here — it drives Media3 and FFmpeg through native + * libraries that do not exist on the JVM — and what `jobSnapshots` needs from it is only a + * SUCCEEDED `WorkInfo` carrying an output path. Echoing the input means one factory can produce + * several jobs with results of their own, which is what the ordering and aliasing cases need. + */ +private object EchoingWorkerFactory : WorkerFactory() { + override fun createWorker( + appContext: Context, + workerClassName: String, + workerParameters: WorkerParameters, + ): ListenableWorker = object : Worker(appContext, workerParameters) { + override fun doWork(): Result { + val path = inputData.getString(ConversionWorker.KEY_OUTPUT_PATH) + ?: return Result.success() + return Result.success(workDataOf(ConversionWorker.KEY_OUTPUT_PATH to path)) + } + } +} diff --git a/app/src/test/java/org/libremediaconverter/work/PerJobStagingTest.kt b/app/src/test/java/org/libremediaconverter/work/PerJobStagingTest.kt index 0716e7b..7439dc1 100644 --- a/app/src/test/java/org/libremediaconverter/work/PerJobStagingTest.kt +++ b/app/src/test/java/org/libremediaconverter/work/PerJobStagingTest.kt @@ -10,11 +10,11 @@ import kotlinx.coroutines.runBlocking import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse +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.installTestWorkManager import org.libremediaconverter.model.DeviceCodecs import org.libremediaconverter.model.EnginePreference @@ -35,19 +35,24 @@ import java.util.UUID * * These drive the real worker rather than the naming function, because the naming function was * never the part that was wrong. What was wrong is which name the worker asked for. + * + * Both workers, for the same reason. The join side collided harder — `joined.` is one string + * for every join of a format, where a conversion at least needed two inputs of the same name — and + * it was the half with no test at all: reverting `ConcatWorker` to that constant left all 257 + * tests green. */ @UnstableApi @RunWith(RobolectricTestRunner::class) class PerJobStagingTest { private lateinit var app: Application - private lateinit var publisher: OutputPublisher + private lateinit var publisher: NamingPublisher private lateinit var stagingDir: File @Before fun setUp() { app = RuntimeEnvironment.getApplication() - publisher = AlwaysRoomPublisher(app) + publisher = NamingPublisher(app) ConversionDependencies.publisher = { publisher } ConversionDependencies.probe = { _, _ -> InputProbe() } ConversionDependencies.deviceCodecs = { DeviceCodecs.PERMISSIVE } @@ -56,6 +61,9 @@ class PerJobStagingTest { stagingDir = publisher.createStagingFile("anything").parentFile!! stagingDir.listFiles()?.forEach { it.delete() } + // Asking for the directory above is itself a staging request; the tests are about the ones + // the workers make. + publisher.requestedNames.clear() } @After @@ -104,8 +112,34 @@ class PerJobStagingTest { ) } + @Test + fun `two joins of the same format stage under names of their own`() { + runBlocking { concatWorker(JOB_A).doWork() } + runBlocking { concatWorker(JOB_B).doWork() } + + // Read off what the worker asked for rather than off the directory, and not for + // convenience: ConcatEngine is native, so neither join gets past it here, and the catch on + // the way out deletes whatever was staged. The name is where the collision lived -- + // "joined.${format.extension}" is one string for every join of a format, so two joins were + // one file, exactly as two conversions of a same-named input were. + val names = publisher.requestedNames + assertEquals("each join must stage under a name of its own, got $names", 2, names.toSet().size) + assertTrue("the first join's name must carry its own job id, got ${names[0]}", names[0].contains("$JOB_A")) + assertTrue("the second join's name must carry its own job id, got ${names[1]}", names[1].contains("$JOB_B")) + } + private fun stagedNames(): List = stagingDir.listFiles().orEmpty().map { it.name }.sorted() + private fun concatWorker(id: UUID): ConcatWorker = TestListenableWorkerBuilder( + context = app, + inputData = workDataOf( + ConcatWorker.KEY_INPUT_URIS to arrayOf(INPUT.toString(), "file:///tmp/second.mp4"), + ConcatWorker.KEY_TOTAL_BYTES to INPUT_BYTES, + ConcatWorker.KEY_FORMAT to JOIN_FORMAT.name, + ), + runAttemptCount = 0, + ).setId(id).build() + private fun conversionWorker( id: UUID, runAttemptCount: Int = 0, @@ -129,6 +163,7 @@ class PerJobStagingTest { const val DISPLAY_NAME = "input.mp4" const val INPUT_BYTES = 1024L val SPEC = OutputFormat.MP4_H265.spec + val JOIN_FORMAT = OutputFormat.MP4_H264 val JOB_A: UUID = UUID.fromString("00000000-0000-4000-8000-00000000000a") val JOB_B: UUID = UUID.fromString("00000000-0000-4000-8000-00000000000b") } diff --git a/app/src/test/java/org/libremediaconverter/work/ReattachmentTest.kt b/app/src/test/java/org/libremediaconverter/work/ReattachmentTest.kt index 42725ff..43fcf5b 100644 --- a/app/src/test/java/org/libremediaconverter/work/ReattachmentTest.kt +++ b/app/src/test/java/org/libremediaconverter/work/ReattachmentTest.kt @@ -35,6 +35,16 @@ class ReattachmentTest { assertNull(Reattachment.choose(listOf(job(state = WorkInfo.State.FAILED)))) } + @Test + fun `a failed job is not reattached to even when it left a file behind`() { + // The fixture that matters, and the one every other FAILED case here was missing: a job + // killed mid-write leaves a partial in staging -- the 2 MB orphan the device pass found -- + // so the exclusion has to hold for a FAILED job that really does name a file on disk. + // Ranking it like a result would offer the user a truncated file with a Save button. + val partial = job(state = WorkInfo.State.FAILED, outputPath = STAGED, outputExists = true) + assertNull(Reattachment.choose(listOf(partial))) + } + @Test fun `a finished result still on disk is offered`() { val result = job(state = WorkInfo.State.SUCCEEDED, outputPath = "/cache/out.mp4", outputExists = true) diff --git a/app/src/test/java/org/libremediaconverter/work/WorkerEnumFallbackTest.kt b/app/src/test/java/org/libremediaconverter/work/WorkerEnumFallbackTest.kt new file mode 100644 index 0000000..32cea02 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/WorkerEnumFallbackTest.kt @@ -0,0 +1,173 @@ +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.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.convert.ConversionDependencies +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 +import org.libremediaconverter.model.EnginePreference +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.model.OutputFormat +import org.libremediaconverter.model.QualityTier +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File +import java.util.UUID + +/** + * Input `Data` naming something this build does not define. + * + * Not a malformed-input hypothetical: WorkManager keeps queued and finished work for about a week, + * so a downgrade — or any rollback with work still in the queue — hands this build a job enqueued + * by another one. That is the same previous-version case [JobTags] is written for. + * + * What made it worth a test is *where* the reads are. All three sit above the workers' `try`, so an + * unknown name threw `IllegalArgumentException` out of `doWork()` entirely: WorkManager logged + * FAILURE with `reschedule = false`, the output `Data` reached the UI with zero entries so the + * screen said "Conversion failed." with nothing else, and the staged file was never deleted. That + * is the signature `setForeground` was moved inside the `try` to end, reached through a different + * door. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class WorkerEnumFallbackTest { + + private lateinit var app: Application + private lateinit var publisher: NamingPublisher + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = NamingPublisher(app) + ConversionDependencies.publisher = { publisher } + ConversionDependencies.probe = { _, _ -> InputProbe() } + ConversionDependencies.deviceCodecs = { DeviceCodecs.PERMISSIVE } + // The progress notification builds its cancel action from WorkManager.getInstance(). + installTestWorkManager(app, Data.EMPTY) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `a quality tier this build does not define falls back to the default`() { + val transcoder = RequestRecordingTranscoder() + ConversionDependencies.software = { transcoder } + + val result = runBlocking { conversionWorker(quality = "ULTRA_FIDELITY").doWork() } + + // A Result at all is half the assertion -- the read is above the try, so the defect was an + // exception rather than a wrong answer. The other half is which tier ran: falling back to + // something arbitrary would silently convert at a quality nobody asked for. + assertEquals(ListenableWorker.Result.success(), stripOutput(result)) + assertEquals(listOf(QualityTier.FAST), transcoder.qualities) + } + + @Test + fun `an engine preference this build does not define does not end the job`() { + // Refused on space, which is the first thing below the three reads: it proves the reads + // were reached and returned, without dragging in a routing decision this test is not about. + publisher.refuseSpace = true + + val result = runBlocking { conversionWorker(preference = "FORCE_QUANTUM").doWork() } + + assertEquals( + ListenableWorker.Result.failure( + workDataOf(ConversionWorker.KEY_ERROR to "Not enough free space to convert."), + ), + result, + ) + } + + @Test + fun `an output format this build does not define falls back to the default`() { + runBlocking { concatWorker(format = "AVI_MPEG4").doWork() } + + // The join itself fails -- ConcatEngine is native and there is no seam for it here -- so + // what is asserted is the name it staged under, which is where the format actually lands. + // A format nobody could resolve must produce the default's extension, not no extension and + // not a throw on the way past. + assertEquals( + listOf(StagingNames.forJob(CONCAT_ID, ConcatWorker.DEFAULT_FORMAT.extension)), + publisher.requestedNames, + ) + } + + /** [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 conversionWorker( + quality: String = QualityTier.FAST.name, + preference: String = EnginePreference.FORCE_SOFTWARE.name, + ): ConversionWorker = TestListenableWorkerBuilder( + context = app, + inputData = workDataOf( + ConversionWorker.KEY_INPUT_URI to INPUT.toString(), + ConversionWorker.KEY_DISPLAY_NAME to DISPLAY_NAME, + ConversionWorker.KEY_SIZE_BYTES to INPUT_BYTES, + ConversionWorker.KEY_CONTAINER to SPEC.container.name, + ConversionWorker.KEY_VIDEO_CODEC to SPEC.videoCodec.name, + ConversionWorker.KEY_AUDIO_CODEC to SPEC.audioCodec.name, + ConversionWorker.KEY_QUALITY to quality, + ConversionWorker.KEY_ENGINE_PREFERENCE to preference, + ), + runAttemptCount = 0, + ).setId(CONVERSION_ID).build() + + private fun concatWorker(format: String): ConcatWorker = TestListenableWorkerBuilder( + context = app, + inputData = workDataOf( + ConcatWorker.KEY_INPUT_URIS to arrayOf(INPUT.toString(), "file:///tmp/second.mp4"), + ConcatWorker.KEY_TOTAL_BYTES to INPUT_BYTES, + ConcatWorker.KEY_FORMAT to format, + ), + runAttemptCount = 0, + ).setId(CONCAT_ID).build() + + private companion object { + val INPUT: Uri = Uri.parse("file:///tmp/holiday.mp4") + const val DISPLAY_NAME = "holiday.mp4" + const val INPUT_BYTES = 1024L + val SPEC = OutputFormat.MP4_H265.spec + val CONVERSION_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000021") + val CONCAT_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000022") + } +} + +/** An engine that writes the output and remembers what it was asked to produce. */ +private class RequestRecordingTranscoder : SoftwareTranscoder { + + val qualities = mutableListOf() + + override suspend fun run( + request: ConversionRequest, + inputPath: String, + output: File, + durationMs: Long, + onProgress: (Int) -> Unit, + ) { + qualities += request.quality + output.writeBytes(ByteArray(OUTPUT_BYTES)) + } + + private companion object { + const val OUTPUT_BYTES = 512 + } +} diff --git a/app/src/test/java/org/libremediaconverter/work/WorkerStubs.kt b/app/src/test/java/org/libremediaconverter/work/WorkerStubs.kt index 6a16964..848c04b 100644 --- a/app/src/test/java/org/libremediaconverter/work/WorkerStubs.kt +++ b/app/src/test/java/org/libremediaconverter/work/WorkerStubs.kt @@ -24,6 +24,31 @@ open class AlwaysRoomPublisher(context: Context) : OutputPublisher(context) { override fun hasSpaceFor(bytes: Long): Boolean = true } +/** + * An [AlwaysRoomPublisher] that records the staging names it is asked for. + * + * For a conversion the staged file survives the job and a directory listing says everything. For a + * join it does not: `ConcatEngine` is native, so no test here gets past it, and the catch on the way + * out deletes what was staged. The name the worker *asked* for is then the only place its job id + * and its output format are legible at all — the same reason `SpaceCheckTest` records the question + * rather than the verdict. + */ +open class NamingPublisher(context: Context) : AlwaysRoomPublisher(context) { + + /** Every name passed to [createStagingFile], in order. */ + val requestedNames = mutableListOf() + + /** Set to refuse every space check, the way `FakeFailures.FullDisk` does. */ + var refuseSpace = false + + override fun hasSpaceFor(bytes: Long): Boolean = !refuseSpace + + override fun createStagingFile(name: String): File { + requestedNames += name + return super.createStagingFile(name) + } +} + /** * An engine that writes the output file and nothing else. *