From c5c4c5323bde1abfed61e117bb1b90ae4cab4cf5 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 22 Aug 2026 23:06:37 -0500 Subject: [PATCH 1/4] Test the edge that feeds reattachment, and stop it reporting ENOENT Reattachment.choose has twenty tests and every mutation aimed at it bites. Everything that computes its inputs had none, and five mutations there passed the whole 257-test suite. Four are closed here, each verified by applying the mutation and watching the new test go red. jobSnapshots() is the half that has to touch WorkManager and the filesystem, so it is where the untested values live. JobSnapshotsTest drives it against a real WorkManager and a real cacheDir: - A zero-byte staged file is not an output. Relaxing the filter to `exists()` -- which is what a job killed before its engine wrote anything leaves behind -- made the snapshot claim a result, and the user would meet a Save button for a zero-byte "conversion". The same case pins that the path is still reported and that the mtime stays 0 for a file that is not a result. - Each result carries its own file's mtime. Hardcoding it to zero starves the newest-file tie-break of the only data it has, which is precisely the failure the tie-break exists to prevent: the query has no ORDER BY, so an arbitrary winner keeps winning every launch. Timestamps are set with setLastModified and compared against what the filesystem stored, because mtime granularity is not this test's claim to make. ReattachGuardsTest covers the two decisions the ViewModel makes that the pure rule cannot: - A file picked while the query was still in flight is not reattached over. Deleting the guard turns the user's pick into yesterday's job -- with the Save button pointing at a file the card does not name. Made deterministic by holding WorkManager's task executor rather than by racing two IO hops: the query cannot finish until the pick has landed. The test also asserts the brake really gripped, so a reattachment that never arrived cannot pass for one that was refused. - An Ambiguous result is offered without being attributed. Two finished jobs naming one staged file is what the device produced before staging was keyed on the job id; taking the first job's tags labels the file with the other conversion's name, which is the confident lie the KDoc rejects. The neutral label and the absent size are both pinned. ReattachmentTest's FAILED exclusion was only ever tested with pathless FAILED jobs, so a narrow regression ranking a FAILED job that carries a file like a result passed all 257 tests. The live shape is the 2 MB orphan the device pass found: a job killed mid-write leaves a partial, and under that regression the user is offered a truncated file with a Save button. One fixture with outputPath and outputExists set closes it. save() re-checks the staged file, in both ViewModels. The check reattachment made ran inside a tag query that can be hours older than the tap, and cacheDir is what the OS empties when it wants space and what the sweep collects after a day. The file's absence used to arrive as staged.inputStream() throwing, and e.message put "/data/user/0/.../4b4882....mp4: open failed: ENOENT" on screen -- a true statement about a path the user has never seen and cannot act on. It now reads as a sentence with an action in it. The message is one constant next to OutputPublisher because both ViewModels need it and staging is what it is about. Reattachment's KDoc claimed a defect that was fixed in the commit before it -- that "Start over" keeps its staged file -- which would send a maintainer to re-fix D2. Rewritten to say what is actually true: the delete happens, and the gap it leaves is the reset() whose delete is cancelled with the Activity, which is the sweep's job and is named in the sweep's own KDoc. R1 / #10, R2 / #11, R24 / #33, R25 / #34 Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/ConversionViewModel.kt | 13 ++ .../convert/OutputPublisher.kt | 17 ++ .../libremediaconverter/join/JoinViewModel.kt | 13 ++ .../libremediaconverter/work/Reattachment.kt | 11 +- .../convert/MissingStagedFileTest.kt | 107 +++++++++ .../convert/ReattachGuardsTest.kt | 214 ++++++++++++++++++ .../work/JobSnapshotsTest.kt | 183 +++++++++++++++ .../work/ReattachmentTest.kt | 10 + 8 files changed, 564 insertions(+), 4 deletions(-) create mode 100644 app/src/test/java/org/libremediaconverter/convert/MissingStagedFileTest.kt create mode 100644 app/src/test/java/org/libremediaconverter/convert/ReattachGuardsTest.kt create mode 100644 app/src/test/java/org/libremediaconverter/work/JobSnapshotsTest.kt 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/OutputPublisher.kt b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt index ebb8355..504510b 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. * 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/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/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/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/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/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) From a6cf4f4ff48cdff17f5295b6921b6b5c32bd3cd9 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 22 Aug 2026 23:11:03 -0500 Subject: [PATCH 2/4] Stop three enum reads escaping doWork, and pin what the attempt bound buys Both workers read enums out of their input Data with Enum.valueOf, and all three reads sit ABOVE the try. A name this build does not define -- which is what a downgrade or a rollback with work still in the queue produces, since WorkManager keeps work for about a week -- threw IllegalArgumentException straight out of doWork(). That is D13's signature verbatim: FAILURE with reschedule = false, output Data with zero entries so the screen says "Conversion failed." and nothing else, and no staged.delete(), so the partial stays in cache. readSpec() twelve lines below already handles exactly this case, and its KDoc says why. So all three take readSpec's shape: entries.firstOrNull { it.name == name } ?: default. Consistency argues for it as much as correctness does -- the file already contains the right answer to this question, three times. WorkerEnumFallbackTest reaches each read. Two of them pin the value that replaces the unknown name rather than only that nothing threw: a quality tier falling back to something arbitrary would convert at a setting nobody chose, and a join's format decides the extension its output is staged with, which is where FFmpeg infers the container from. The engine-preference case asserts through the space check instead, because predicting which engine AUTO picks would tie the test to a routing rule it is not about. Against the unfixed code all three fail with "No enum constant ...". MAX_FOREGROUND_START_ATTEMPTS had no test of its value. Both existing cases are written against the symbol, which pins the relationship and leaves the number free: changed to 2, the job gives up about ninety seconds after process death -- exactly the long conversion the retry exists to protect -- and all 257 tests stayed green. The new assertion is the property the KDoc argues, not the literal: summed against WorkRequest's own DEFAULT_BACKOFF_DELAY_MILLIS and MAX_BACKOFF_MILLIS, the attempts have to span at least eight hours, which is what makes "the user will have opened the app by then" a claim rather than a hope. A deliberate re-tune that keeps the property passes; the accident does not, and reports the span it got (0.025 hours at 2). R22 / #31, R10 / #19 Co-Authored-By: Claude Opus 5 (1M context) --- .../libremediaconverter/work/ConcatWorker.kt | 9 +- .../work/ConversionWorker.kt | 18 +- .../work/FailureOutcomeTest.kt | 42 ++++ .../work/WorkerEnumFallbackTest.kt | 194 ++++++++++++++++++ 4 files changed, 254 insertions(+), 9 deletions(-) create mode 100644 app/src/test/java/org/libremediaconverter/work/WorkerEnumFallbackTest.kt 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/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/WorkerEnumFallbackTest.kt b/app/src/test/java/org/libremediaconverter/work/WorkerEnumFallbackTest.kt new file mode 100644 index 0000000..9b3ff3a --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/WorkerEnumFallbackTest.kt @@ -0,0 +1,194 @@ +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.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.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 +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.refuse = 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") + } +} + +/** + * A real [OutputPublisher] that records the staging names it is asked for, and can refuse on space. + * + * The name is the only place a join's format is legible from outside: the engine that would use it + * is native, and the worker deletes the staged file on its way out of a failed attempt. + */ +private class NamingPublisher(context: Context) : OutputPublisher(context) { + + val requestedNames = mutableListOf() + var refuse = false + + override fun hasSpaceFor(bytes: Long): Boolean = !refuse + + override fun createStagingFile(name: String): File { + requestedNames += name + return super.createStagingFile(name) + } +} + +/** 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 + } +} From 3534c6d9960e276cea3169319beae485a225ad96 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 22 Aug 2026 23:15:19 -0500 Subject: [PATCH 3/4] Clean up the empty document a refused open leaves, and stop the space sums wrapping publish() opened the destination stream outside its guarded region, justified by "nothing has been written at that point, so there is nothing of ours to remove". That reasoning is wrong about what exists: SAF's CreateDocument contract creates the document before publish() is ever called -- which is why every fixture in OutputPublisherPublishTest starts as an existing empty file. A provider that then hands out no stream, because it dropped between the picker and the write or simply returns null, left a zero-byte file at the name the user chose while the screen said the save had failed. The open moves inside the try, so the same two bounds that already govern a failed copy govern this: only a document URI, and only a destination positively known to be empty. The dead-provider case is untouched and now demonstrably by the guard rather than by the placement -- nothing answers for that authority, so no size can be read, and "I could not tell" still refuses to authorise a delete. Its test comment said the old thing and now says that one. The space arithmetic overflows in two places, both live on main and independent of the allocatable-versus-usable question that stays parked: - hasSpaceFor computed `free > required + headroom`. A request within 128 MiB of Long.MAX_VALUE wraps that sum negative, and every free-space measurement beats a negative number, so the check answers "plenty of room" to the largest request it can be handed. Rewritten as `free - headroom > required` with both operands clamped at zero, which is the form the parked branch's StagingSpace.hasRoomFor already argues for. - InputQuery.total folded a join's inputs with nothing stopping the sum from wrapping, and that is the reachable half: no single file overflows, three four-exabyte inputs do. It saturates at Long.MAX_VALUE now, which the check above then refuses. SpaceArithmeticTest ties the two together in the shape the defect had -- the total that came out negative is handed straight to the space check -- and keeps one allowed case so the refusals cannot pass by refusing everything. The negative-size clamp is deliberately left unasserted, with a comment saying why: it only changes the answer when free space is below the headroom, which a test reading the host's real cache volume cannot arrange. R6 / #15, R23 / #32 Co-Authored-By: Claude Opus 5 (1M context) --- .../libremediaconverter/convert/InputQuery.kt | 11 ++- .../convert/OutputPublisher.kt | 28 +++++-- .../convert/OutputPublisherPublishTest.kt | 28 ++++++- .../convert/SpaceArithmeticTest.kt | 79 +++++++++++++++++++ 4 files changed, 138 insertions(+), 8 deletions(-) create mode 100644 app/src/test/java/org/libremediaconverter/convert/SpaceArithmeticTest.kt 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 504510b..430b135 100644 --- a/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt +++ b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt @@ -67,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]. @@ -128,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/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/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 + } +} From d3975617a81b68c4c4a59598af2502732e62fbea Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 22 Aug 2026 23:22:35 -0500 Subject: [PATCH 4/4] Put a gate on the three pieces of wiring that had none Three separate mutations passed the full 257-test suite, all for the same reason: the tool was tested and the thing that calls it was not. The join half of per-job staging. Reverting ConcatWorker to the constant the audit's own D8 table names -- "joined.", one string for every join of a format -- left everything green: PerJobStagingTest drives only the conversion worker, and StagingNamesTest pins only the pure function. The new case drives two real ConcatWorkers with different ids and reads what they asked for rather than what is on disk, because ConcatEngine is native, so neither join gets past it here and the catch on the way out deletes what it staged. The recorder moves into WorkerStubs, which is what that file is for, and the enum test that already had a private copy now uses it. The process-start sweep. Deleting the one line in LibreMediaConverterApp.onCreate() -- the only reason that class exists, and the backstop for every leak discardStaged cannot reach -- left everything green too. The test stages one file a day old and one written now, calls onCreate() again, and asserts both halves: the abandoned one is collected and the live one is not. The second half is what says this is a sweep rather than the clearStaging() it replaced, which could take a file out from under a running job. The mtime is set explicitly, because "written long enough ago" is not something a test can wait for when the period is twenty-four hours. Casting the Robolectric application to LibreMediaConverterApp is an assertion in itself: it fails if android:name ever stops pointing here, in which case the swept line would be correct code that never runs. The backup and device-transfer exclusions. Reverting data_extraction_rules.xml to the template's boilerplate left the unit tests green AND lintDebug green -- it is a resource, so nothing was reading it -- and the failure it causes is one nobody meets in development. WorkManager's queue is the app's whole backup payload, and its rows name content:// grants and cacheDir paths that do not survive a transfer; reattachment queries by tag on launch, so a fresh install would come up attached to a job the user never ran on it. The test reads the compiled resource table, so what it pins is what the APK carries, and it asserts domain and path for all four entries in both sections -- an with no path is skipped unchecked by lint's own detector, so half an entry could protect nothing. Its KDoc records the one thing it does not cover: the manifest attribute that points the system at the file. R8 / #17, R9 / #18, R11 / #20 Co-Authored-By: Claude Opus 5 (1M context) --- .../libremediaconverter/AppStartSweepTest.kt | 98 ++++++++++++++++ .../BackupExclusionsTest.kt | 106 ++++++++++++++++++ .../work/PerJobStagingTest.kt | 41 ++++++- .../work/WorkerEnumFallbackTest.kt | 23 +--- .../libremediaconverter/work/WorkerStubs.kt | 25 +++++ 5 files changed, 268 insertions(+), 25 deletions(-) create mode 100644 app/src/test/java/org/libremediaconverter/AppStartSweepTest.kt create mode 100644 app/src/test/java/org/libremediaconverter/BackupExclusionsTest.kt 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/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/WorkerEnumFallbackTest.kt b/app/src/test/java/org/libremediaconverter/work/WorkerEnumFallbackTest.kt index 9b3ff3a..32cea02 100644 --- a/app/src/test/java/org/libremediaconverter/work/WorkerEnumFallbackTest.kt +++ b/app/src/test/java/org/libremediaconverter/work/WorkerEnumFallbackTest.kt @@ -1,7 +1,6 @@ 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 @@ -15,7 +14,6 @@ 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.StagingNames import org.libremediaconverter.convert.installTestWorkManager @@ -85,7 +83,7 @@ class WorkerEnumFallbackTest { 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.refuse = true + publisher.refuseSpace = true val result = runBlocking { conversionWorker(preference = "FORCE_QUANTUM").doWork() } @@ -153,25 +151,6 @@ class WorkerEnumFallbackTest { } } -/** - * A real [OutputPublisher] that records the staging names it is asked for, and can refuse on space. - * - * The name is the only place a join's format is legible from outside: the engine that would use it - * is native, and the worker deletes the staged file on its way out of a failed attempt. - */ -private class NamingPublisher(context: Context) : OutputPublisher(context) { - - val requestedNames = mutableListOf() - var refuse = false - - override fun hasSpaceFor(bytes: Long): Boolean = !refuse - - override fun createStagingFile(name: String): File { - requestedNames += name - return super.createStagingFile(name) - } -} - /** An engine that writes the output and remembers what it was asked to produce. */ private class RequestRecordingTranscoder : SoftwareTranscoder { 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. *