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. 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() +} 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)