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 <jobId>.<ext> -- 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) <noreply@anthropic.com>
This commit is contained in:
2026-09-01 22:06:52 -05:00
co-authored by Claude Opus 5
parent 794cef7b34
commit aa7e1d8b01
4 changed files with 278 additions and 5 deletions
@@ -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<Uri>,
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) }
@@ -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<Uri>, output: File, format: OutputFormat = OutputFormat.MP4_H264): Result {
override suspend fun join(inputs: List<Uri>, output: File, format: OutputFormat): Result {
require(inputs.size >= 2) { "Joining needs at least two files." }
val paths = inputs.map { uri ->
@@ -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<Uri>` 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."
/**