From 794cef7b34eeb03d1760b9230f4cb80b6f0ac5bc Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 1 Sep 2026 21:59:00 -0500 Subject: [PATCH 1/2] C2 (#177): cut MediaProbe's two-probe merge into a seam, and ask which probe wins probe() runs MediaExtractor and FFprobe independently and merges the two, and every rule in that merge is a decision nothing held. The reason is structural rather than an oversight: RemuxTest drives the whole thing on a device against committed fixtures, but only ever with one probe answering and the other agreeing or also failing. Nothing on any source set can arrange for a real extractor and a real FFprobe to *disagree*, so every elvis in the merge was taken in one direction and never the other. The seam is `internal fun merge(Extracted?, FFprobeInfo?): InputProbe`, pulled out of probe() whole -- probe() now reads the two probes, merges, and keeps the log. FFprobeInfo becomes internal alongside it; Extracted already was, with a KDoc giving this exact reason, and FFprobeInfo simply never got the same treatment. Half a signature being private is what made the function unnameable from a test. Eleven tests, and the mutations that hold them: image beats a real video codec demote the isImage arm below the video arm the extractor wins on codecs flip the elvis to FFprobe-first duration is the larger reading replace maxOf with extractor-first dimensions prefer the extractor flip the width elvis no recognised stream is unreadable narrow the guard to `extracted == null && info == null` All five red, then restored. One mutation I tried first was *semantically equivalent* -- moving the image arm above the both-null arm changes nothing for any reachable input -- so it stayed green and is recorded here rather than counted: a green mutation is only evidence when the mutation is a real change. The last row is the arm the ticket was filed for: parsed, and carrying no stream either probe recognised, which is what a container holding only subtitles looks like. Its input was already being constructed elsewhere in the suite -- MediaProbeTrackWalkTest calls extractedFrom(emptyList()) and gets exactly it -- and had never been handed to the merge. 568 -> 579 JVM tests, 0 failures. MediaProbe: 35 -> 24 missed lines, 72 -> 40 missed branches. Line 2103/2348 -> 2173/2348; branch 1029/1340 -> 1091/1342. The branch denominator moved by two, and it is the seam that moved it -- worth stating separately from the numerator, because CLAUDE.md's coverage entry has a documented history of explaining its own numbers wrongly. Co-Authored-By: Claude Opus 5 (1M context) --- .../libremediaconverter/convert/MediaProbe.kt | 47 +++-- .../convert/MediaProbeMergeTest.kt | 166 ++++++++++++++++++ 2 files changed, 203 insertions(+), 10 deletions(-) create mode 100644 app/src/test/java/org/libremediaconverter/convert/MediaProbeMergeTest.kt diff --git a/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt b/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt index d878163..b226f95 100644 --- a/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt +++ b/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt @@ -50,19 +50,42 @@ object MediaProbe { ) fun probe(context: Context, uri: Uri): InputProbe { - val extracted = probeWithExtractor(context, uri) - val info = probeWithFFprobe(context, uri) + val merged = merge(probeWithExtractor(context, uri), probeWithFFprobe(context, uri)) + if (merged.kind == InputKind.UNPARSEABLE) { + // Not a failure: an unparseable input is a strong signal that this job belongs on + // FFmpeg. Reporting an unknown codec makes the router say so. + Log.i(TAG, "Neither MediaExtractor nor FFprobe could read $uri; routing to FFmpeg.") + } + return merged + } + /** + * What the two probes together say about one input. + * + * A pure function, and `internal` for the same reason [extractedFrom] is: the precedence rules + * below are the answer to "which probe wins", and until this was pulled out of [probe] the only + * way to ask was to have a real `MediaExtractor` and a real FFprobe **disagree**, which nothing + * on any source set can arrange. `RemuxTest` drives this on a device against committed + * fixtures, but only ever with one probe answering and the other agreeing or also failing -- + * so every elvis here was taken in one direction and never the other. + * + * The rules, each of which is a decision rather than an accident: + * + * - **The extractor wins on codecs.** It is the platform's own view of what it can decode, + * which is the thing the router is about to ask about. FFprobe's name for the same track can + * differ, and the copy planner keys off these strings. + * - **FFprobe alone reports the container.** `MediaExtractor` cannot, which is why [InputProbe] + * carries a nullable one and `CopyPlanner` treats null as "container unknown". + * - **Duration is the larger of the two**, not the first non-zero. Either probe can report zero + * for a file the other times correctly, and a zero duration makes the FFmpeg progress + * percentage undefined. + */ + internal fun merge(extracted: Extracted?, info: FFprobeInfo?): InputProbe { val videoCodec = extracted?.videoCodec ?: info?.videoCodec val audioCodec = extracted?.audioCodec ?: info?.audioCodec val kind = classify(extracted, info) - if (kind == InputKind.UNPARSEABLE) { - // Not a failure: an unparseable input is a strong signal that this job belongs on - // FFmpeg. Reporting an unknown codec makes the router say so. - Log.i(TAG, "Neither MediaExtractor nor FFprobe could read $uri; routing to FFmpeg.") - return UNREADABLE - } + if (kind == InputKind.UNPARSEABLE) return UNREADABLE return InputProbe( videoCodec = videoCodec, @@ -83,7 +106,7 @@ object MediaProbe { * audio file and a corrupt file indistinguishable. The source-info card cannot describe either * honestly until they are separate, and neither can the copy planner. */ - private fun classify(extracted: Extracted?, info: FFprobeInfo?): InputKind = when { + internal fun classify(extracted: Extracted?, info: FFprobeInfo?): InputKind = when { info?.isImage == true -> InputKind.IMAGE extracted == null && info == null -> InputKind.UNPARSEABLE (extracted?.videoCodec ?: info?.videoCodec) != null -> InputKind.VIDEO @@ -162,7 +185,11 @@ object MediaProbe { } } - private class FFprobeInfo( + /** + * `internal` rather than `private` for the same reason [Extracted] is, and it should have been + * from the start: [merge] cannot be named from a test while half its signature is private. + */ + internal class FFprobeInfo( val container: Container?, val videoCodec: String?, val audioCodec: String?, diff --git a/app/src/test/java/org/libremediaconverter/convert/MediaProbeMergeTest.kt b/app/src/test/java/org/libremediaconverter/convert/MediaProbeMergeTest.kt new file mode 100644 index 0000000..173959a --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/MediaProbeMergeTest.kt @@ -0,0 +1,166 @@ +package org.libremediaconverter.convert + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test +import org.libremediaconverter.model.Container +import org.libremediaconverter.model.InputKind +import org.libremediaconverter.model.InputProbe + +/** + * Which of the two probes wins, when they disagree. + * + * [MediaProbe.probe] runs `MediaExtractor` and FFprobe independently and then merges the two, and + * every rule in that merge is a decision. None of them had a test, for a reason that is structural + * rather than an oversight: `RemuxTest` drives the whole thing on a device against committed + * fixtures, but only ever with **one probe answering and the other agreeing or also failing**. + * Nothing on any source set can arrange for a real extractor and a real FFprobe to disagree, so + * every elvis in the merge was taken in one direction and never the other. + * + * Cutting `merge` out of `probe` is what makes the question askable. Both halves of its signature + * had to become `internal` for that -- `Extracted` already was, with a KDoc giving this exact + * reason; `FFprobeInfo` simply never got the same treatment. + */ +class MediaProbeMergeTest { + + /** + * The rule with the loudest failure mode, and `isImageFormat`'s own KDoc names it: a false + * positive here "makes the source-info card describe a video as an image". So the image verdict + * has to beat a real video codec from the extractor, and the ordering that makes it do so is + * the first arm of `classify` rather than anything a reader would infer from the fields. + */ + @Test + fun `an image verdict from FFprobe beats a video codec from the extractor`() { + val merged = MediaProbe.merge( + extracted = extracted(video = "h264"), + info = info(video = "mjpeg", isImage = true), + ) + + assertEquals(InputKind.IMAGE, merged.kind) + } + + @Test + fun `the extractor wins on codecs, because it is the view the router will act on`() { + val merged = MediaProbe.merge( + extracted = extracted(video = "h264", audio = "aac"), + info = info(video = "hevc", audio = "mp3"), + ) + + assertEquals("h264", merged.videoCodec) + assertEquals("aac", merged.audioCodec) + } + + @Test + fun `FFprobe answers for a file the extractor could not open`() { + val merged = MediaProbe.merge(extracted = null, info = info(video = "vp9", audio = "opus")) + + assertEquals("vp9", merged.videoCodec) + assertEquals("opus", merged.audioCodec) + assertEquals(InputKind.VIDEO, merged.kind) + } + + @Test + fun `the extractor answers for a file FFprobe could not read`() { + val merged = MediaProbe.merge(extracted = extracted(video = "h264", audio = "aac"), info = null) + + assertEquals("h264", merged.videoCodec) + assertEquals("aac", merged.audioCodec) + assertNull("only FFprobe can name the container, so it stays unknown here", merged.container) + } + + /** + * The larger of the two, not the first non-zero. + * + * Either probe can report zero for a file the other times correctly, and a zero duration makes + * the FFmpeg progress percentage undefined -- `FFmpegEngine` divides by it. Both orderings are + * asserted because "take the extractor's" and "take the larger" agree in one direction and not + * the other, and only one of them is the rule. + */ + @Test + fun `duration is the longer of the two readings, whichever probe supplied it`() { + assertEquals( + 5_000L, + MediaProbe.merge(extracted(duration = 0L), info(duration = 5_000L)).durationMs, + ) + assertEquals( + 5_000L, + MediaProbe.merge(extracted(duration = 5_000L), info(duration = 0L)).durationMs, + ) + } + + @Test + fun `dimensions come from the extractor, and from FFprobe only when it has none`() { + assertEquals(1920, MediaProbe.merge(extracted(width = 1920), info(width = 640)).width) + assertEquals(640, MediaProbe.merge(extracted = null, info = info(width = 640)).width) + assertEquals(0, MediaProbe.merge(extracted(width = 0), info(width = 0)).width) + } + + @Test + fun `the container comes from FFprobe, which is the only probe that can name one`() { + val merged = MediaProbe.merge(extracted(video = "h264"), info(container = Container.MKV)) + + assertEquals(Container.MKV, merged.container) + } + + @Test + fun `a file with audio and no video is audio-only, not unparseable`() { + val merged = MediaProbe.merge(extracted(video = null, audio = "mp3"), info = null) + + assertEquals(InputKind.AUDIO_ONLY, merged.kind) + assertFalse(merged.hasVideo) + } + + @Test + fun `a file neither probe could open is the one unreadable answer`() { + val merged = MediaProbe.merge(extracted = null, info = null) + + assertEquals(MediaProbe.UNREADABLE, merged) + assertEquals(InputProbe.UNPARSEABLE, merged.videoCodec) + } + + /** + * The arm the ticket was filed for: parsed, and carrying no stream either probe recognised. + * + * Distinct from "neither probe could open it" -- here the extractor opened the file happily and + * found nothing convertible, which is what a container holding only subtitles looks like. It + * has to reach the same [MediaProbe.UNREADABLE] answer, because the router keys off that and + * there is nothing here for Media3 to do either way. + * + * Its input was already being built elsewhere in the suite -- `MediaProbeTrackWalkTest` calls + * `extractedFrom(emptyList())` and gets exactly this -- and had simply never been handed to the + * merge. + */ + @Test + fun `a file that parsed but carries no recognised stream is unreadable too`() { + val merged = MediaProbe.merge(extracted = MediaProbe.extractedFrom(emptyList()), info = null) + + assertEquals(InputKind.UNPARSEABLE, merged.kind) + assertEquals(MediaProbe.UNREADABLE, merged) + } + + @Test + fun `hasVideo follows the codec that survived the merge, not either probe alone`() { + assertTrue(MediaProbe.merge(extracted(video = null), info(video = "vp9")).hasVideo) + assertFalse(MediaProbe.merge(extracted(video = null, audio = "aac"), info(video = null)).hasVideo) + } + + private fun extracted( + video: String? = "h264", + audio: String? = "aac", + duration: Long = 1_000L, + width: Int = 1280, + height: Int = 720, + ) = MediaProbe.Extracted(video, audio, duration, width, height) + + private fun info( + container: Container? = null, + video: String? = "h264", + audio: String? = "aac", + duration: Long = 1_000L, + width: Int = 1280, + height: Int = 720, + isImage: Boolean = false, + ) = MediaProbe.FFprobeInfo(container, video, audio, duration, width, height, isImage) +} -- 2.47.3 From aa7e1d8b01a7fc101e926dd13af292c29a32bd45 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 1 Sep 2026 22:06:52 -0500 Subject: [PATCH 2/2] C1 (#176): give ConcatWorker the seam ConversionWorker always had, and test what was behind it ConcatWorker constructed ConcatEngine in place while ConversionWorker reached its engines through ConversionDependencies. That asymmetry is the whole reason one worker had a tested failure path and the other had none: everything past setForeground was untested on *every* source set, JVM and device alike. The repo had already measured the cost and written it down. PerJobStagingTest's KDoc records that **reverting ConcatWorker to a constant staging name left all 257 tests green**, because nothing could reach the line that names the file. RefusedJobTest says it from the other side -- "the next thing past the count guard is ConcatEngine, which is native". That mutation is red now. The seam is `ConversionDependencies.concat: (Context) -> ConcatJoiner`, beside .hardware and .software. ConcatEngine implements the interface; its Result type stays nested in the implementation, because moving it would touch every call site to buy nothing -- what a test needs is the ability to not run FFmpeg, and that is the method, not the type. Five tests, and the mutations that hold them: the engine's own reason reaches the user replace e.message with the generic string a failure with no message still says one drop the ?: GENERIC_FAILURE_MESSAGE fallback a failed join deletes its partial drop staged.delete() from the catch no input array at all is refused swap in TOO_FEW_INPUTS_MESSAGE a join reports its own staged file revert to the constant staging name The delete test was vacuous on its first draft and the mutation caught it: it scanned the staging directory for a "join-" prefix that StagingNames.forJob does not produce -- it names files . -- so the assertion was trivially true. Rewritten to assert against the handle the joiner was actually given. Two small fixes ride along, both the repo's own conventions rather than new opinions. "No input files." becomes NO_INPUTS_MESSAGE, per #158: a message the user can see is named once, so a test asserts the string the worker writes rather than a copy that can drift. And the JVM now covers that arm, which ran before staging and before any native code and had no business being a device test. 579 -> 584 JVM tests, 0 failures. ConcatWorker: 14 -> 5 missed lines, 2 -> 0 missed branches. Line 2173/2348 -> 2183/2352; branch 1091/1342 unchanged in the numerator. The line denominator moved 2348 -> 2352: that is the ConcatJoiner interface, not new untested code. Said plainly because CLAUDE.md's coverage entry has a history of explaining its own numbers wrongly. Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/Transcoders.kt | 26 ++ .../ffmpeg/ConcatEngine.kt | 5 +- .../libremediaconverter/work/ConcatWorker.kt | 15 +- .../work/ConcatFailureTest.kt | 237 ++++++++++++++++++ 4 files changed, 278 insertions(+), 5 deletions(-) create mode 100644 app/src/test/java/org/libremediaconverter/work/ConcatFailureTest.kt diff --git a/app/src/main/java/org/libremediaconverter/convert/Transcoders.kt b/app/src/main/java/org/libremediaconverter/convert/Transcoders.kt index 0a0b407..49455f8 100644 --- a/app/src/main/java/org/libremediaconverter/convert/Transcoders.kt +++ b/app/src/main/java/org/libremediaconverter/convert/Transcoders.kt @@ -4,6 +4,7 @@ import android.content.Context import android.net.Uri import androidx.media3.common.util.UnstableApi import org.libremediaconverter.codec.AndroidDeviceCodecs +import org.libremediaconverter.ffmpeg.ConcatEngine import org.libremediaconverter.ffmpeg.FFmpegEngine import org.libremediaconverter.model.ConversionRequest import org.libremediaconverter.model.DeviceCodecs @@ -40,6 +41,27 @@ interface SoftwareTranscoder { ) } +/** + * The join path. Implemented by [org.libremediaconverter.ffmpeg.ConcatEngine]. + * + * Added last of the three, and the gap it closes was measured rather than guessed: + * `PerJobStagingTest`'s KDoc records that reverting `ConcatWorker` to a constant staging name left + * all 257 tests green, because nothing in the JVM suite can get past a `ConcatEngine` constructed + * in place. Everything after that line -- the failure mapping, the message fallback, the staged + * delete -- was untested on every source set. + * + * The result type stays nested in the implementation rather than being lifted here. Moving it would + * touch every call site to buy nothing: what a test needs is the ability to *not* run FFmpeg, and + * that is the method, not the type. + */ +interface ConcatJoiner { + suspend fun join( + inputs: List, + output: File, + format: OutputFormat = OutputFormat.MP4_H264, + ): ConcatEngine.Result +} + /** * The seam that lets tests force failure paths. * @@ -69,6 +91,9 @@ object ConversionDependencies { @Volatile var software: () -> SoftwareTranscoder = { FFmpegEngine() } + @Volatile + var concat: (Context) -> ConcatJoiner = { ConcatEngine(it) } + @Volatile var publisher: (Context) -> OutputPublisher = { OutputPublisher(it) } @@ -103,6 +128,7 @@ object ConversionDependencies { fun reset() { hardware = { Media3Engine(it) } software = { FFmpegEngine() } + concat = { ConcatEngine(it) } publisher = { OutputPublisher(it) } deviceCodecs = { AndroidDeviceCodecs.get() } probe = { context, uri -> MediaProbe.probe(context, uri) } diff --git a/app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt b/app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt index 1afff26..932a32d 100644 --- a/app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt +++ b/app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt @@ -7,6 +7,7 @@ import com.arthenica.ffmpegkit.FFmpegKit import com.arthenica.ffmpegkit.FFmpegKitConfig import com.arthenica.ffmpegkit.ReturnCode import kotlinx.coroutines.suspendCancellableCoroutine +import org.libremediaconverter.convert.ConcatJoiner import org.libremediaconverter.convert.MediaProbe import org.libremediaconverter.convert.StagingNames import org.libremediaconverter.model.ConcatPlanner @@ -24,11 +25,11 @@ import kotlin.coroutines.resumeWithException * reliably fail when they differ — it can emit a file whose later segments are * garbled. See [ConcatPlanner]. */ -class ConcatEngine(private val context: Context) { +class ConcatEngine(private val context: Context) : ConcatJoiner { data class Result(val strategy: ConcatStrategy, val output: File) - suspend fun join(inputs: List, output: File, format: OutputFormat = OutputFormat.MP4_H264): Result { + override suspend fun join(inputs: List, output: File, format: OutputFormat): Result { require(inputs.size >= 2) { "Joining needs at least two files." } val paths = inputs.map { uri -> diff --git a/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt b/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt index bd839c7..8d75d97 100644 --- a/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt +++ b/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt @@ -15,7 +15,6 @@ import kotlinx.coroutines.CancellationException import org.libremediaconverter.convert.ConversionDependencies import org.libremediaconverter.convert.InputQuery import org.libremediaconverter.convert.StagingNames -import org.libremediaconverter.ffmpeg.ConcatEngine import org.libremediaconverter.model.OutputFormat /** @@ -37,7 +36,7 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker override suspend fun doWork(): Result { val uris = inputData.getStringArray(KEY_INPUT_URIS)?.map(Uri::parse) - ?: return Result.failure(workDataOf(KEY_ERROR to "No input files.")) + ?: return Result.failure(workDataOf(KEY_ERROR to NO_INPUTS_MESSAGE)) if (uris.size < 2) { return Result.failure(workDataOf(KEY_ERROR to TOO_FEW_INPUTS_MESSAGE)) } @@ -77,7 +76,7 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker ), ) - val result = ConcatEngine(applicationContext).join(uris, staged, format) + val result = ConversionDependencies.concat(applicationContext).join(uris, staged, format) Result.success( workDataOf( KEY_OUTPUT_PATH to staged.absolutePath, @@ -148,6 +147,16 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker * Here rather than in the ViewModel because the rule is the worker's: `request(...)` takes * a `List` and checks nothing about its length, so this is the guard that always runs. */ + /** + * A job carrying no input array at all -- a downgrade, or a queue entry from a build that + * spelled the key differently. + * + * A constant rather than the literal it was, for the convention #158 established: a message + * the user can see is named once, so a test asserts the same string the worker writes + * rather than a copy of it that can drift. + */ + const val NO_INPUTS_MESSAGE: String = "No input files." + const val TOO_FEW_INPUTS_MESSAGE: String = "Pick at least two files to join." /** diff --git a/app/src/test/java/org/libremediaconverter/work/ConcatFailureTest.kt b/app/src/test/java/org/libremediaconverter/work/ConcatFailureTest.kt new file mode 100644 index 0000000..27cbeb7 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/ConcatFailureTest.kt @@ -0,0 +1,237 @@ +package org.libremediaconverter.work + +import android.app.Application +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.Data +import androidx.work.ListenableWorker +import androidx.work.testing.TestListenableWorkerBuilder +import androidx.work.workDataOf +import kotlinx.coroutines.runBlocking +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.convert.ConcatJoiner +import org.libremediaconverter.convert.ConversionDependencies +import org.libremediaconverter.convert.StagingNames +import org.libremediaconverter.convert.installTestWorkManager +import org.libremediaconverter.ffmpeg.ConcatEngine +import org.libremediaconverter.model.ConcatStrategy +import org.libremediaconverter.model.OutputFormat +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File +import java.util.UUID + +/** + * What a join does when the engine fails partway. + * + * Everything past `ConcatWorker`'s `setForeground` was untested on **every** source set, and the + * repo had already measured the cost: `PerJobStagingTest`'s KDoc records that reverting + * `ConcatWorker` to a constant staging name left all 257 tests green, because nothing in the JVM + * suite can get past a `ConcatEngine` constructed in place. `RefusedJobTest` says the same from the + * other side -- "the next thing past the count guard is `ConcatEngine`, which is native". + * `ConcatEngineTest` on a device tests the engine directly, bypassing the worker, and + * `ConcatWorkerTest` covers only the too-few-inputs guard and the happy path. + * + * `ConversionDependencies.concat` is the seam that closes it, added here to sit beside the + * `.hardware` and `.software` that `ConversionWorker` has had all along -- the asymmetry between the + * two workers was the whole reason one of them had a tested failure path and the other did not. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class ConcatFailureTest { + + private lateinit var app: Application + private lateinit var publisher: AlwaysRoomPublisher + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = AlwaysRoomPublisher(app) + ConversionDependencies.publisher = { publisher } + installTestWorkManager(app, Data.EMPTY) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `a join whose engine fails reports the engine's own reason`() { + ConversionDependencies.concat = { FailingJoiner { error(DEMUXER_MESSAGE) } } + + val result = runBlocking { joinWorker().doWork() } + + assertEquals( + ListenableWorker.Result.failure(workDataOf(ConcatWorker.KEY_ERROR to DEMUXER_MESSAGE)), + result, + ) + } + + /** + * A failure carrying no message at all, which Kotlin and Java both allow and FFmpegKit's + * wrappers can produce. + * + * Without the fallback the user is shown an empty error, and `JoinViewModel` cannot tell that + * from a job that reported nothing -- the two would be one blank screen with different causes. + */ + @Test + fun `a failure with no message of its own still says something`() { + ConversionDependencies.concat = { FailingJoiner { throw RuntimeException() } } + + val result = runBlocking { joinWorker().doWork() } + + assertEquals( + ListenableWorker.Result.failure( + workDataOf(ConcatWorker.KEY_ERROR to ConcatWorker.GENERIC_FAILURE_MESSAGE), + ), + result, + ) + } + + /** + * The staged file is deleted on the way out. + * + * The joiner writes before it fails, exactly as `PartialThenFailingTranscoder` does on the + * conversion side: a stub that only threw would let a missing `staged.delete()` pass. What it + * costs to lose is a full-size partial per failed join, sitting in cache until the sweep is old + * enough to be sure nobody is coming back for it. + * + * Asserted against the file the joiner was actually handed rather than by scanning the staging + * directory for a name. The first draft did scan, for a `"join-"` prefix that + * `StagingNames.forJob` does not produce -- it names files `.` -- so the assertion + * was trivially true and the mutation walked straight through it. + */ + @Test + fun `a failed join leaves nothing behind in staging`() { + val joiner = FailingJoiner { error(DEMUXER_MESSAGE) } + ConversionDependencies.concat = { joiner } + + runBlocking { joinWorker().doWork() } + + val staged = requireNotNull(joiner.lastOutput) { "the joiner never ran, so this proves nothing" } + assertEquals( + "the fixture has to write before it fails, or the delete is unobservable", + PARTIAL_BYTES, + joiner.bytesWritten, + ) + assertFalse("a failed join must not leave its partial behind: $staged", staged.exists()) + } + + /** + * The success path, and the staging name #159's fixture and `PerJobStagingTest` both care about. + * + * Worth its place rather than a happy-path formality: `PerJobStagingTest`'s KDoc records that + * **reverting `ConcatWorker` to a constant staging name left all 257 tests green**, because + * nothing could reach the line that names the file. This is the test that was missing when that + * was written -- the join's output `Data` had never been read by anything on the JVM. + * + * The staged path is asserted to carry the job id, not a constant: two joins of the same format + * sharing one name is the defect, and `ConcatEngine`'s list file collided harder still. + */ + @Test + fun `a join that works reports its own staged file, strategy and name`() { + val joiner = SucceedingJoiner() + ConversionDependencies.concat = { joiner } + + val result = runBlocking { joinWorker().doWork() } + + assertTrue("got $result", result is ListenableWorker.Result.Success) + val data = (result as ListenableWorker.Result.Success).outputData + assertEquals( + "the staged file has to be this job's, not a name every join shares", + File(publisherStagingDir(), StagingNames.forJob(JOB_ID, OutputFormat.MP4_H264.extension)).absolutePath, + data.getString(ConcatWorker.KEY_OUTPUT_PATH), + ) + assertEquals(ConcatStrategy.STREAM_COPY.name, data.getString(ConcatWorker.KEY_STRATEGY)) + assertEquals(OutputFormat.MP4_H264.mimeType, data.getString(ConcatWorker.KEY_MIME_TYPE)) + assertTrue( + "the save dialog needs a name with the right extension, got ${data.getString( + ConcatWorker.KEY_SUGGESTED_NAME, + )}", + data.getString(ConcatWorker.KEY_SUGGESTED_NAME).orEmpty().endsWith(".${OutputFormat.MP4_H264.extension}"), + ) + } + + /** + * The arm beside the count guard: no URI array at all. + * + * Covered today only by `UnopenableUriTest` on a device, although it runs before staging and + * before any native code. It is the exact sibling of `RefusedJobTest`'s ConversionWorker twin, + * and it belongs on the JVM with it -- a device test for a branch that needs no device is a + * slower test that reports later. + */ + @Test + fun `a join with no input array at all is refused with a message`() { + val result = runBlocking { + TestListenableWorkerBuilder( + context = app, + inputData = workDataOf(ConcatWorker.KEY_FORMAT to OutputFormat.MP4_H264.name), + runAttemptCount = 0, + ).setId(JOB_ID).build().doWork() + } + + assertEquals( + ListenableWorker.Result.failure(workDataOf(ConcatWorker.KEY_ERROR to ConcatWorker.NO_INPUTS_MESSAGE)), + result, + ) + } + + private fun publisherStagingDir(): File? = publisher.createStagingFile("probe").parentFile + + private fun joinWorker(): ConcatWorker = TestListenableWorkerBuilder( + context = app, + inputData = workDataOf( + ConcatWorker.KEY_INPUT_URIS to arrayOf(FIRST.toString(), SECOND.toString()), + ConcatWorker.KEY_TOTAL_BYTES to TOTAL_BYTES, + ConcatWorker.KEY_FORMAT to OutputFormat.MP4_H264.name, + ), + runAttemptCount = 0, + ).setId(JOB_ID).build() + + private companion object { + val FIRST: Uri = Uri.parse("file:///tmp/one.mp4") + val SECOND: Uri = Uri.parse("file:///tmp/two.mp4") + const val TOTAL_BYTES = 2048L + const val DEMUXER_MESSAGE = "the demuxer rejected the input list" + const val PARTIAL_BYTES = 2048 + val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-00000000000b") + } +} + +/** A joiner that writes something and then fails, so a missing `staged.delete()` cannot pass. */ +private class FailingJoiner(private val failure: () -> Nothing) : ConcatJoiner { + + /** The handle the worker created, kept so a test can ask whether it survived the failure. */ + var lastOutput: File? = null + var bytesWritten = 0 + + override suspend fun join(inputs: List, output: File, format: OutputFormat): ConcatEngine.Result { + lastOutput = output + output.writeBytes(ByteArray(PARTIAL_BYTES)) + bytesWritten = PARTIAL_BYTES + failure() + } + + private companion object { + const val PARTIAL_BYTES = 2048 + } +} + +/** The joiner that finishes, so the success path and the output `Data` can be read on the JVM. */ +private class SucceedingJoiner : ConcatJoiner { + override suspend fun join(inputs: List, output: File, format: OutputFormat): ConcatEngine.Result { + output.writeBytes(ByteArray(OUTPUT_BYTES)) + return ConcatEngine.Result(ConcatStrategy.STREAM_COPY, output) + } + + private companion object { + const val OUTPUT_BYTES = 4096 + } +} -- 2.47.3