From 2fbc9571196771ea96ac18a13224358a5422695e Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 1 Sep 2026 21:45:43 -0500 Subject: [PATCH 1/2] A5 (#171): fire the muxer guard that repairs "MP4 for everything", which had never fired Media3Muxers' KDoc names the defect this guards -- "the router claimed five containers while the engine silently wrote MP4 for all of them" -- and the repair itself was untested: Media3Engine$buildTransformer$3, the requireNotNull message lambda, was four lines and four branches at 0%. Nothing had ever driven a plan whose container Media3 cannot mux, and factoryFor answers null for fourteen of them. Weakening it does not crash. The wrong output is a playable file with the wrong container, which is why a test rather than a bug report is what would catch it. Same harness and the same two disciplines as Media3EngineEmptyCompositionTest, which is the sibling this joins: assert the plan really is the one the test needs before driving the engine, and rule out CancellationException so an unresumed continuation cannot read as a pass. Three premises are asserted here rather than assumed -- that the plan is still WebM by the time the engine sees it, that Media3 really has no muxer for WebM, and that neither track was dropped, since the empty-composition refusal fires earlier and is a different test's subject. The assertion is on the exception type *and* its message, and the ticket predicted why: replacing requireNotNull with `?: DefaultMuxer.Factory()` does not make the export succeed, it lets it run on and fail some other way. Measured -- that mutation fails the type assertion, so the guard is genuinely what this test is holding, and the message assertion stands behind it. 560 -> 561 JVM tests, 0 failures. No production code changed. Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/Media3MuxerGuardTest.kt | 99 +++++++++++++++++++ 1 file changed, 99 insertions(+) create mode 100644 app/src/test/java/org/libremediaconverter/convert/Media3MuxerGuardTest.kt diff --git a/app/src/test/java/org/libremediaconverter/convert/Media3MuxerGuardTest.kt b/app/src/test/java/org/libremediaconverter/convert/Media3MuxerGuardTest.kt new file mode 100644 index 0000000..8207986 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/Media3MuxerGuardTest.kt @@ -0,0 +1,99 @@ +package org.libremediaconverter.convert + +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotEquals +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.model.AudioCodec +import org.libremediaconverter.model.AudioPlan +import org.libremediaconverter.model.Container +import org.libremediaconverter.model.ConversionRequest +import org.libremediaconverter.model.CopyPlanner +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.model.OutputSpec +import org.libremediaconverter.model.VideoCodec +import org.libremediaconverter.model.VideoPlan +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File +import java.util.concurrent.CancellationException + +/** + * A job that reached Media3 with a container Media3 cannot mux. + * + * [Media3Muxers]' own KDoc names the defect this guards: *"the router claimed five containers while + * the engine silently wrote MP4 for all of them."* `factoryFor` answers null for fourteen of the + * app's containers, and `buildTransformer` turns that null into a failed job rather than letting + * `Transformer` fall back to its default muxer. + * + * The guard had never fired. `Media3Engine$buildTransformer$3` -- the `requireNotNull` message + * lambda -- was four lines and four branches at 0%, which is to say the entire repair for a defect + * the codebase went to the trouble of writing down was untested. Weakening it would restore that + * bug silently, because the wrong output is a *playable file with the wrong container*, not a crash. + * + * Same harness and same two disciplines as [Media3EngineEmptyCompositionTest]: assert the plan + * really is the one the test needs before driving the engine, and rule out + * `CancellationException` so an unresumed continuation cannot read as a pass. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class Media3MuxerGuardTest { + + @Test + fun `a container Media3 cannot mux fails the job rather than silently writing MP4`() { + val context = RuntimeEnvironment.getApplication() + val engine = Media3Engine(context) + val request = ConversionRequest( + spec = OutputSpec(Container.WEBM, VideoCodec.VP9, AudioCodec.OPUS), + probe = InputProbe(videoCodec = "h264", audioCodec = "aac", container = Container.MP4), + ) + + // The premise, asserted rather than assumed -- three separate ways this test could pass + // over a path it never entered. + val plan = CopyPlanner.plan(request.spec, request.probe) + assertEquals("the plan has to still be WebM by the time the engine sees it", Container.WEBM, plan.container) + assertNull("...and Media3 really has no muxer for it", Media3Muxers.factoryFor(plan.container)) + // Not the empty-composition refusal, which fires earlier and is a different test's subject. + assertNotEquals(VideoPlan.Drop, plan.video) + assertNotEquals(AudioPlan.Drop, plan.audio) + + val failure = try { + runCatching { + runBlocking { + withTimeout(TIMEOUT_MS) { + engine.transcode(Uri.parse("file:///dev/null"), File(context.cacheDir, "guard.webm"), request) { + } + } + } + }.exceptionOrNull() + } finally { + engine.close() + } + + assertFalse( + "the continuation was never resumed -- the refusal escaped instead of failing the job: $failure", + failure is CancellationException, + ) + // Type *and* message, and the message half is the load-bearing one. Replacing the + // requireNotNull with a fallback factory does not make the export succeed here: it lets it + // run on and fail some other way, which a bare type assertion would happily accept. + assertTrue("expected the muxer guard to refuse the job, got $failure", failure is IllegalArgumentException) + assertTrue( + "the refusal has to name the container it could not mux, got: ${failure?.message}", + failure?.message.orEmpty().contains("cannot mux") && + failure?.message.orEmpty().contains(Container.WEBM.name), + ) + } + + private companion object { + /** Nothing is decoded or muxed on this path -- the guard refuses before any of that. */ + const val TIMEOUT_MS = 10_000L + } +} -- 2.47.3 From 0f842243b5e8fc22833207c66f6ddb336a99e828 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 1 Sep 2026 21:48:36 -0500 Subject: [PATCH 2/2] B2 (#174): read what the progress notification actually says An assertion gap rather than a coverage one, which is the reason it survived. JaCoCo is green on build()'s `if (indeterminate)` because ProgressNotificationTest drives it through a real worker -- but that test reads the notification id and EXTRA_PROGRESS and nothing else. Nothing had ever read the text. Swapping the two branches passed the whole suite; so did replacing the caller's title with a constant. Both are red now. What it costs to get wrong is small and permanent: a conversion four minutes in still saying "Preparing", or one that has not started reporting yet claiming 0%. Neither is a crash, and nothing else here would have found it. Nothing in the suite had constructed ConversionNotifications directly, and the reason turned out to be mechanical rather than an oversight: build() reaches WorkManager.getInstance for the Cancel action's PendingIntent, so the notification cannot be built without one. installTestWorkManager in setUp is the whole fixture, and the KDoc records the coupling so the next person does not rediscover it. areEnabled() in the same file is deliberately still untested. It has no caller anywhere in app/src/main, so a test would assert that a function nobody calls returns what the platform told it -- and would imply the app handles the disabled-notification case, which it does not. That is F5 in docs/coverage-read-findings.md, and it asks for a decision rather than a test. 561 -> 563 JVM tests, 0 failures. No production code changed. Co-Authored-By: Claude Opus 5 (1M context) --- .../work/NotificationProgressTextTest.kt | 89 +++++++++++++++++++ 1 file changed, 89 insertions(+) create mode 100644 app/src/test/java/org/libremediaconverter/work/NotificationProgressTextTest.kt diff --git a/app/src/test/java/org/libremediaconverter/work/NotificationProgressTextTest.kt b/app/src/test/java/org/libremediaconverter/work/NotificationProgressTextTest.kt new file mode 100644 index 0000000..7bbef1c --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/NotificationProgressTextTest.kt @@ -0,0 +1,89 @@ +package org.libremediaconverter.work + +import android.app.Notification +import androidx.media3.common.util.UnstableApi +import androidx.work.Data +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.convert.installTestWorkManager +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.util.UUID + +/** + * The two things a progress notification can say, and that they are not the same thing. + * + * An assertion gap rather than a coverage one, and the distinction is the reason this file exists. + * JaCoCo is green on `build`'s `if (indeterminate)`, because `ProgressNotificationTest` drives it + * through a real worker -- but that test reads only the notification id and + * `Notification.EXTRA_PROGRESS`. **Nothing had ever read the text.** Swapping the two branches, or + * collapsing them into one string, passed the entire suite. + * + * What it costs to get wrong is small and constant: a conversion that has been running for four + * minutes still saying "Preparing", or one that has not started reporting yet claiming 0%. Neither + * is a crash, and neither would be found by anything else here -- which is exactly the kind of + * thing that survives for a long time. + * + * Nothing else in the suite constructs [ConversionNotifications] directly. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class NotificationProgressTextTest { + + /** + * `build` reaches `WorkManager.getInstance` for the Cancel action's PendingIntent, so the + * notification cannot be built at all without one. That coupling is why nothing had ever + * constructed this class directly and read what it produced. + */ + @Before + fun setUp() { + installTestWorkManager(RuntimeEnvironment.getApplication(), Data.EMPTY) + } + + @Test + fun `an indeterminate notification says something different from a measured one`() { + val context = RuntimeEnvironment.getApplication() + val notifications = ConversionNotifications(context) + + val preparing = notifications.build(JOB_ID, TITLE, percent = 0, indeterminate = true).text() + val measured = notifications.build(JOB_ID, TITLE, percent = 42, indeterminate = false).text() + + assertNotEquals( + "the two states have to read differently, or the text says nothing at all", + preparing, + measured, + ) + assertTrue( + "a measured notification has to carry its percentage, got \"$measured\"", + measured.contains("42"), + ) + assertTrue( + "an indeterminate one must not invent one, got \"$preparing\"", + !preparing.contains("42") && !preparing.contains("0"), + ) + } + + /** + * The title is the caller's, not the builder's -- it is the file the user picked, and it is what + * tells two simultaneous conversions apart in the shade. + */ + @Test + fun `the notification is titled with the file it is converting`() { + val context = RuntimeEnvironment.getApplication() + + val built = ConversionNotifications(context).build(JOB_ID, TITLE, percent = 10) + + assertEquals(TITLE, built.extras.getString(Notification.EXTRA_TITLE)) + } + + private fun Notification.text(): String = extras.getString(Notification.EXTRA_TEXT).orEmpty() + + private companion object { + const val TITLE = "holiday.mp4" + val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-00000000000a") + } +} -- 2.47.3