From 92bcff8656b2ba78f92c7136e6e1b097b674b1c3 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 2 Sep 2026 18:02:39 -0500 Subject: [PATCH 1/3] Make a failure that says nothing still say something (#193) Three sites, all ci == 0 before this, and all the same rule: work/ConversionWorker.kt:316 cause.message ?: GENERIC_FAILURE_MESSAGE convert/ConversionViewModel.kt:631 e.message ?: SAVE_FAILED_MESSAGE join/JoinViewModel.kt:416 e.message ?: SAVE_FAILED_MESSAGE Every existing test throws WITH a message, so the right-hand side had never been evaluated anywhere in the suite. A Throwable carrying none is not exotic: RuntimeException(), IOException() and most platform exceptions raised without an argument all have a null message, and a native engine that dies is exactly where one comes from. The worker case needed care, and the care is the reason it survived three waves. ConversionStateMappingTest's "a failure with nothing said still says something" looks like it covers that site and does not -- it drives the READ side, map(FAILED, Data.EMPTY), and that side has a fallback of its own at ConversionViewModel.kt:147-149 which turns a blank KEY_ERROR back into the same constant. So a test asserting on the resulting Failed state stays green while the worker's fallback is broken. Measured rather than reasoned: with :316 mutated to .orEmpty(), exactly ONE of 587 tests went red, and it was the new one. Everything else, including the test that appears to cover it, stayed green. So the worker case reads KEY_ERROR off the worker's own Result, before anything downstream can repair it. The two save cases have no such second line -- both write _state.value directly -- so the state is the right thing to assert there, and both also assert that `pending` still travels: a fallback that dropped the handle would leave the file unreachable from the very screen that just said the save failed. Held in one class against the ticket's suggestion of three. They are one rule at three layers, and the masking above has to be explained once rather than three times. FailedSaveRetryTest already sets the precedent for both ViewModels in one file; this adds one worker to that shape. FORCE_SOFTWARE in the worker fixture so the failure comes straight out of runFFmpeg. AUTO would enter runMedia3OrFallBack, whose catch runs the job a second time in software: the same exception arrives, but by a path this is not about and which HardwareFallbackTest owns. Mutations, all run and restored -- each site to .orEmpty(), never to a different constant, which would only prove the test reads a constant: ConversionWorker:316 1 of 587 red (this file) ConversionViewModel:631 red JoinViewModel:416 red Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/MessagelessFailureTest.kt | 213 ++++++++++++++++++ 1 file changed, 213 insertions(+) create mode 100644 app/src/test/java/org/libremediaconverter/convert/MessagelessFailureTest.kt diff --git a/app/src/test/java/org/libremediaconverter/convert/MessagelessFailureTest.kt b/app/src/test/java/org/libremediaconverter/convert/MessagelessFailureTest.kt new file mode 100644 index 0000000..83ea383 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/MessagelessFailureTest.kt @@ -0,0 +1,213 @@ +package org.libremediaconverter.convert + +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.Dispatchers +import kotlinx.coroutines.runBlocking +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +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.ConversionRequest +import org.libremediaconverter.model.EnginePreference +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.model.OutputFormat +import org.libremediaconverter.work.ConcatWorker +import org.libremediaconverter.work.ConversionWorker +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File +import java.util.UUID + +/** + * A failure that says nothing still has to say something. + * + * Three sites, all `ci == 0` before this file, and all the same rule: + * + * ``` + * work/ConversionWorker.kt:316 cause.message ?: GENERIC_FAILURE_MESSAGE + * convert/ConversionViewModel.kt:631 e.message ?: SAVE_FAILED_MESSAGE + * join/JoinViewModel.kt:416 e.message ?: SAVE_FAILED_MESSAGE + * ``` + * + * Every existing test throws *with* a message, so the right-hand side had never been evaluated + * anywhere in the suite. A `Throwable` carrying none is not exotic — `RuntimeException()`, + * `IOException()` and most platform exceptions raised without an argument all have a null message. + * + * ## Held in one class, against the ticket's suggestion + * + * #193 proposed putting each case beside the behaviour it neighbours. They are together instead, + * because they are one rule at three layers and because the trap below has to be explained once + * rather than three times. `FailedSaveRetryTest` sets the precedent for both ViewModels in one + * file; this extends it by one worker. + * + * ## The trap, which is why the worker case asserts what it does + * + * `ConversionStateMappingTest`'s *"a failure with nothing said still says something"* looks like it + * already covers the worker site. It does not: it drives the **read** side, `map(FAILED, Data.EMPTY)`, + * and that side has a fallback of its own (`ConversionViewModel.kt:147-149`): + * + * ```kotlin + * update.outputData.getString(ConversionWorker.KEY_ERROR) + * ?.takeIf { it.isNotBlank() } + * ?: ConversionWorker.GENERIC_FAILURE_MESSAGE + * ``` + * + * So mutating the worker's fallback to `.orEmpty()` writes `KEY_ERROR to ""`, and the ViewModel + * turns that straight back into the same constant. **A test asserting on the resulting `Failed` + * state stays green under the mutation**, which is most likely why the write-side fallback survived + * three waves of test work. The worker case therefore reads `KEY_ERROR` off the worker's own + * `Result`, before anything downstream can repair it. + * + * The two save cases have no such second line: both write `_state.value` directly, so the state is + * the right thing to assert there. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class MessagelessFailureTest { + + private lateinit var app: Application + private lateinit var publisher: RecordingPublisher + private lateinit var staged: File + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = RecordingPublisher(app) + ConversionDependencies.publisher = { publisher } + ConversionDependencies.probe = { _, _ -> InputProbe() } + staged = publisher.createStagingFile("holiday.mp4").apply { writeBytes(ByteArray(4096)) } + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + /** + * The engine gives up without saying why, which is what a native crash looks like from here. + * + * Asserted on the worker's own output `Data` rather than on a screen — see the class KDoc. + */ + @Test + fun `a conversion that fails without a message still reports one`() { + installTestWorkManager(app, Data.EMPTY) + ConversionDependencies.software = { MessagelessTranscoder } + + val result = runBlocking { failingWorker().doWork() } + + assertTrue("the job must fail rather than retry, got $result", result is ListenableWorker.Result.Failure) + assertEquals( + "a failure with no message must still put something on screen", + ConversionWorker.GENERIC_FAILURE_MESSAGE, + (result as ListenableWorker.Result.Failure).outputData.getString(ConversionWorker.KEY_ERROR), + ) + } + + @Test + fun `a save that fails without a message still reports one`() { + installTestWorkManager(app, conversionOutput()) + val viewModel = ConversionViewModel(app, Dispatchers.Unconfined) + viewModel.onInputPicked(Uri.parse("content://test/holiday.mkv")) + awaitState(viewModel.state, "Ready") { it is ConversionState.Ready } + viewModel.convert() + awaitState(viewModel.state, "Converted") { it is ConversionState.Converted } + + publisher.publishFailure = RuntimeException() + viewModel.save(DESTINATION) + + val failed = awaitState(viewModel.state, "Failed") { it is ConversionState.Failed } as ConversionState.Failed + assertEquals(SAVE_FAILED_MESSAGE, failed.message) + // The handle travels even on the wordless path. Without this, a fallback that also dropped + // `pending` would pass -- and the file would be unreachable from the screen that just said + // the save failed. + assertNotNull("a wordless failure must still offer the file again", failed.retry) + } + + @Test + fun `a join save that fails without a message still reports one`() { + installTestWorkManager(app, joinOutput()) + 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 } + + publisher.publishFailure = RuntimeException() + viewModel.save(DESTINATION) + + val failed = awaitState(viewModel.state, "Failed") { it is JoinState.Failed } as JoinState.Failed + assertEquals(SAVE_FAILED_MESSAGE, failed.message) + assertNotNull("a wordless failure must still offer the file again", failed.retry) + } + + /** + * `FORCE_SOFTWARE` so the failure comes straight out of `runFFmpeg`. + * + * `AUTO` would enter `runMedia3OrFallBack`, whose catch runs the job a second time in software + * — the same exception would arrive, but through a path this test is not about and which + * `HardwareFallbackTest` already owns. + */ + private fun failingWorker(): ConversionWorker { + val spec = OutputFormat.MP4_H265.spec + return TestListenableWorkerBuilder( + context = app, + inputData = workDataOf( + ConversionWorker.KEY_INPUT_URI to "file:///tmp/holiday.mp4", + ConversionWorker.KEY_DISPLAY_NAME to "holiday.mp4", + ConversionWorker.KEY_CONTAINER to spec.container.name, + ConversionWorker.KEY_VIDEO_CODEC to spec.videoCodec.name, + ConversionWorker.KEY_AUDIO_CODEC to spec.audioCodec.name, + ConversionWorker.KEY_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name, + ), + runAttemptCount = 0, + ).setId(JOB_ID).build() + } + + private fun conversionOutput() = workDataOf( + ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath, + ConversionWorker.KEY_SUGGESTED_NAME to SUGGESTED_NAME, + ConversionWorker.KEY_MIME_TYPE to JOB_MIME_TYPE, + ) + + private fun joinOutput() = workDataOf( + ConcatWorker.KEY_OUTPUT_PATH to staged.absolutePath, + ConcatWorker.KEY_SUGGESTED_NAME to SUGGESTED_NAME, + ConcatWorker.KEY_MIME_TYPE to JOB_MIME_TYPE, + ) + + private companion object { + val DESTINATION: Uri = Uri.parse("content://test/destination.mp4") + val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-00000000019a") + const val SUGGESTED_NAME = "holiday.mp4" + const val JOB_MIME_TYPE = "video/mp4" + } +} + +/** + * An engine that gives up without saying why. + * + * `RuntimeException()` rather than a subclass with a blank message: `Throwable.message` is *null* + * here, which is the case the elvis exists for. A blank-but-present message takes the left-hand + * side and is a different path — `ConversionStateMappingTest` covers that one, on the read side. + */ +@UnstableApi +private object MessagelessTranscoder : SoftwareTranscoder { + override suspend fun run( + request: ConversionRequest, + inputPath: String, + output: File, + durationMs: Long, + onProgress: (Int) -> Unit, + ): Unit = throw RuntimeException() +} -- 2.47.3 From cc215195ee2f4e9eed8b4773e86c03e3c942b321 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 2 Sep 2026 18:05:11 -0500 Subject: [PATCH 2/3] Build a command for the audio the user turned off (#198) audioArgs' Drop arm -- `AudioPlan.Drop -> listOf("-an")` -- was ci == 0. The suite's only -an assertion lives in "gif generates a palette to avoid banding and drops audio", and that one comes from the image path at FFmpegCommandBuilder.kt:79/:90, which emits -an directly and never reaches audioArgs. Two sites, one string, one tested. It is a live path rather than defensive code. AdvancedPicker renders all of AudioCodec.entries including NONE, ContainerCapabilities.validate permits audio-off whenever the input has video, and MKV routes the job to FFmpeg -- so "convert this and drop the soundtrack" is something a user can do today and nothing had built the command for. Both halves are asserted, and the second is not padding: -an alone still passes if the arm falls through to the else and emits an AAC encoder beside the flag, which is a file that is silent because the flag won while carrying an encoder nobody asked for. Three mutations, all run and restored. The third is the one that justifies the second assertion, since the first two break -an as a side effect and so cannot show it: Drop -> emptyList() red Drop -> the else arm's aac encoder red (loses -an as well) Drop -> listOf("-an", "-c:a", "aac") red -- -an intact, caught by assertFalse Co-Authored-By: Claude Opus 5 (1M context) --- .../ffmpeg/FFmpegCommandBuilderTest.kt | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/app/src/test/java/org/libremediaconverter/ffmpeg/FFmpegCommandBuilderTest.kt b/app/src/test/java/org/libremediaconverter/ffmpeg/FFmpegCommandBuilderTest.kt index 9f9624d..1274d11 100644 --- a/app/src/test/java/org/libremediaconverter/ffmpeg/FFmpegCommandBuilderTest.kt +++ b/app/src/test/java/org/libremediaconverter/ffmpeg/FFmpegCommandBuilderTest.kt @@ -190,6 +190,30 @@ class FFmpegCommandBuilderTest { assertPair(cmd(OutputFormat.M4A_AAC), "-b:a", "192k") } + /** + * Turning audio off, which the Advanced picker offers and nothing had ever built a command for. + * + * `audioArgs`' `Drop` arm was `ci == 0`. The suite's only `-an` assertion is in + * `gif generates a palette to avoid banding and drops audio`, and that one comes from the image + * path (`FFmpegCommandBuilder.kt:79`/`:90`), which emits `-an` directly and never reaches + * `audioArgs`. Two sites, one string, one tested. + * + * It is a live path rather than defensive code: `AdvancedPicker` renders all of + * `AudioCodec.entries` including `NONE`, `ContainerCapabilities.validate` permits audio-off + * whenever the input has video, and MKV routes the job to FFmpeg. + * + * Both halves are asserted. `-an` alone would still pass if the arm fell through to the `else` + * and emitted an AAC encoder beside it -- a file that is silent because the flag won, carrying + * an encoder nobody asked for. + */ + @Test + fun `turning audio off drops the track instead of encoding one`() { + val args = cmd(OutputSpec(Container.MKV, VideoCodec.H264, AudioCodec.NONE)) + + assertTrue("audio turned off must emit -an, got $args", args.contains("-an")) + assertFalse("a dropped track must not also carry an encoder, got $args", args.contains("-c:a")) + } + @Test fun `audio only formats never carry a video encoder`() { listOf(OutputFormat.MP3, OutputFormat.FLAC, OutputFormat.WAV, OutputFormat.OPUS) -- 2.47.3 From da8d53851bd45c2b9086aaa497b35718836d300c Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 2 Sep 2026 18:07:43 -0500 Subject: [PATCH 3/3] Render the container row for a video nothing could name (#199) ConverterScreen.kt:668's null arm -- DetailRow("Container", probe.container?.label ?: "Unknown") in the VIDEO branch -- had never rendered. Every video case in FileCardTest uses VIDEO_PROBE, which carries container = MP4. The argument for adding it is the asymmetry, not the coverage. FileCard renders that exact expression twice, once in AUDIO_ONLY (:660) and once in VIDEO (:668), and "an audio-only file nothing else could describe degrades one row at a time" drives only the first. Same expression, same fallback, one kind covered and one not -- which is the same argument CLAUDE.md records for including ContainerCapabilities:94. Nor is null an edge case here. InputProbe.container's own KDoc says MediaExtractor cannot report a container at all, so it comes from FFprobe alone: any run where FFprobe did not answer produces exactly this shape -- real codec, real dimensions, real duration, no container. An empty value in its place would read as a rendering bug rather than as a probe that got half its sources. The other three rows are asserted alongside, which is what keeps this from being a copy of the audio-only case. There, everything is unknown at once; here one field is missing from a probe that is otherwise complete, and the rest have to be unaffected by it. Mutation: `?: "Unknown"` -> `?: ""` at :668 only, run and restored. The AUDIO_ONLY twin at :660 is a separate expression, and mutating that one would redden the existing test instead -- which would prove nothing about this one. Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/FileCardTest.kt | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/app/src/test/java/org/libremediaconverter/convert/FileCardTest.kt b/app/src/test/java/org/libremediaconverter/convert/FileCardTest.kt index a6e8112..7826daa 100644 --- a/app/src/test/java/org/libremediaconverter/convert/FileCardTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/FileCardTest.kt @@ -205,6 +205,34 @@ class FileCardTest { assertNoRow("Length") } + /** + * A video the app knows a great deal about and cannot name the container of. + * + * Not an edge case. `InputProbe.container`'s own KDoc says `MediaExtractor` cannot report a + * container at all -- it comes from FFprobe -- so any run where FFprobe did not answer produces + * exactly this: real codec, real dimensions, real duration, `container = null`. + * + * **The twin was already tested and this one was not**, which is the argument for adding it. + * `FileCard` renders `probe.container?.label ?: "Unknown"` twice, once in the `AUDIO_ONLY` + * branch (`ConverterScreen.kt:660`) and once in the `VIDEO` branch (`:668`), and + * `an audio-only file nothing else could describe degrades one row at a time` drives only the + * first. Same expression, same fallback, one kind covered. That asymmetry is the same one + * `CLAUDE.md` records for including `ContainerCapabilities:94`. + * + * The other rows are asserted alongside so this is not a copy of the audio-only case: there, + * everything is unknown at once; here, one field is missing from a probe that is otherwise + * complete, and the rest must be unaffected by it. + */ + @Test + fun `a video file whose container nothing identified says so and keeps its other rows`() { + setFileCard(input(probe = VIDEO_PROBE.copy(container = null))) + + assertRow("Container", "Unknown") + assertRow("Video", "${VideoCodec.H264.label} · 1920×1080") + assertRow("Audio", AudioCodec.AAC.label) + assertRow("Length", "1:30") + } + /** * The row is one node, not a label node beside a value node. A test matching on `"Container"` * alone would pass against either shape. -- 2.47.3