diff --git a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt index de587e3..c2542b2 100644 --- a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt +++ b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt @@ -120,6 +120,29 @@ class ConversionViewModel @JvmOverloads constructor( * the first screen. */ private val cleanupDispatcher: CoroutineDispatcher = Dispatchers.IO, + /** + * Where the two blocking hops behind a pick run — the metadata query and the probe. + * + * A seam for the probe above all, because that is the one call in this class that throws + * on purpose. [probeOrUnreadable] rethrows anything that is not a native load failure, and + * the `launch` it runs in has no exception handler by design: on a device the error reaches + * the thread's default handler and takes the process down, which is what an + * [OutOfMemoryError] should do. + * + * On the JVM there is no such handler. kotlinx-coroutines-test installs a process-wide + * collector, once and for the life of the classloader, that keeps an escaped error and + * hands it to whichever `runTest` starts next — so it failed a Compose test class that had + * nothing to do with it, and *which* class moved between runs of identical code. Naming the + * dispatcher is what lets a test keep the throw inside its own window, where it fails the + * test that caused it and is consumed rather than collected. + * + * Both hops rather than the probe alone, which is where this differs from the seam issue #66 + * proposed: leaving the metadata query on a real [Dispatchers.IO] makes the coroutine resume + * on a main looper that Robolectric leaves paused, and that bounce is precisely the + * asynchrony that made delivery unpredictable. One dispatcher covers a whole pick, and + * leaves nothing about it to timing. + */ + private val pickDispatcher: CoroutineDispatcher = Dispatchers.IO, ) : AndroidViewModel(app) { private val workManager = WorkManager.getInstance(app) @@ -241,13 +264,13 @@ class ConversionViewModel @JvmOverloads constructor( viewModelScope.launch { // Both the metadata query and the probe touch disk, and the probe spawns FFprobe. // Neither belongs on the main thread. - val file = withContext(Dispatchers.IO) { InputQuery.describe(getApplication(), uri) } + val file = withContext(pickDispatcher) { InputQuery.describe(getApplication(), uri) } // Show the file as soon as its name and size are known. Probing now runs FFprobe on // every pick, which is a native process spawn, and making the whole screen wait on it // would read as the app having ignored the tap. _state.value = ConversionState.Ready(file) - val probe = withContext(Dispatchers.IO) { probeOrUnreadable(uri) } + val probe = withContext(pickDispatcher) { probeOrUnreadable(uri) } // Only fill in the probe if the user has not moved on in the meantime. _state.update { current -> if (current is ConversionState.Ready && current.input.uri == uri) { diff --git a/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelProbeFailureTest.kt b/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelProbeFailureTest.kt index dfb5079..9bfe5b9 100644 --- a/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelProbeFailureTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelProbeFailureTest.kt @@ -2,14 +2,15 @@ package org.libremediaconverter.convert import android.app.Application import android.net.Uri -import android.os.Looper import androidx.media3.common.util.UnstableApi import androidx.work.workDataOf import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.test.runTest import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull +import org.junit.Assert.assertThrows import org.junit.Before import org.junit.Test import org.junit.runner.RunWith @@ -18,8 +19,6 @@ 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.util.concurrent.TimeUnit /** * That a probe which throws leaves a screen the user can act on, not a dead coroutine. @@ -87,19 +86,35 @@ class ConversionViewModelProbeFailureTest { * is out of memory" into "this video looks unreadable" and let the app carry on in a * state it cannot honour — which is the regression a blanket `catch (Throwable)` would * have introduced, and the reason this defect was left open rather than fixed carelessly. + * + * **The error itself is what is asserted here, and that is what the `pickDispatcher` seam + * bought.** With the hop hard-coded to `Dispatchers.IO` this was impossible: the throw + * happened on a pool thread some time after this method had returned, so all a test could do + * was infer it from a card that never filled in — which is also what a probe returning null + * would look like. Worse, the escaped error went into kotlinx-coroutines-test's process-wide + * collector and was rethrown at whichever `runTest` started next, which is a *different* + * Compose class between runs of identical code. Putting the pick on [Dispatchers.Unconfined] + * runs it inline, inside a `runTest` whose scope owns the collector's callback: the error is + * handed to this test and consumed, rather than stored for a stranger. + * + * Note where it surfaces — at the end of `runTest`, not inside `onInputPicked`. `launch` + * gives an escaped error to the handler chain and never to its caller, so nothing can catch + * it at the call itself. This is as close as the coroutine machinery allows, and unlike the + * old assertion it is the real [OutOfMemoryError] instance. */ @Test fun `an OutOfMemoryError is not swallowed`() { ConversionDependencies.probe = { _, _ -> throw OutOfMemoryError("Failed to allocate 512 MB") } + // Unconfined for the pick, so the whole of onInputPicked runs inline on this thread and + // has thrown before runTest can leave the scope that has to receive the error. + val viewModel = ConversionViewModel(app, Dispatchers.Unconfined, Dispatchers.Unconfined) - val viewModel = ConversionViewModel(app, Dispatchers.Unconfined) - viewModel.onInputPicked(INPUT) + val escaped = assertThrows(OutOfMemoryError::class.java) { runTest { viewModel.onInputPicked(INPUT) } } - // The observable difference, and the reason this is asserted on state rather than on a - // caught throwable: the probe hop is on Dispatchers.IO, so an error that escapes lands - // on that thread's handler rather than at this call. What must not happen is the card - // filling in with an "unreadable" verdict the app would then act on. - val settled = settle(viewModel) + assertEquals("Failed to allocate 512 MB", escaped.message) + // The other half of the contract, unchanged: an OOM is about the process, so the card is + // left as it was rather than filled in with a verdict the app would then act on. + val settled = viewModel.state.value // `sizeBytes = null`, not `0L`: no provider is registered for this authority, so the // metadata query returns nothing and the descriptor cannot be opened either. That is the // unknown, and it stopped being spelled the same way as "empty" -- see [InputQuery]. @@ -132,23 +147,7 @@ class ConversionViewModelProbeFailureTest { return (ready as ConversionState.Ready).input.probe } - /** - * Pumps the looper the way [awaitState] does, but for a fixed span and without requiring - * anything to happen — here "the pick never came back" is the expected outcome, so there - * is no predicate to wait on. - */ - private fun settle(viewModel: ConversionViewModel): ConversionState { - val deadline = System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(SETTLE_MS) - while (System.nanoTime() < deadline) { - shadowOf(Looper.getMainLooper()).idle() - Thread.sleep(POLL_MS) - } - return viewModel.state.value - } - private companion object { val INPUT: Uri = Uri.parse("content://test/holiday.mp4") - const val SETTLE_MS = 500L - const val POLL_MS = 5L } }