From 8ab433b647135059a9def3ef2b42b659d810540d Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 26 Aug 2026 22:24:10 -0500 Subject: [PATCH 1/8] C1 (#135): pin readSpec's three enum fallbacks WorkerEnumFallbackTest already existed for this defect class -- a name this build does not define, read above the try, throwing out of doWork entirely: FAILED with reschedule=false, empty output Data so the screen said "Conversion failed." with nothing else, and the staged file never deleted. It covered 2 of the 5 above-the-try reads. readSpec's three were the ones left, and all three were cold. The baseline is the part worth reviewing. readSpec returns the *entire* fallback spec the moment any one axis fails to resolve, so a test starting from MP4_H265 -- which is itself the fallback -- cannot tell a worker that read the spec correctly from one that gave up on it. These start from MKV/H.264, which differs on container and video codec at once, and assert the spec that actually reached the transcoder rather than only that a Result came back. Mutations run, four for three tests: KEY_CONTAINER `?: return fallback` -> `?: error(...)` -> container test red KEY_VIDEO_CODEC same -> video test red KEY_AUDIO_CODEC same -> audio test red fallback = MP4_H264 instead of MP4_H265 -> all three red The first three confirm the tests are isolated to their own axis; the fourth confirms they pin *which* spec ran, which is what "a Result at all" would have missed. readSpec is now fully covered, branches included. Co-Authored-By: Claude Opus 5 (1M context) --- .../work/WorkerEnumFallbackTest.kt | 64 ++++++++++++++++++- 1 file changed, 61 insertions(+), 3 deletions(-) diff --git a/app/src/test/java/org/libremediaconverter/work/WorkerEnumFallbackTest.kt b/app/src/test/java/org/libremediaconverter/work/WorkerEnumFallbackTest.kt index 32cea02..ce32a1e 100644 --- a/app/src/test/java/org/libremediaconverter/work/WorkerEnumFallbackTest.kt +++ b/app/src/test/java/org/libremediaconverter/work/WorkerEnumFallbackTest.kt @@ -22,6 +22,7 @@ import org.libremediaconverter.model.DeviceCodecs import org.libremediaconverter.model.EnginePreference import org.libremediaconverter.model.InputProbe import org.libremediaconverter.model.OutputFormat +import org.libremediaconverter.model.OutputSpec import org.libremediaconverter.model.QualityTier import org.robolectric.RobolectricTestRunner import org.robolectric.RuntimeEnvironment @@ -109,6 +110,50 @@ class WorkerEnumFallbackTest { ) } + @Test + fun `a container this build does not define falls back to the default spec`() { + assertFallsBackToDefault(container = "HOLOTAPE") + } + + @Test + fun `a video codec this build does not define falls back to the default spec`() { + assertFallsBackToDefault(video = "H267") + } + + @Test + fun `an audio codec this build does not define falls back to the default spec`() { + assertFallsBackToDefault(audio = "SUPER_AAC") + } + + /** + * Drives a job whose spec is [NOT_THE_FALLBACK] on every axis but the one named, and asserts the + * whole spec came back as [DEFAULT_SPEC]. + * + * **The baseline is the point.** `readSpec` returns the *entire* fallback spec the moment any + * one axis fails to resolve, so a test starting from `MP4_H265` -- which is itself the fallback + * -- could not tell a worker that read the spec correctly from one that gave up on it. Starting + * from MKV/H.264 makes the difference visible on two axes at once. + * + * Asserting the spec that *ran*, rather than only that a `Result` came back, is the other half: + * the defect these three are written for threw out of `doWork` entirely, so "a Result at all" + * would pass against a fallback to something arbitrary. + */ + private fun assertFallsBackToDefault( + container: String = NOT_THE_FALLBACK.container.name, + video: String = NOT_THE_FALLBACK.videoCodec.name, + audio: String = NOT_THE_FALLBACK.audioCodec.name, + ) { + val transcoder = RequestRecordingTranscoder() + ConversionDependencies.software = { transcoder } + + val result = runBlocking { + conversionWorker(container = container, video = video, audio = audio).doWork() + } + + assertEquals(ListenableWorker.Result.success(), stripOutput(result)) + assertEquals(listOf(DEFAULT_SPEC), transcoder.specs) + } + /** [ListenableWorker.Result.Success] compares its output data, which these tests do not pin. */ private fun stripOutput(result: ListenableWorker.Result): ListenableWorker.Result = if (result is ListenableWorker.Result.Success) ListenableWorker.Result.success() else result @@ -116,15 +161,18 @@ class WorkerEnumFallbackTest { private fun conversionWorker( quality: String = QualityTier.FAST.name, preference: String = EnginePreference.FORCE_SOFTWARE.name, + container: String = SPEC.container.name, + video: String = SPEC.videoCodec.name, + audio: String = SPEC.audioCodec.name, ): ConversionWorker = TestListenableWorkerBuilder( context = app, inputData = workDataOf( ConversionWorker.KEY_INPUT_URI to INPUT.toString(), ConversionWorker.KEY_DISPLAY_NAME to DISPLAY_NAME, ConversionWorker.KEY_SIZE_BYTES to INPUT_BYTES, - 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_CONTAINER to container, + ConversionWorker.KEY_VIDEO_CODEC to video, + ConversionWorker.KEY_AUDIO_CODEC to audio, ConversionWorker.KEY_QUALITY to quality, ConversionWorker.KEY_ENGINE_PREFERENCE to preference, ), @@ -146,6 +194,12 @@ class WorkerEnumFallbackTest { const val DISPLAY_NAME = "holiday.mp4" const val INPUT_BYTES = 1024L val SPEC = OutputFormat.MP4_H265.spec + + /** What `readSpec` returns when any axis fails to resolve. */ + val DEFAULT_SPEC = OutputFormat.MP4_H265.spec + + /** A spec that differs from [DEFAULT_SPEC] on container *and* video codec. See the helper. */ + val NOT_THE_FALLBACK = OutputFormat.MKV_H264.spec val CONVERSION_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000021") val CONCAT_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000022") } @@ -156,6 +210,9 @@ private class RequestRecordingTranscoder : SoftwareTranscoder { val qualities = mutableListOf() + /** The spec each run was asked for. Which one ran is what the three readSpec tests assert. */ + val specs = mutableListOf() + override suspend fun run( request: ConversionRequest, inputPath: String, @@ -164,6 +221,7 @@ private class RequestRecordingTranscoder : SoftwareTranscoder { onProgress: (Int) -> Unit, ) { qualities += request.quality + specs += request.spec output.writeBytes(ByteArray(OUTPUT_BYTES)) } From 04850a0415a627d045360acf14f065c34d24e965 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 26 Aug 2026 22:27:27 -0500 Subject: [PATCH 2/8] C2 (#136): test the audio half of validate, and the one video refusal missing The two halves of ContainerCapabilities.validate were written together and only one of them was ever checked. Six audio outcomes had no test -- every one a string the user reads -- while the video twin of each was already covered. Seven tests, deliberately shaped like their twins rather than as a fresh idea about what to assert: unidentifiable source audio on a COPY twin of `an unidentifiable source codec cannot be copied` container cannot hold the copied source twin of `a codec the container cannot hold is refused...` container cannot carry it on encode twin of `H265 in AVI is refused` this app cannot encode it twin of `copying is offered as the fix when...` accepts(_, AudioCodec.NONE, _) -> true twin of the VideoCodec.NONE arm accepts(_, AudioCodec.COPY, _) throws twin of `resolving COPY before asking the matrix is required` The seventh is not the audio axis: validateVideo's copy-into-a-container- that-cannot-hold-it refusal was the one video outcome with no test, and it is the same shape and the same file. Each asserts the message verbatim and re-validates every suggestion the refusal offers. Validation.Invalid promises its suggestions are themselves valid and names this class as the proof; the existing property test walks the presets, and no preset reaches suggestions() through validateAudio. Seven mutations run, seven red, each isolated to exactly one test: CARRIES_AUDIO check -> false encode-path test only drop the COPY error arm resolve-first test only AudioCodec.NONE -> false no-audio-track test only drop ENCODABLE_AUDIO check unencodable test only drop audio copy container check audio-copy test only drop video copy container check video-copy test only drop unidentified-audio guard unidentifiable test only Co-Authored-By: Claude Opus 5 (1M context) --- .../model/ContainerCapabilitiesTest.kt | 116 ++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/app/src/test/java/org/libremediaconverter/model/ContainerCapabilitiesTest.kt b/app/src/test/java/org/libremediaconverter/model/ContainerCapabilitiesTest.kt index daae30b..30a44e5 100644 --- a/app/src/test/java/org/libremediaconverter/model/ContainerCapabilitiesTest.kt +++ b/app/src/test/java/org/libremediaconverter/model/ContainerCapabilitiesTest.kt @@ -358,4 +358,120 @@ class ContainerCapabilitiesTest { assertEquals(emptyList(), ContainerCapabilities.encodableVideo(container)) } } + + // --- the audio axis ----------------------------------------------------- + // + // Every rule below has a video twin already tested above. The two halves of `validate` were + // written together and only one of them was ever checked, so these are deliberately shaped like + // their twins rather than as a fresh idea about what to assert. + + @Test + fun `an unidentifiable source audio codec cannot be copied`() { + // The audio twin of `an unidentifiable source codec cannot be copied`. Never guess: a copy + // of an unidentified codec is how you ship a file that does not play. + val unknownAudio = InputProbe(videoCodec = "h264", audioCodec = null, container = Container.MP4) + val spec = OutputSpec(Container.MP4, VideoCodec.H264, AudioCodec.COPY) + + val invalid = ContainerCapabilities.validate(spec, unknownAudio) as? Validation.Invalid + ?: throw AssertionError("copying an unidentified audio codec must be refused") + + assertTrue(invalid.message, invalid.message.contains("could not be identified")) + assertEverySuggestionValid(invalid, unknownAudio) + } + + @Test + fun `copying an audio codec the container cannot hold is refused`() { + // MP4 carries AAC, MP3, Opus and FLAC. Vorbis lives in Ogg and Matroska, so a stream copy + // out of a Vorbis source into MP4 has nowhere to put the track. + val vorbisAudio = InputProbe(videoCodec = "h264", audioCodec = "vorbis", container = Container.MKV) + val spec = OutputSpec(Container.MP4, VideoCodec.H264, AudioCodec.COPY) + + val invalid = ContainerCapabilities.validate(spec, vorbisAudio) as? Validation.Invalid + ?: throw AssertionError("Vorbis copied into MP4 must be refused") + + assertEquals("MP4 cannot hold Vorbis audio.", invalid.message) + assertEverySuggestionValid(invalid, vorbisAudio) + } + + @Test + fun `an audio codec the container cannot hold is refused on the encode path too`() { + // WAV carries PCM and nothing else. The twin is `H265 in AVI is refused`. + val spec = OutputSpec(Container.WAV, VideoCodec.NONE, AudioCodec.AAC) + + val invalid = ContainerCapabilities.validate(spec, mp3Source) as? Validation.Invalid + ?: throw AssertionError("AAC in WAV must be refused") + + assertEquals("WAV cannot hold AAC audio.", invalid.message) + assertEverySuggestionValid(invalid, mp3Source) + } + + @Test + fun `an audio codec this app cannot encode is refused, and copying is offered instead`() { + // Matroska carries Vorbis; nothing here encodes it. The refusal has to say so *and* say + // what would work, which is the audio twin of `copying is offered as the fix when the codec + // is right but unencodable`. + val spec = OutputSpec(Container.MKV, VideoCodec.H264, AudioCodec.VORBIS) + + val invalid = ContainerCapabilities.validate(spec, h264Source) as? Validation.Invalid + ?: throw AssertionError("encoding Vorbis must be refused") + + assertEquals( + "This app cannot encode Vorbis audio. It can still be copied from a Vorbis source.", + invalid.message, + ) + assertEverySuggestionValid(invalid, h264Source) + } + + @Test + fun `copying a video codec the container cannot hold is refused`() { + // Not the audio axis, but the one video refusal with no test: AVI predates H.265, so a + // stream copy out of an HEVC source into AVI has nowhere to put the track. `H265 in AVI is + // refused` covers the matrix; this covers what validate() does with it. + val h265Source = InputProbe(videoCodec = "hevc", audioCodec = "mp3", container = Container.MP4) + val spec = OutputSpec(Container.AVI, VideoCodec.COPY, AudioCodec.MP3) + + val invalid = ContainerCapabilities.validate(spec, h265Source) as? Validation.Invalid + ?: throw AssertionError("H.265 copied into AVI must be refused") + + assertEquals("AVI cannot hold H.265 video.", invalid.message) + assertEverySuggestionValid(invalid, h265Source) + } + + @Test + fun `no audio track is accepted by every container in both modes`() { + // The audio twin of VideoCodec.NONE -> true. A container that refused "no audio" would make + // every video-only output invalid. + Container.entries.forEach { container -> + listOf(CodecMode.COPY, CodecMode.ENCODE).forEach { mode -> + assertTrue( + "$container should accept no audio track ($mode)", + ContainerCapabilities.accepts(container, AudioCodec.NONE, mode), + ) + } + } + } + + @Test + fun `resolving audio COPY before asking the matrix is required`() { + // The audio twin of `resolving COPY before asking the matrix is required`, and the reason is + // identical: silently answering "false" would refuse a perfectly good remux. + runCatching { ContainerCapabilities.accepts(Container.MP4, AudioCodec.COPY, CodecMode.COPY) } + .onSuccess { throw AssertionError("expected audio COPY to be rejected by the matrix") } + } + + /** + * Every alternative a refusal offers has to be one the same input could actually take. + * + * `Validation.Invalid` promises exactly this and names this class as the proof. The global + * property test walks the presets; these paths reach `suggestions()` through `validateAudio`, + * which no preset does. + */ + private fun assertEverySuggestionValid(invalid: Validation.Invalid, probe: InputProbe) { + invalid.suggestions.forEach { + assertTrue( + "suggestion $it is itself invalid, so the chip leads to a second error", + ContainerCapabilities.validate(it, probe).isValid, + ) + } + } } From bb3358f209cd3c63a527589c0f57a7a546ea2e33 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 26 Aug 2026 22:33:04 -0500 Subject: [PATCH 3/8] C4 (#138): ConcatWorker's cancellation and give-up arms ConversionWorker has WorkerCancellationTest and DeniedForegroundStartTest. Its twin had the retry case only -- `a join whose foreground start is denied` already existed -- so two of ConcatWorker's three failure exits were cold: the CancellationException arm, and FOREGROUND_DENIED. Four tests, added to the files that own each rule rather than to a new ConcatWorker file, which is how this suite is organised: a file per rule, tested across both workers. The cancellation seam is worth a look in review. The conversion twin cancels inside the engine, which is honest there because ConversionDependencies has a seam for it. ConcatWorker calls ConcatEngine directly and has none -- it is native and nothing here gets past it -- so the cancellation is injected at the only other point inside the try, setForeground. That is a real shape rather than a contrivance: a job cancelled while WorkManager is promoting it is exactly when that window is open, and the catch arm cannot tell where in the try it came from. FailedFuture moved to WorkerStubs.kt on the way. Two tests now inject two different failures through it, and Kotlin will not take two file-private top-level classes of one name in one package. Four mutations, four red, each isolated: cancellation arm -> Result.failure propagation test only drop delete on cancellation cancellation-partial test only FOREGROUND_DENIED -> Result.retry past-the-bound test only drop delete on the Throwable path give-up-partial test only ConcatWorker's :92, :95-96 and :105-106 are covered; missed branches 4 -> 3. What is left is what the ticket scoped out: the two input guards (e2e), the ConcatEngine success path (native), and getForegroundInfo (#88's named exemption). Co-Authored-By: Claude Opus 5 (1M context) --- .../work/DeniedForegroundStartTest.kt | 63 ++++++++++------ .../work/WorkerCancellationTest.kt | 72 +++++++++++++++++++ .../libremediaconverter/work/WorkerStubs.kt | 26 +++++++ 3 files changed, 140 insertions(+), 21 deletions(-) diff --git a/app/src/test/java/org/libremediaconverter/work/DeniedForegroundStartTest.kt b/app/src/test/java/org/libremediaconverter/work/DeniedForegroundStartTest.kt index 2308a44..a8e4c9b 100644 --- a/app/src/test/java/org/libremediaconverter/work/DeniedForegroundStartTest.kt +++ b/app/src/test/java/org/libremediaconverter/work/DeniedForegroundStartTest.kt @@ -27,9 +27,6 @@ import org.robolectric.RobolectricTestRunner import org.robolectric.RuntimeEnvironment import java.io.File import java.util.UUID -import java.util.concurrent.ExecutionException -import java.util.concurrent.Executor -import java.util.concurrent.TimeUnit /** * That a refused foreground-service start does not end the job. @@ -125,6 +122,40 @@ class DeniedForegroundStartTest { ) } + @Test + fun `a join denied past the attempt bound fails with a message the user can act on`() { + // The join twin of the conversion case above. ConcatWorker reaches the same FailureOutcome + // through its own `when`, and that arm was the only one of its three with no test -- so a + // join that gave up silently, or gave up with an empty Data, would have looked identical to + // one that retried. + val worker = concatWorker(runAttemptCount = FailureOutcome.MAX_FOREGROUND_START_ATTEMPTS) + + val result = runBlocking { worker.doWork() } + + assertEquals( + ListenableWorker.Result.failure( + workDataOf(ConcatWorker.KEY_ERROR to FailureOutcome.FOREGROUND_DENIED_MESSAGE), + ), + result, + ) + } + + @Test + fun `a join that gives up collects the partial it had already staged`() { + // The delete lives on ConcatWorker's `catch (e: Throwable)` path, which every give-up goes + // through. Written first so a missing delete cannot pass by asking whether a file nobody + // wrote is absent. + concatStagedFile().writeBytes(ByteArray(PARTIAL_BYTES)) + + runBlocking { concatWorker(runAttemptCount = FailureOutcome.MAX_FOREGROUND_START_ATTEMPTS).doWork() } + + assertEquals( + "a join that gave up must not orphan what it staged", + emptyList(), + stagedNames(), + ) + } + private fun conversionWorker(runAttemptCount: Int = 0): ConversionWorker = TestListenableWorkerBuilder( context = app, @@ -141,18 +172,22 @@ class DeniedForegroundStartTest { .setForegroundUpdater(DenyingForegroundUpdater) .build() - private fun concatWorker(): ConcatWorker = TestListenableWorkerBuilder( + private fun concatWorker(runAttemptCount: Int = 0): ConcatWorker = TestListenableWorkerBuilder( context = app, inputData = workDataOf( ConcatWorker.KEY_INPUT_URIS to arrayOf(INPUT.toString(), "content://test/second.mp4"), ConcatWorker.KEY_TOTAL_BYTES to INPUT_BYTES, - ConcatWorker.KEY_FORMAT to OutputFormat.MP4_H264.name, + ConcatWorker.KEY_FORMAT to CONCAT_FORMAT.name, ), - runAttemptCount = 0, + runAttemptCount = runAttemptCount, ).setId(CONCAT_ID) .setForegroundUpdater(DenyingForegroundUpdater) .build() + /** The staging path the join will compute, asked for rather than spelled out here. */ + private fun concatStagedFile(): File = + publisher.createStagingFile(StagingNames.forJob(CONCAT_ID, CONCAT_FORMAT.extension)) + /** The staging path the worker will compute, asked for rather than spelled out here. */ private fun stagedFile(): File = publisher.createStagingFile(StagingNames.forJob(CONVERSION_ID, SPEC.extension)) @@ -164,6 +199,7 @@ class DeniedForegroundStartTest { const val INPUT_BYTES = 1024L const val PARTIAL_BYTES = 2048 val SPEC = OutputFormat.MP4_H265.spec + val CONCAT_FORMAT = OutputFormat.MP4_H264 val CONVERSION_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000001") val CONCAT_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000002") } @@ -182,18 +218,3 @@ private object DenyingForegroundUpdater : ForegroundUpdater { ), ) } - -/** - * An already-failed future, written out rather than pulled from a futures library. - * - * `await()` takes the `isDone` fast path and unwraps the `ExecutionException`, which is what puts - * the platform's own exception in front of the worker's catch rather than a wrapper. - */ -private class FailedFuture(private val failure: Throwable) : ListenableFuture { - override fun addListener(listener: Runnable, executor: Executor): Unit = executor.execute(listener) - override fun cancel(mayInterruptIfRunning: Boolean): Boolean = false - override fun isCancelled(): Boolean = false - override fun isDone(): Boolean = true - override fun get(): Void = throw ExecutionException(failure) - override fun get(timeout: Long, unit: TimeUnit): Void = throw ExecutionException(failure) -} diff --git a/app/src/test/java/org/libremediaconverter/work/WorkerCancellationTest.kt b/app/src/test/java/org/libremediaconverter/work/WorkerCancellationTest.kt index a04f345..0a79eb8 100644 --- a/app/src/test/java/org/libremediaconverter/work/WorkerCancellationTest.kt +++ b/app/src/test/java/org/libremediaconverter/work/WorkerCancellationTest.kt @@ -1,12 +1,16 @@ package org.libremediaconverter.work import android.app.Application +import android.content.Context import android.net.Uri import androidx.media3.common.util.UnstableApi import androidx.work.Data +import androidx.work.ForegroundInfo +import androidx.work.ForegroundUpdater import androidx.work.ListenableWorker import androidx.work.testing.TestListenableWorkerBuilder import androidx.work.workDataOf +import com.google.common.util.concurrent.ListenableFuture import kotlinx.coroutines.CancellationException import kotlinx.coroutines.runBlocking import org.junit.After @@ -18,6 +22,7 @@ import org.junit.runner.RunWith import org.libremediaconverter.convert.ConversionDependencies import org.libremediaconverter.convert.OutputPublisher import org.libremediaconverter.convert.SoftwareTranscoder +import org.libremediaconverter.convert.StagingNames import org.libremediaconverter.convert.installTestWorkManager import org.libremediaconverter.model.ConversionRequest import org.libremediaconverter.model.DeviceCodecs @@ -108,6 +113,54 @@ class WorkerCancellationTest { assertEquals("a failed attempt must not leave its partial behind", emptyList(), stagedNames()) } + @Test + fun `a cancelled join propagates instead of being turned into a Result`() { + val thrown = runCatching { runBlocking { concatWorker().doWork() } }.exceptionOrNull() + + assertTrue( + "cancellation must leave doWork as cancellation, not as a Result; got $thrown", + thrown is CancellationException, + ) + } + + @Test + fun `a cancelled join still deletes the partial it had already staged`() { + // Written first, so a missing delete cannot pass by asking whether a file nobody wrote is + // absent -- the same reason PartialThenFailingTranscoder writes before it throws. + concatStagedFile().writeBytes(ByteArray(PARTIAL_STAGED_BYTES)) + + runCatching { runBlocking { concatWorker().doWork() } } + + assertEquals("a cancelled join must not leave its partial behind", emptyList(), stagedNames()) + } + + /** + * A join whose foreground start is cancelled rather than denied. + * + * The conversion twin cancels *inside the engine*, which is the honest shape there because + * `ConversionDependencies` has a seam for it. `ConcatWorker` calls `ConcatEngine` directly and + * has no such seam -- it is native, and nothing here gets past it -- so the cancellation is + * injected at the only other point inside the `try`: `setForeground`. That is not a contrivance. + * A job cancelled while WorkManager is promoting it to the foreground is precisely when the + * window is open, and what is being tested is the `catch` arm, which cannot tell where in the + * `try` the cancellation came from. + */ + private fun concatWorker(): ConcatWorker = TestListenableWorkerBuilder( + context = app, + inputData = workDataOf( + ConcatWorker.KEY_INPUT_URIS to arrayOf(INPUT.toString(), "file:///tmp/second.mp4"), + ConcatWorker.KEY_TOTAL_BYTES to INPUT_BYTES, + ConcatWorker.KEY_FORMAT to CONCAT_FORMAT.name, + ), + runAttemptCount = 0, + ).setId(CONCAT_ID) + .setForegroundUpdater(CancellingForegroundUpdater) + .build() + + /** The staging path the join will compute, asked for rather than spelled out here. */ + private fun concatStagedFile(): File = + publisher.createStagingFile(StagingNames.forJob(CONCAT_ID, CONCAT_FORMAT.extension)) + /** * A worker routed to the software engine, which is [failure] and nothing else. * @@ -142,7 +195,10 @@ class WorkerCancellationTest { const val DISPLAY_NAME = "holiday.mp4" const val INPUT_BYTES = 1024L val SPEC = OutputFormat.MP4_H265.spec + val CONCAT_FORMAT = OutputFormat.MP4_H264 + const val PARTIAL_STAGED_BYTES = 2048 val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000003") + val CONCAT_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000004") } } @@ -167,3 +223,19 @@ private class PartialThenFailingTranscoder(private val failure: () -> Nothing) : const val PARTIAL_BYTES = 2048 } } + +/** + * Stands in for a job cancelled while WorkManager is promoting it to the foreground. + * + * The mechanism `DeniedForegroundStartTest` documents, carrying a different exception: + * `WorkForegroundUpdater` propagates whatever the future failed with, and + * `ListenableFuture.await()` unwraps the `ExecutionException`, so the worker meets a bare + * `CancellationException` exactly where a real cancellation would put one. + */ +private object CancellingForegroundUpdater : ForegroundUpdater { + override fun setForegroundAsync( + context: Context, + id: UUID, + foregroundInfo: ForegroundInfo, + ): ListenableFuture = FailedFuture(CancellationException("cancelled while going foreground")) +} diff --git a/app/src/test/java/org/libremediaconverter/work/WorkerStubs.kt b/app/src/test/java/org/libremediaconverter/work/WorkerStubs.kt index 848c04b..14f3abe 100644 --- a/app/src/test/java/org/libremediaconverter/work/WorkerStubs.kt +++ b/app/src/test/java/org/libremediaconverter/work/WorkerStubs.kt @@ -1,10 +1,14 @@ package org.libremediaconverter.work import android.content.Context +import com.google.common.util.concurrent.ListenableFuture import org.libremediaconverter.convert.OutputPublisher import org.libremediaconverter.convert.SoftwareTranscoder import org.libremediaconverter.model.ConversionRequest import java.io.File +import java.util.concurrent.ExecutionException +import java.util.concurrent.Executor +import java.util.concurrent.TimeUnit /** * Scaffolding more than one worker test needs. @@ -68,3 +72,25 @@ object WritingTranscoder : SoftwareTranscoder { private const val OUTPUT_BYTES = 512 } + +/** + * An already-failed future, written out rather than pulled from a futures library. + * + * `await()` takes the `isDone` fast path and unwraps the `ExecutionException`, which is what puts + * the original exception in front of the worker's `catch` rather than a wrapper. That is the whole + * mechanism behind driving a `ForegroundUpdater` to fail: `WorkForegroundUpdater` propagates + * whatever the future failed with rather than swallowing it, so `setForeground()` throws exactly + * what is handed here. + * + * Shared because two tests inject two different failures through it -- a denied foreground start + * and a cancellation -- and Kotlin will not take two file-private top-level classes of one name in + * one package. + */ +internal class FailedFuture(private val failure: Throwable) : ListenableFuture { + override fun addListener(listener: Runnable, executor: Executor): Unit = executor.execute(listener) + override fun cancel(mayInterruptIfRunning: Boolean): Boolean = false + override fun isCancelled(): Boolean = false + override fun isDone(): Boolean = true + override fun get(): Void = throw ExecutionException(failure) + override fun get(timeout: Long, unit: TimeUnit): Void = throw ExecutionException(failure) +} From de6d9526baab933b80721300cdfb2cdf8f7584fe Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 26 Aug 2026 22:37:23 -0500 Subject: [PATCH 4/8] C5 (#139): the two jobs ConversionWorker refuses before converting Both exits were cold, and both are reachable for the same reason: a job does not have to come from the picker. WorkManager keeps work for about a week, so a downgrade or rollback hands this build a job enqueued by another one, and request(...) is callable directly. :62 -- a missing KEY_INPUT_URI -- was untested everywhere, JVM and device. The nearest e2e test, ForcedFailureTest.aMissingInputFailsRatherThanCrashing, passes a URI pointing at a file that does not exist, which reaches the engine and fails much later with a different message. :124-126 -- the Validation.Invalid refusal -- had no test at all, though its comment names both arrival paths it exists for. Five tests, in a new file because both are about the *message*. A refusal that fails with empty output Data renders the UI's generic "Conversion failed." with nothing else to say, which is the defect shape DeniedForegroundStartTest records from the device pass; asserting the verdict alone would pass against exactly that. Two of the five are there to stop the others passing for the wrong reason: `a refused spec never reaches an engine` says it failed *before* converting rather than during, and `a valid spec is not refused` is the control -- without it every assertion here would still pass against a worker that refused everything. Three mutations, three red: change the no-input message no-input message test drop the validation refusal both refusal tests validate but keep converting both refusal tests L61-62 and L123-126 are now fully covered, branches included. Co-Authored-By: Claude Opus 5 (1M context) --- .../work/RefusedJobTest.kt | 212 ++++++++++++++++++ 1 file changed, 212 insertions(+) create mode 100644 app/src/test/java/org/libremediaconverter/work/RefusedJobTest.kt diff --git a/app/src/test/java/org/libremediaconverter/work/RefusedJobTest.kt b/app/src/test/java/org/libremediaconverter/work/RefusedJobTest.kt new file mode 100644 index 0000000..bbb274c --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/RefusedJobTest.kt @@ -0,0 +1,212 @@ +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.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.convert.ConversionDependencies +import org.libremediaconverter.convert.OutputPublisher +import org.libremediaconverter.convert.SoftwareTranscoder +import org.libremediaconverter.convert.installTestWorkManager +import org.libremediaconverter.model.AudioCodec +import org.libremediaconverter.model.ContainerCapabilities +import org.libremediaconverter.model.ConversionRequest +import org.libremediaconverter.model.DeviceCodecs +import org.libremediaconverter.model.EnginePreference +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.model.OutputFormat +import org.libremediaconverter.model.OutputSpec +import org.libremediaconverter.model.Validation +import org.libremediaconverter.model.VideoCodec +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File +import java.util.UUID + +/** + * Jobs the worker refuses before it converts anything, and what it says about them. + * + * Two exits, both cold before this file, and both reachable for the same underlying reason: **a job + * does not have to come from the picker.** WorkManager keeps queued and finished work for about a + * week, so a downgrade or a rollback hands this build a job enqueued by another one — the premise + * `WorkerEnumFallbackTest` and `JobTags` are both written on — and `ConversionWorker.request(...)` + * is callable directly. + * + * What makes these worth their own file rather than another case in an existing one is that both + * are about the *message*. A refusal that fails with empty output `Data` renders the UI's generic + * "Conversion failed." with nothing else to say, which is the defect shape `DeniedForegroundStartTest` + * records from the device pass. Asserting the verdict alone would pass against exactly that. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class RefusedJobTest { + + private lateinit var app: Application + private lateinit var publisher: OutputPublisher + private lateinit var engine: RefusingTranscoder + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = AlwaysRoomPublisher(app) + engine = RefusingTranscoder() + ConversionDependencies.publisher = { publisher } + ConversionDependencies.software = { engine } + // Neither test is about probing or about this machine's codecs; both would otherwise decide + // the outcome for reasons no assertion mentions. See WorkerCancellationTest's setUp. + ConversionDependencies.probe = { _, _ -> InputProbe() } + ConversionDependencies.deviceCodecs = { DeviceCodecs.PERMISSIVE } + installTestWorkManager(app, Data.EMPTY) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `a job with no input URI fails with a message rather than a bare failure`() { + val result = runBlocking { workerWithout(ConversionWorker.KEY_INPUT_URI).doWork() } + + // `Failure.equals` compares output data, so this pins the message and the verdict together. + assertEquals( + ListenableWorker.Result.failure(workDataOf(ConversionWorker.KEY_ERROR to "No input file.")), + result, + ) + } + + @Test + fun `a job with no input URI stages nothing`() { + // The URI read is the first thing doWork does -- above the space check, above the staging + // name, above the try. A refusal there must not have reserved anything. + runBlocking { workerWithout(ConversionWorker.KEY_INPUT_URI).doWork() } + + assertEquals("a job refused for having no input must not stage a file", emptyList(), stagedNames()) + } + + @Test + fun `a spec the picker would never have allowed is refused with the reason`() { + // WAV carries PCM and nothing else. The picker cannot produce this combination today, which + // is exactly why the worker checks: the job can arrive from a queue written before the + // settings changed, or from a direct request(...) call. + val expected = ContainerCapabilities.validate(REFUSED_SPEC, InputProbe()) as? Validation.Invalid + ?: throw AssertionError("the fixture spec is supposed to be invalid; ContainerCapabilities disagrees") + + val result = runBlocking { worker(REFUSED_SPEC).doWork() } + + assertEquals( + ListenableWorker.Result.failure(workDataOf(ConversionWorker.KEY_ERROR to expected.message)), + result, + ) + } + + @Test + fun `a refused spec never reaches an engine`() { + // The half that says it failed *before* converting rather than during. Without this, a + // worker that ran the job and then reported the validation message would pass the test + // above -- and would have spent the user's battery on a file it was going to refuse. + runBlocking { worker(REFUSED_SPEC).doWork() } + + assertTrue("a refused spec must be refused before any engine runs", engine.invocations.isEmpty()) + } + + @Test + fun `a valid spec is not refused`() { + // The control. Every assertion above is about a refusal, so without this they would all + // still pass against a worker that refused everything. + val result = runBlocking { worker(OutputFormat.MP4_H265.spec).doWork() } + + assertEquals(ListenableWorker.Result.success(), stripOutput(result)) + assertEquals(listOf(OutputFormat.MP4_H265.spec), engine.invocations) + } + + /** [ListenableWorker.Result.Success] compares its output data, which these tests do not pin. */ + private fun stripOutput(result: ListenableWorker.Result): ListenableWorker.Result = + if (result is ListenableWorker.Result.Success) ListenableWorker.Result.success() else result + + private fun worker(spec: OutputSpec): ConversionWorker = build( + workDataOf( + ConversionWorker.KEY_INPUT_URI to INPUT.toString(), + ConversionWorker.KEY_DISPLAY_NAME to DISPLAY_NAME, + ConversionWorker.KEY_SIZE_BYTES to INPUT_BYTES, + 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, + ), + ) + + /** + * The ordinary input `Data`, less one key. + * + * Built by removal rather than by spelling out a shorter map, so the test cannot drift into + * omitting something else as well and passing for a reason it does not name. + */ + private fun workerWithout(key: String): ConversionWorker { + val full = OutputFormat.MP4_H265.spec + val entries = mapOf( + ConversionWorker.KEY_INPUT_URI to INPUT.toString(), + ConversionWorker.KEY_DISPLAY_NAME to DISPLAY_NAME, + ConversionWorker.KEY_SIZE_BYTES to INPUT_BYTES, + ConversionWorker.KEY_CONTAINER to full.container.name, + ConversionWorker.KEY_VIDEO_CODEC to full.videoCodec.name, + ConversionWorker.KEY_AUDIO_CODEC to full.audioCodec.name, + ConversionWorker.KEY_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name, + ) - key + return build(Data.Builder().putAll(entries).build()) + } + + private fun build(data: Data): ConversionWorker = + TestListenableWorkerBuilder(context = app, inputData = data, runAttemptCount = 0) + .setId(JOB_ID) + .build() + + private fun stagedNames(): List = + publisher.createStagingFile("anything").parentFile?.listFiles().orEmpty().map { it.name }.sorted() + + private companion object { + val INPUT: Uri = Uri.parse("file:///tmp/holiday.mp4") + const val DISPLAY_NAME = "holiday.mp4" + const val INPUT_BYTES = 1024L + + /** WAV carries PCM and nothing else, so AAC in WAV has nowhere to go. */ + val REFUSED_SPEC = OutputSpec( + org.libremediaconverter.model.Container.WAV, + VideoCodec.NONE, + AudioCodec.AAC, + ) + val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000005") + } +} + +/** An engine that records what it was asked for and writes an output, so a success is a success. */ +private class RefusingTranscoder : SoftwareTranscoder { + + /** Every spec that actually reached an engine. Empty is the assertion for a refused job. */ + val invocations = mutableListOf() + + override suspend fun run( + request: ConversionRequest, + inputPath: String, + output: File, + durationMs: Long, + onProgress: (Int) -> Unit, + ) { + invocations += request.spec + output.writeBytes(ByteArray(OUTPUT_BYTES)) + } + + private companion object { + const val OUTPUT_BYTES = 512 + } +} From b2790e13d9d425eef4cbe3277f46493757f175aa Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 26 Aug 2026 22:51:10 -0500 Subject: [PATCH 5/8] S1 (#141): cut the track walk into a pure seam, and test the matrix #84 closed by classifying probeWithExtractor and probeForConcat as device-bound and explicitly not a gap. That was right about FFprobe and right about the measurement boundary, and wrong that these are only orchestration. The track walk is a branch matrix, and androidTest reaches it only through whatever the committed fixtures happen to contain -- so none of its rules is *chosen* by any test there. #133 offered two ways to reach it: drive ShadowMediaExtractor, or cut the loop into a pure function. Taking the second, which is the pattern CLAUDE.md names and work/FailureOutcome.kt documents. extractedFrom and concatInputFrom take List; what is left needing a device -- setDataSource, getTrackFormat, release -- is one three-line extension function, which is the thin edge androidTest should be covering. The two are deliberately not merged despite the overlap. One reads duration and not frame rate; the other reads frame rate and not duration. A merged version would compute both for every caller, and ConcatPlanner treats an unknown frame rate as "cannot prove a match" -- so a field the join flow does not need must not start arriving as a number. Eleven tests over cases no fixture provides: two video tracks, two audio tracks, audio outlasting video, a track with no KEY_DURATION, audio declared before video, a subtitle track, and no tracks at all. Six mutations, six red: last video track wins first-video test last audio track wins first-audio test duration = last rather than max longest-track test drop the containsKey guard six tests (getLong throws on a missing key) guess a frame rate of 30 no-frame-rate test join takes the last video track join frame-rate test MediaProbe's missed branches drop 91 -> 70; what is left is the FFprobe half and the two catch arms, which are native and device-bound exactly as #84 said. Co-Authored-By: Claude Opus 5 (1M context) --- .../libremediaconverter/convert/MediaProbe.kt | 134 +++++++---- .../convert/MediaProbeTrackWalkTest.kt | 221 ++++++++++++++++++ 2 files changed, 312 insertions(+), 43 deletions(-) create mode 100644 app/src/test/java/org/libremediaconverter/convert/MediaProbeTrackWalkTest.kt diff --git a/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt b/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt index de4ba3d..d878163 100644 --- a/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt +++ b/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt @@ -92,7 +92,12 @@ object MediaProbe { else -> InputKind.UNPARSEABLE } - private class Extracted( + /** + * `internal` rather than `private` so [extractedFrom] can be named from a test. The JVM test + * source set is a friend of `main`, so this stays invisible outside the module — the precedent + * is `MainActivity`'s `Destination`, and [containerFrom] beside it. + */ + internal class Extracted( val videoCodec: String?, val audioCodec: String?, val durationMs: Long, @@ -100,33 +105,55 @@ object MediaProbe { val height: Int, ) + /** + * What a set of track formats says about a file. + * + * Split out of [probeWithExtractor] so the rules below can be tested against tracks a test + * *chooses*, rather than against whatever the committed fixtures happen to contain. The device + * tests exercise this through real files; none of them can construct a two-video-track input, + * a track that omits its duration, or an audio-before-video ordering on purpose. + * + * Three rules live here, and each is a decision rather than plumbing: + * + * - **First track of a type wins.** `video == null` is the whole guard. A file with two video + * tracks must report the first, because that is the one an engine will transcode. + * - **Duration is the maximum across tracks**, not the first one found or the last. A file + * whose audio outlasts its video is ordinary, and reporting the video's length would cut the + * progress bar short. + * - **A track that omits `KEY_DURATION` contributes nothing** rather than zero. `MediaExtractor` + * omits it for plenty of real tracks — see `MediaProbeTrackFieldsTest` — and `maxOf` against a + * fabricated 0 would still be correct here, but reading a key that is absent is not. + */ + internal fun extractedFrom(formats: List): Extracted { + var video: String? = null + var audio: String? = null + var durationUs = 0L + var width = 0 + var height = 0 + + for (format in formats) { + val mime = format.getString(MediaFormat.KEY_MIME).orEmpty() + if (format.containsKey(MediaFormat.KEY_DURATION)) { + durationUs = maxOf(durationUs, format.getLong(MediaFormat.KEY_DURATION)) + } + when { + mime.startsWith("video/") && video == null -> { + video = shortName(mime) + width = format.intOr(MediaFormat.KEY_WIDTH) + height = format.intOr(MediaFormat.KEY_HEIGHT) + } + + mime.startsWith("audio/") && audio == null -> audio = shortName(mime) + } + } + return Extracted(video, audio, durationUs / US_PER_MS, width, height) + } + private fun probeWithExtractor(context: Context, uri: Uri): Extracted? { val extractor = MediaExtractor() return try { extractor.setDataSource(context, uri, null) - var video: String? = null - var audio: String? = null - var durationUs = 0L - var width = 0 - var height = 0 - - for (i in 0 until extractor.trackCount) { - val format = extractor.getTrackFormat(i) - val mime = format.getString(MediaFormat.KEY_MIME).orEmpty() - if (format.containsKey(MediaFormat.KEY_DURATION)) { - durationUs = maxOf(durationUs, format.getLong(MediaFormat.KEY_DURATION)) - } - when { - mime.startsWith("video/") && video == null -> { - video = shortName(mime) - width = format.intOr(MediaFormat.KEY_WIDTH) - height = format.intOr(MediaFormat.KEY_HEIGHT) - } - - mime.startsWith("audio/") && audio == null -> audio = shortName(mime) - } - } - Extracted(video, audio, durationUs / US_PER_MS, width, height) + extractedFrom(extractor.trackFormats()) } catch (e: Exception) { Log.i(TAG, "Platform extractor could not read $uri.", e) null @@ -269,25 +296,7 @@ object MediaProbe { val extractor = MediaExtractor() return try { extractor.setDataSource(context, uri, null) - var video: String? = null - var audio: String? = null - var width = 0 - var height = 0 - var fps = 0 - - for (i in 0 until extractor.trackCount) { - val format = extractor.getTrackFormat(i) - val mime = format.getString(MediaFormat.KEY_MIME).orEmpty() - if (mime.startsWith("video/") && video == null) { - video = shortName(mime) - width = format.intOr(MediaFormat.KEY_WIDTH) - height = format.intOr(MediaFormat.KEY_HEIGHT) - fps = format.intOr(MediaFormat.KEY_FRAME_RATE) - } else if (mime.startsWith("audio/") && audio == null) { - audio = shortName(mime) - } - } - ConcatInput(video, audio, width, height, fps) + concatInputFrom(extractor.trackFormats()) } catch (e: Exception) { Log.i(TAG, "Could not probe $uri for concat; will re-encode.", e) ConcatInput(null, null, 0, 0, 0) @@ -296,6 +305,45 @@ object MediaProbe { } } + /** + * The join flow's read of the same track formats. See [extractedFrom] for why this is separate + * from the extractor. + * + * Deliberately **not** folded into [extractedFrom] despite the overlap. This one reads frame + * rate and does not read duration; that one reads duration and does not read frame rate. A + * merged version would have to compute both for every caller, and `ConcatPlanner` treats an + * unknown frame rate as "cannot prove a match" — so a field this flow does not need must not + * start arriving as a number. + */ + internal fun concatInputFrom(formats: List): ConcatInput { + var video: String? = null + var audio: String? = null + var width = 0 + var height = 0 + var fps = 0 + + for (format in formats) { + val mime = format.getString(MediaFormat.KEY_MIME).orEmpty() + if (mime.startsWith("video/") && video == null) { + video = shortName(mime) + width = format.intOr(MediaFormat.KEY_WIDTH) + height = format.intOr(MediaFormat.KEY_HEIGHT) + fps = format.intOr(MediaFormat.KEY_FRAME_RATE) + } else if (mime.startsWith("audio/") && audio == null) { + audio = shortName(mime) + } + } + return ConcatInput(video, audio, width, height, fps) + } + + /** + * Every track format this extractor holds, read once. + * + * The thin edge the two pure functions above leave behind: a `trackCount` and a + * `getTrackFormat` per index, which is the whole of what needs a real `MediaExtractor`. + */ + private fun MediaExtractor.trackFormats(): List = (0 until trackCount).map(::getTrackFormat) + /** * One track property as an Int, or [fallback] when the format has no Int to give. * diff --git a/app/src/test/java/org/libremediaconverter/convert/MediaProbeTrackWalkTest.kt b/app/src/test/java/org/libremediaconverter/convert/MediaProbeTrackWalkTest.kt new file mode 100644 index 0000000..f7fbabb --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/MediaProbeTrackWalkTest.kt @@ -0,0 +1,221 @@ +package org.libremediaconverter.convert + +import android.media.MediaFormat +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * The rules `MediaProbe` applies to a set of track formats. + * + * ## Why this exists, and what it revises + * + * Issue #84 classified `probeWithExtractor` and `probeForConcat` as device-bound and explicitly not + * a gap: + * + * > These are exercised by `RemuxTest`, `ConcatEngineTest` and `RealMediaBenchmark` in + * > `androidTest` … **Do not read their 0% as untested.** + * + * That was right about the measurement boundary and right about FFprobe. It was not right that + * these are only orchestration. The track walk is a **branch matrix**, and `androidTest` reaches it + * only through whatever the committed fixtures happen to contain — so none of the rules below is + * *chosen* by any test there. A fixture with two video tracks, a track that omits its duration, or + * an audio-before-video ordering is not something a device test would produce on purpose. + * + * The seam is the answer #133 preferred over driving `ShadowMediaExtractor`: the walk is a pure + * function over `List`, and what is left needing a device — `setDataSource`, + * `getTrackFormat`, `release` — is the thin edge `androidTest` should be covering. This is the + * `work/FailureOutcome.kt` pattern `CLAUDE.md` names. + * + * `MediaFormat` is a real one throughout, not a stub. `MediaProbeTrackFieldsTest` records why that + * matters: it is a heterogeneous map whose getters throw rather than coerce, and a hand-rolled + * double would not reproduce that. + */ +@RunWith(RobolectricTestRunner::class) +class MediaProbeTrackWalkTest { + + // --- extractedFrom: the conversion flow's read --------------------------- + + @Test + fun `the first video track wins when a file carries two`() { + // `video == null` is the entire guard. A file with two video tracks must report the first, + // because that is the one an engine will transcode -- and the width and height must come + // from the same track, not be mixed across them. + val extracted = MediaProbe.extractedFrom( + listOf( + video(MediaFormat.MIMETYPE_VIDEO_AVC, width = 1920, height = 1080), + video(MediaFormat.MIMETYPE_VIDEO_HEVC, width = 640, height = 480), + ), + ) + + assertEquals("h264", extracted.videoCodec) + assertEquals(1920, extracted.width) + assertEquals(1080, extracted.height) + } + + @Test + fun `the first audio track wins when a file carries two`() { + val extracted = MediaProbe.extractedFrom( + listOf( + audio(MediaFormat.MIMETYPE_AUDIO_AAC), + audio(MediaFormat.MIMETYPE_AUDIO_OPUS), + ), + ) + + assertEquals("aac", extracted.audioCodec) + } + + @Test + fun `duration is the longest track, not the first or the last`() { + // A file whose audio outlasts its video is ordinary. Taking the video's length would cut + // the progress bar short; taking the last track's would be right only by accident of order. + val extracted = MediaProbe.extractedFrom( + listOf( + video(MediaFormat.MIMETYPE_VIDEO_AVC, durationUs = 10_000_000), + audio(MediaFormat.MIMETYPE_AUDIO_AAC, durationUs = 12_500_000), + audio(MediaFormat.MIMETYPE_AUDIO_OPUS, durationUs = 1_000_000), + ), + ) + + assertEquals(12_500L, extracted.durationMs) + } + + @Test + fun `a track that does not declare its duration contributes nothing to it`() { + // MediaExtractor omits KEY_DURATION for plenty of real tracks -- MediaProbeTrackFieldsTest + // records the same for KEY_FRAME_RATE. Reading a key that is absent is what containsKey + // stands between us and. + val extracted = MediaProbe.extractedFrom( + listOf( + video(MediaFormat.MIMETYPE_VIDEO_AVC), + audio(MediaFormat.MIMETYPE_AUDIO_AAC, durationUs = 7_000_000), + ), + ) + + assertEquals(7_000L, extracted.durationMs) + } + + @Test + fun `declaring audio before video changes nothing`() { + // Track order is a property of the container, not of the content. Both orderings have to + // reach the same answer or the same file remuxed twice would probe differently. + val videoFirst = MediaProbe.extractedFrom( + listOf( + video(MediaFormat.MIMETYPE_VIDEO_AVC, width = 1280, height = 720), + audio(MediaFormat.MIMETYPE_AUDIO_AAC), + ), + ) + val audioFirst = MediaProbe.extractedFrom( + listOf( + audio(MediaFormat.MIMETYPE_AUDIO_AAC), + video(MediaFormat.MIMETYPE_VIDEO_AVC, width = 1280, height = 720), + ), + ) + + assertEquals(videoFirst.videoCodec, audioFirst.videoCodec) + assertEquals(videoFirst.audioCodec, audioFirst.audioCodec) + assertEquals(videoFirst.width, audioFirst.width) + assertEquals(videoFirst.height, audioFirst.height) + } + + @Test + fun `a track that is neither audio nor video is ignored`() { + // Subtitle and timed-metadata tracks are common in MKV and MP4. Neither prefix matches, so + // neither slot is filled -- and, importantly, a subtitle track must not be mistaken for the + // absence of an audio track by some later `else`. + val extracted = MediaProbe.extractedFrom( + listOf( + MediaFormat().apply { setString(MediaFormat.KEY_MIME, "text/vtt") }, + video(MediaFormat.MIMETYPE_VIDEO_AVC), + ), + ) + + assertEquals("h264", extracted.videoCodec) + assertNull(extracted.audioCodec) + } + + @Test + fun `a file with no tracks reports nothing rather than zero-width video`() { + val extracted = MediaProbe.extractedFrom(emptyList()) + + assertNull(extracted.videoCodec) + assertNull(extracted.audioCodec) + assertEquals(0L, extracted.durationMs) + assertEquals(0, extracted.width) + assertEquals(0, extracted.height) + } + + @Test + fun `an audio-only file reports no video codec at all`() { + // The distinction MediaProbe.classify turns into InputKind.AUDIO_ONLY, and the reason + // `hasVideo` exists: an audio file and a corrupt file must not look alike. + val extracted = MediaProbe.extractedFrom(listOf(audio(MediaFormat.MIMETYPE_AUDIO_AAC))) + + assertNull(extracted.videoCodec) + assertEquals("aac", extracted.audioCodec) + assertEquals(0, extracted.width) + } + + // --- concatInputFrom: the join flow's read ------------------------------- + + @Test + fun `the join read takes frame rate from the first video track`() { + val input = MediaProbe.concatInputFrom( + listOf( + video(MediaFormat.MIMETYPE_VIDEO_AVC, width = 1920, height = 1080, frameRate = 30), + video(MediaFormat.MIMETYPE_VIDEO_HEVC, width = 640, height = 480, frameRate = 60), + audio(MediaFormat.MIMETYPE_AUDIO_AAC), + ), + ) + + assertEquals("h264", input.videoCodec) + assertEquals("aac", input.audioCodec) + assertEquals(1920, input.width) + assertEquals(1080, input.height) + assertEquals(30, input.frameRate) + } + + @Test + fun `a video track with no declared frame rate reports zero rather than guessing`() { + // ConcatPlanner treats 0 as "cannot prove a match" and re-encodes. A guessed 30 would read + // as agreement and produce a stream copy of clips that do not actually match -- the failure + // its KDoc says the whole flow is arranged to avoid. + val input = MediaProbe.concatInputFrom(listOf(video(MediaFormat.MIMETYPE_VIDEO_AVC))) + + assertEquals(0, input.frameRate) + } + + @Test + fun `a file with no tracks joins as entirely unknown`() { + val input = MediaProbe.concatInputFrom(emptyList()) + + assertNull(input.videoCodec) + assertNull(input.audioCodec) + assertEquals(0, input.width) + assertEquals(0, input.height) + assertEquals(0, input.frameRate) + } + + private fun video( + mime: String, + width: Int = 1920, + height: Int = 1080, + durationUs: Long? = null, + frameRate: Int? = null, + ): MediaFormat = MediaFormat.createVideoFormat(mime, width, height).apply { + durationUs?.let { setLong(MediaFormat.KEY_DURATION, it) } + frameRate?.let { setInteger(MediaFormat.KEY_FRAME_RATE, it) } + } + + private fun audio(mime: String, durationUs: Long? = null): MediaFormat = + MediaFormat.createAudioFormat(mime, SAMPLE_RATE, CHANNELS).apply { + durationUs?.let { setLong(MediaFormat.KEY_DURATION, it) } + } + + private companion object { + const val SAMPLE_RATE = 48_000 + const val CHANNELS = 2 + } +} From 44493d994386cf0ed379262816ebb382efbd3aca Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 26 Aug 2026 23:14:46 -0500 Subject: [PATCH 6/8] C5 (#139): the join side's count refusal, found by the residual-gap audit A gap audit over the eight branches merged together looked for lines still never executed and asked, for each, whether something already accounts for it. Everything mapped except one: `ConcatWorker.kt:42`, the refusal of a join with fewer than two inputs. Its neighbour maps. `ConcatWorker.kt:40` -- the missing-URI-array arm, two lines above -- is covered on the device by `UnopenableUriTest.aJoinWithNoInputArrayFailsWithAMessage`. That is invisible to JaCoCo, which measures `testDebugUnitTest` only, so the report shows both arms cold and cannot distinguish the one that is e2e-covered from the one nothing touches. Only reading the androidTest source separates them. `grep` says nothing in either source set mentions "Pick at least two files to join." Two tests here now do: - `a join of a single file is refused with a message rather than joined` pins the verdict and the message together, via `Failure.equals`, for the reason the file's header already gives. - `a join of two files is not refused for its count` is the control that puts the assertion on the boundary rather than on the string. It refuses the *space* rather than letting the job run: the next thing past the count guard is `ConcatEngine`, which is native, and `NamingPublisher`'s KDoc already records that no JVM test gets past it. A failure carrying the space message is proof execution reached line 57, which is proof it cleared line 42, at no engine cost. Reachability is the header's argument plus one of its own: `request(...)` takes a `List` and checks nothing about its length, so a one-item join is a well-formed call rather than a corrupted queue entry. Mutations, each killing exactly the test it should: | mutation | red | |---|---| | guard deleted outright | `a join of a single file is refused...` | | `uris.size < 2` -> `< 3` | `a join of two files is not refused for its count` | Restored, both green. Gate green: assembleDebug, testDebugUnitTest, compileDebugAndroidTestKotlin, ktlintCheck, detekt, lintDebug. Co-Authored-By: Claude Opus 5 (1M context) --- .../work/RefusedJobTest.kt | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/app/src/test/java/org/libremediaconverter/work/RefusedJobTest.kt b/app/src/test/java/org/libremediaconverter/work/RefusedJobTest.kt index bbb274c..f988c06 100644 --- a/app/src/test/java/org/libremediaconverter/work/RefusedJobTest.kt +++ b/app/src/test/java/org/libremediaconverter/work/RefusedJobTest.kt @@ -130,6 +130,50 @@ class RefusedJobTest { assertEquals(listOf(OutputFormat.MP4_H265.spec), engine.invocations) } + // --- the same refusal, on the join side ---------------------------------- + + @Test + fun `a join of a single file is refused with a message rather than joined`() { + // The arm beside it -- a job with no URI array at all -- is covered on the device by + // `UnopenableUriTest.aJoinWithNoInputArrayFailsWithAMessage`. This one was covered by + // nothing in either source set, which a coverage report cannot say because it cannot see + // androidTest: the two arms are adjacent lines and only one of them had a test. + // + // Reachable for the reason this file's header gives, plus one of its own: `request(...)` + // takes a `List` and checks nothing about its length, so a single-item join is a + // well-formed call, not a corrupted queue entry. + val result = runBlocking { joinWorker(INPUT).doWork() } + + assertEquals( + ListenableWorker.Result.failure( + workDataOf(ConcatWorker.KEY_ERROR to "Pick at least two files to join."), + ), + result, + ) + } + + @Test + fun `a join of two files is not refused for its count`() { + // The control, and the half that makes the test above bite on the boundary rather than on + // the message: without it, `uris.size < 3` passes everything here. + // + // It refuses the space instead of letting the job run, because the next thing past the + // count guard is `ConcatEngine`, which is native -- `NamingPublisher`'s KDoc records that + // no JVM test gets past it. A refusal with the *space* message is proof that execution + // reached line 57, which is proof it got past line 42, and it costs no engine to say so. + val noRoom = NamingPublisher(app).apply { refuseSpace = true } + ConversionDependencies.publisher = { noRoom } + + val result = runBlocking { joinWorker(INPUT, SECOND_INPUT).doWork() } + + assertEquals( + ListenableWorker.Result.failure( + workDataOf(ConcatWorker.KEY_ERROR to "Not enough free space to join these files."), + ), + result, + ) + } + /** [ListenableWorker.Result.Success] compares its output data, which these tests do not pin. */ private fun stripOutput(result: ListenableWorker.Result): ListenableWorker.Result = if (result is ListenableWorker.Result.Success) ListenableWorker.Result.success() else result @@ -171,6 +215,23 @@ class RefusedJobTest { .setId(JOB_ID) .build() + /** + * A join job carrying [inputs], a declared total, and a format. + * + * The total is declared so `hasRoomFor` takes its `hasSpaceFor` branch: the other branch is + * `hasSpaceForUnknownSize`, which `NamingPublisher` does not override and which would measure + * this machine's real disk. + */ + private fun joinWorker(vararg inputs: Uri): ConcatWorker = TestListenableWorkerBuilder( + context = app, + inputData = workDataOf( + ConcatWorker.KEY_INPUT_URIS to inputs.map(Uri::toString).toTypedArray(), + ConcatWorker.KEY_TOTAL_BYTES to INPUT_BYTES * inputs.size, + ConcatWorker.KEY_FORMAT to OutputFormat.MP4_H264.name, + ), + runAttemptCount = 0, + ).setId(JOB_ID).build() + private fun stagedNames(): List = publisher.createStagingFile("anything").parentFile?.listFiles().orEmpty().map { it.name }.sorted() @@ -179,6 +240,9 @@ class RefusedJobTest { const val DISPLAY_NAME = "holiday.mp4" const val INPUT_BYTES = 1024L + /** A join needs two, and "two" is the boundary the count guard is about. */ + val SECOND_INPUT: Uri = Uri.parse("file:///tmp/holiday-2.mp4") + /** WAV carries PCM and nothing else, so AAC in WAV has nowhere to go. */ val REFUSED_SPEC = OutputSpec( org.libremediaconverter.model.Container.WAV, From ad47ce6c9613091bae97a0b180fc84799865c99a Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 26 Aug 2026 22:58:08 -0500 Subject: [PATCH 7/8] S2 + S3 (#142, #143): the two OutputPublisher seams, and where the second one goes #142 -- openOutputStream refuses two ways and only one was reachable. A provider that has gone away throws from inside the call, which `a destination the provider will not open...` already drives. A provider that is present and declines returns null, and nothing could produce that on demand. openDestination is the seam; the test asserts the failure names the destination, which is what separates the `?: error(...)` from an NPE inside `use`. #143 -- the sweep's re-read. **The seam the ticket proposed does not reach it.** Overriding the listing fires before the entries are snapshotted, so StagingSweep.collectable is handed the new timestamp, the file is never proposed for deletion, and the guard is never exercised. Measured: with an entriesIn seam, deleting the guard outright left the test green. The race is a file that *was* collectable when the snapshot was taken and is not by the time the delete comes round, so the seam has to sit at the snapshot. `snapshot(listing)` does, and deleting the guard now reddens the test. Three mutations after the move, three red: null stream returns silently null-return test null stream via !! instead null-return test sweep deletes unconditionally race test OutputPublisher.kt now has no never-executed lines at all. Two partial branches are left and both are named exemptions rather than gaps: L216's `getOrNull() ?: false` and L304's `getOrDefault(absoluteFile)` are the failure arms of a runCatching whose body cannot be made to throw through any public entry point -- the same shape as the `size >= 0` exemption recorded in the previous commit. Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/OutputPublisher.kt | 37 +++++++++++++++- .../convert/OutputPublisherPublishTest.kt | 20 +++++++++ .../convert/OutputPublisherStagingTest.kt | 43 +++++++++++++++++-- 3 files changed, 95 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt index 4c8aceb..d532697 100644 --- a/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt +++ b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt @@ -5,6 +5,7 @@ import android.net.Uri import android.provider.DocumentsContract import android.provider.OpenableColumns import java.io.File +import java.io.OutputStream /** * What a save has to say when the staged file is not there any more. @@ -170,7 +171,7 @@ open class OutputPublisher(private val context: Context) { open fun publish(staged: File, destination: Uri) { val destinationWasEmpty = destinationIsKnownEmpty(destination) try { - val out = context.contentResolver.openOutputStream(destination) + val out = openDestination(destination) ?: error("Could not open destination for writing: $destination") out.use { sink -> staged.inputStream().use { source -> source.copyTo(sink) } } } catch (failure: Throwable) { @@ -179,6 +180,22 @@ open class OutputPublisher(private val context: Context) { } } + /** + * Opens [destination] for writing, or null when the provider will not. + * + * A seam, and a narrow one: it exists because `openOutputStream` has **two** ways of refusing + * and only one of them is reachable from a test otherwise. A provider that has gone away throws + * `FileNotFoundException` from inside the call; a provider that is present and declines returns + * null. The two are not interchangeable here — the `?: error(...)` above is the only thing that + * turns the second into a failure rather than an NPE further down — and no fake provider can be + * asked to produce a null return on demand. + * + * `protected open` rather than injected, matching `hasSpaceFor` and `createStagingFile`: + * `WorkerStubs.kt`'s publishers already override one method to force one condition. + */ + protected open fun openDestination(destination: Uri): OutputStream? = + context.contentResolver.openOutputStream(destination) + /** * True only when the destination is *positively known* to hold no bytes yet. * @@ -256,7 +273,7 @@ open class OutputPublisher(private val context: Context) { open fun sweepStaging(nowMs: Long = System.currentTimeMillis()) { val dir = stagingDir val listing = dir.listFiles() ?: return - val entries = listing.map { StagingSweep.Entry(it.name, it.lastModified()) } + val entries = snapshot(listing) StagingSweep.collectable(entries, nowMs).forEach { name -> val file = File(dir, name) // Re-read the timestamp rather than trusting the snapshot above. Between the @@ -268,6 +285,22 @@ open class OutputPublisher(private val context: Context) { } } + /** + * The name and age of everything [sweepStaging] found, read once. + * + * A seam for the *race*, not for the clock — [sweepStaging] already takes `nowMs`, so the clock + * is the caller's. What has no seam otherwise is the window between this snapshot and the + * per-file re-read below it, and that window is the entire reason the re-read exists. + * + * **It has to be here and not around `listFiles()`.** A test that changes a file before the + * listing, or during it, changes what `StagingSweep.collectable` is given — so the file is + * never proposed for deletion and the re-read is never reached. The race being modelled is a + * file that *was* collectable when the snapshot was taken and is not by the time the delete + * comes round, which is exactly one worker resuming in this same process. + */ + protected open fun snapshot(listing: Array): List = + listing.map { StagingSweep.Entry(it.name, it.lastModified()) } + private fun File.canonicalOrAbsolute(): File = runCatching { canonicalFile }.getOrDefault(absoluteFile) private companion object { diff --git a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt index aa7fd24..65ec5d6 100644 --- a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt @@ -252,6 +252,26 @@ class OutputPublisherPublishTest { } } + @Test + fun `a provider that declines by returning null fails with the destination named`() { + // openOutputStream has two ways of refusing, and only one of them is otherwise reachable. + // `a destination the provider will not open...` above drives the throwing one -- a provider + // that has gone away. This is the other: a provider that is present, answers, and hands + // back null. Without the `?: error(...)` that becomes an NPE inside `use`, which reaches + // the user as "Conversion failed." with a null message. + val nullOpening = object : OutputPublisher(context) { + override fun openDestination(destination: Uri): OutputStream? = null + } + + val failure = runCatching { nullOpening.publish(staged, documentUri) }.exceptionOrNull() + + assertTrue("a null stream must not appear to succeed, got $failure", failure != null) + assertTrue( + "the failure must name the destination rather than being a bare NPE; got ${failure?.message}", + failure?.message?.contains("Could not open destination for writing") == true, + ) + } + @Test fun `a copy that succeeds delivers every byte and deletes nothing`() { shadowOf(context.contentResolver).registerOutputStreamSupplier(documentUri) { diff --git a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt index 8c39046..c70b965 100644 --- a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt @@ -1,5 +1,6 @@ package org.libremediaconverter.convert +import android.app.Application import org.junit.Assert.assertFalse import org.junit.Assert.assertNull import org.junit.Assert.assertTrue @@ -27,14 +28,18 @@ import java.util.UUID @RunWith(RobolectricTestRunner::class) class OutputPublisherStagingTest { + private lateinit var app: Application private lateinit var cacheDir: File private lateinit var publisher: OutputPublisher @Before fun setUp() { - val context = RuntimeEnvironment.getApplication() - cacheDir = context.cacheDir - publisher = OutputPublisher(context) + // Held as a field rather than a local: the race test below builds an anonymous + // OutputPublisher, and inside that `object` expression a bare `context` resolves to the + // superclass's own constructor property, which is not initialised at the super call. + app = RuntimeEnvironment.getApplication() + cacheDir = app.cacheDir + publisher = OutputPublisher(app) } @Test @@ -150,6 +155,38 @@ class OutputPublisherStagingTest { return stagingPath } + @Test + fun `a file that stops being collectable between the listing and the delete survives`() { + // The race the second timestamp read exists for, and the only branch of it that had never + // run. The comment in sweepStaging states the cost precisely: a worker resumed by + // WorkManager -- in this same process -- could have started writing this very file, and + // unlinking an inode a running job still holds open ends with the job reporting success for + // a path that no longer exists. + // + // So: a file old enough to collect at listing time, touched to now before the delete is + // reached. StagingSweep.collectable already said yes; isCollectable has to say no. + val orphan = publisher.createStagingFile( + StagingNames.forJob(UUID.randomUUID(), "mp4"), + ).apply { writeBytes(ByteArray(4096)) } + assertTrue(orphan.setLastModified(System.currentTimeMillis() - StagingSweep.GRACE_PERIOD_MS - 60_000)) + + // Touched *after* the snapshot is taken, which is the only window that reaches the + // re-read. Doing it around listFiles() instead changes what StagingSweep.collectable is + // given, so the file is never proposed for deletion and the guard is never exercised -- + // measured, and the reason the seam sits where it does. + val racing = object : OutputPublisher(app) { + override fun snapshot(listing: Array): List = + super.snapshot(listing).also { orphan.setLastModified(System.currentTimeMillis()) } + } + + racing.sweepStaging() + + assertTrue( + "a file a live job started writing after the listing must not be unlinked", + orphan.exists(), + ) + } + @Test fun `discarding a file with no parent at all is refused`() { // A relative name has no parent directory, so `staged.parentFile` is null. The handle From 2d4898ad44e3ac6157dbb5145fa2571c9636f170 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 27 Aug 2026 09:01:40 -0500 Subject: [PATCH 8/8] Re-measure the coverage entry against the tree this branch creates 84.9% line / 63.8% branch, 454 tests -> 87.1% line (2025/2324), 69.1% branch (974/1410), 502 tests in 71 classes, as #132 and #133's ten children land. The entry already instructs re-measuring before quoting, and that is why this is here rather than in the batch: quoting these numbers before the work merged would have described a tree that did not exist. It nearly went wrong the other way too -- the first measurement for this commit was taken against a main that was three merges stale and read 85.1%. Also says something the bare numbers do not. Branch moved 5.3 points against line's 2.2, and that asymmetry is the expected shape of this kind of work rather than a curiosity: those children targeted decision code -- enum fallbacks, refusal arms, cursor shapes, a `when` over container rules -- where one test chooses a branch the suite had never taken. Line coverage barely notices that. Branch coverage is the whole point. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index c8282ab..fd473ed 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -130,8 +130,9 @@ install for code that can never run — and on API 37 the full APK does not fit - The `model` package is excluded from `ReturnCount` and `CyclomaticComplexMethod` only. It is the decision layer, where one branch is one documented user-visible outcome and the metric counts answers rather than complexity. Every other rule still applies there. -- **Coverage is reported, not gated** — **84.9% of lines (1971/2321), 63.8% of branches**, - measured 2026-08-26 with `./gradlew :app:jacocoTestReport`, against 454 JVM tests in 67 classes. +- **Coverage is reported, not gated** — **87.1% of lines (2025/2324), 69.1% of branches + (974/1410)**, measured 2026-08-27 with `./gradlew :app:jacocoTestReport`, against 502 JVM tests + in 71 classes. **Every figure this file carried before 2026-08-24 was an artifact, roughly half the real one.** Robolectric loads classes through its own sandbox classloader with no source location, JaCoCo @@ -147,12 +148,19 @@ install for code that can never run — and on API 37 the full APK does not fit disproportionately Robolectric, so each one added denominator and no numerator — the measurement was punishing exactly the tests that were hardest to write. - Two things still hold. A floor needs a baseline that has settled, and this one has not: it moved - 39 points in a single build change on 2026-08-24, then another 16 as the #52 test push and the + Two things still hold. A floor needs a baseline that has settled, and this one has not. It moved + 39 points in a single build change on 2026-08-24; then another 16 as the #52 test push and the fixes it turned up landed — 69.2% -> 84.9% line, 53.2% -> 63.8% branch — while the denominator - grew 2194 -> 2321, because that work added production code of its own. And **re-measure before - quoting**: this entry was written quoting 81.4%, measured four hours earlier, and was already - three points stale by the time it was ready to merge. + grew 2194 -> 2321, because that work added production code of its own; then again on 2026-08-27 + as #132 and #133's ten children landed — 84.9% -> 87.1% line, 63.8% -> **69.1%** branch, 454 -> + 502 tests. **Branch moved four times as far as line that last time**, and that is the shape to + expect from this kind of work rather than a surprise: those children targeted decision code — + enum fallbacks, refusal arms, cursor shapes, a `when` over container rules — where one test + chooses a branch the suite had never taken. Line coverage barely notices; branch coverage is the + whole point. + + And **re-measure before quoting**: this entry was once written quoting 81.4%, measured four hours + earlier, and was already three points stale by the time it was ready to merge. - **Testable code is not done until it is tested.** If a piece is unit testable, it gets unit tests before it counts as done. If it is e2e testable, it gets e2e tests. Both clauses apply — a change that is both needs both.