From 0f842243b5e8fc22833207c66f6ddb336a99e828 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 1 Sep 2026 21:48:36 -0500 Subject: [PATCH 1/3] 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 From b41341a1cb865c1daad6c988ae346a6fc14ffd9a Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 1 Sep 2026 21:50:31 -0500 Subject: [PATCH 2/3] B3 (#175): pin the AAC arm every ordinary conversion takes audioArgs has six arms. Five are named codecs with tests; AAC arrives through the `else`, so nothing named it -- neither "aac" nor "192k" appeared anywhere in FFmpegCommandBuilderTest. It is the audio MP4 and M4A get, which is to say the audio the picker offers first and most conversions produce. Both halves are asserted, and the bitrate is the half worth arguing for: an -b:a that quietly changed would fail nothing, look wrong in no command line, and surface only as files that sound different from the ones the app produced last month. Both mutations confirmed red -- 192k -> 128k and aac -> libfdk_aac. Asserted through MP4_H264 and M4A_AAC rather than one of them, so an AAC arm added above the `else` later has to keep answering the same way for both. **Deliberately not added here: an ENCODABLE_AUDIO-vs-audioArgs agreement test**, the obvious companion to VideoCodecMimeAgreementTest. It would freeze the answer to F1, which is open: ContainerCapabilities.kt:84 says "nothing here emits a Vorbis encoder" and FFmpegCommandBuilder.kt:188 does. docs/coverage-read-findings.md says in terms that the tempting fix there locks in the wrong answer and that the decision comes first. This is the AAC arm only. 563 -> 564 JVM tests, 0 failures. No production code changed. Co-Authored-By: Claude Opus 5 (1M context) --- .../ffmpeg/FFmpegCommandBuilderTest.kt | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/app/src/test/java/org/libremediaconverter/ffmpeg/FFmpegCommandBuilderTest.kt b/app/src/test/java/org/libremediaconverter/ffmpeg/FFmpegCommandBuilderTest.kt index 936717a..9f9624d 100644 --- a/app/src/test/java/org/libremediaconverter/ffmpeg/FFmpegCommandBuilderTest.kt +++ b/app/src/test/java/org/libremediaconverter/ffmpeg/FFmpegCommandBuilderTest.kt @@ -166,6 +166,30 @@ class FFmpegCommandBuilderTest { assertPair(cmd(OutputFormat.OPUS), "-c:a", "libopus") } + /** + * The arm most conversions actually take, and the only one in `audioArgs` with no test. + * + * `flac wav and opus select the right encoders` above covers the three named arms; MP3 has its + * own. AAC arrives through the `else`, so nothing named it and nothing pinned either half of + * what it emits -- neither `aac` nor `192k` appeared anywhere in this file. Both are shipped + * defaults: MP4 and M4A are the formats the picker offers first, so this is the audio + * every ordinary conversion gets. + * + * The bitrate is asserted as well as the encoder because it is the half a refactor is likelier + * to lose. An `-b:a` that quietly changed would not fail anything, would not look wrong in a + * command line, and would show up only as files that sound different from the ones the app + * produced last month. + */ + @Test + fun `aac is the default encoder, at the bitrate the app ships`() { + assertPair(cmd(OutputFormat.MP4_H264), "-c:a", "aac") + assertPair(cmd(OutputFormat.MP4_H264), "-b:a", "192k") + // Through the `else` rather than through a named arm, so an AAC branch added above it later + // has to keep answering the same way. + assertPair(cmd(OutputFormat.M4A_AAC), "-c:a", "aac") + assertPair(cmd(OutputFormat.M4A_AAC), "-b:a", "192k") + } + @Test fun `audio only formats never carry a video encoder`() { listOf(OutputFormat.MP3, OutputFormat.FLAC, OutputFormat.WAV, OutputFormat.OPUS) -- 2.47.3 From eded47d6661608234f8a695642f8bd3873fc2dbe Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 1 Sep 2026 21:54:50 -0500 Subject: [PATCH 3/3] B1 (#173): tell the rail from the bottom bar, and the Convert tab from the Join tab Two assertion gaps, not coverage gaps, which is why they lasted. AppRootRestorationTest already drives AppRoot at Compact and Expanded, so JaCoCo is green on useRail -- but it asserts only that the selected tab survives recreation, through a stub `content` composable. Nothing anywhere queried for a rail or a bar, and nothing composed the real screens. Measured before this file existed: - transposing the NavigationRail and NavigationBar bodies passed the entire suite - transposing Content's two arms passed it too A tablet showing phone chrome, or the Convert tab opening the Join screen, and 546 tests with nothing to say about either. AppRoot's own KDoc is why that matters more than it looks: from targetSdk 37 the app is resized and rotated whether or not it is ready, so the width class is not a preference. WindowWidthSizeClass.Medium appears in no test in either source set today. useRail is `!= Compact`, so Medium takes the rail; narrowing it to `== Expanded` is one character and breaks every tablet and unfolded foldable. That mutation is red now, and it is red only because of the Medium test -- the Compact and Expanded ones both survive it. Two things this needed: **createAndroidComposeRule rather than createComposeRule.** Rendering AppRoot with its default content reaches ConverterScreen's `viewModel = viewModel()`, which needs a ViewModelStoreOwner. It works because both ViewModels are `@JvmOverloads constructor(app: Application, ...)` so AndroidViewModelFactory can build them, and because ui-test-manifest's debugImplementation entry already puts a ComponentActivity in the merged manifest the unit tests build against -- which app/build.gradle.kts says in terms. Checked with a throwaway spike before the ticket was filed, rather than discovered here. **Two tags, applied inside main.** The only production change: TestTags.Shell, set on the rail and the bar. There is no other way to tell the two apart -- both render the same two destinations with the same labels and the same selection state, so any assertion writable without them is satisfied by either layout. In TestTags and applied by the shell rather than handed down by the test, for the reason that file's KDoc gives: a tag the test supplies proves only that the test set it. TagTableUniquenessTest covers the new group. 564 -> 568 JVM tests, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) --- .../org/libremediaconverter/MainActivity.kt | 6 +- .../org/libremediaconverter/ui/TestTags.kt | 14 ++ .../libremediaconverter/AdaptiveShellTest.kt | 129 ++++++++++++++++++ .../ui/TagTableUniquenessTest.kt | 1 + 4 files changed, 148 insertions(+), 2 deletions(-) create mode 100644 app/src/test/java/org/libremediaconverter/AdaptiveShellTest.kt diff --git a/app/src/main/java/org/libremediaconverter/MainActivity.kt b/app/src/main/java/org/libremediaconverter/MainActivity.kt index b03f7c1..48f758c 100644 --- a/app/src/main/java/org/libremediaconverter/MainActivity.kt +++ b/app/src/main/java/org/libremediaconverter/MainActivity.kt @@ -24,9 +24,11 @@ import androidx.compose.runtime.saveable.Saver import androidx.compose.runtime.saveable.rememberSaveable import androidx.compose.runtime.setValue import androidx.compose.ui.Modifier +import androidx.compose.ui.platform.testTag import androidx.media3.common.util.UnstableApi import org.libremediaconverter.convert.ConverterScreen import org.libremediaconverter.join.JoinScreen +import org.libremediaconverter.ui.TestTags import org.libremediaconverter.ui.theme.LibreMediaConverterTheme /** @@ -114,7 +116,7 @@ internal fun AppRoot( if (useRail) { Row(modifier = Modifier.fillMaxSize()) { - NavigationRail { + NavigationRail(modifier = Modifier.testTag(TestTags.Shell.NAVIGATION_RAIL)) { Destination.entries.forEach { item -> NavigationRailItem( selected = destination == item, @@ -132,7 +134,7 @@ internal fun AppRoot( Scaffold( modifier = Modifier.fillMaxSize(), bottomBar = { - NavigationBar { + NavigationBar(modifier = Modifier.testTag(TestTags.Shell.NAVIGATION_BAR)) { Destination.entries.forEach { item -> NavigationBarItem( selected = destination == item, diff --git a/app/src/main/java/org/libremediaconverter/ui/TestTags.kt b/app/src/main/java/org/libremediaconverter/ui/TestTags.kt index f6fe883..8a3e0eb 100644 --- a/app/src/main/java/org/libremediaconverter/ui/TestTags.kt +++ b/app/src/main/java/org/libremediaconverter/ui/TestTags.kt @@ -56,6 +56,20 @@ object TestTags { */ const val RETRY_SAVE: String = "action.retrySave" + /** + * The adaptive shell around both screens -- `AppRoot`'s two layouts. + * + * Named because there is no other way to tell them apart from a test. Both render the same two + * destinations with the same labels and the same selection state, so every assertion that could + * be written without these tags is satisfied by either layout, and transposing the two bodies + * passed the whole suite. Exactly one of the two exists at a time, which is what makes + * `assertExists` / `assertDoesNotExist` on this pair a statement about the width class. + */ + object Shell { + const val NAVIGATION_RAIL: String = "shell.navigationRail" + const val NAVIGATION_BAR: String = "shell.navigationBar" + } + /** `ConverterScreen`. */ object Converter { const val CHOOSE_FILE: String = "converter.chooseFile" diff --git a/app/src/test/java/org/libremediaconverter/AdaptiveShellTest.kt b/app/src/test/java/org/libremediaconverter/AdaptiveShellTest.kt new file mode 100644 index 0000000..9bdfb8d --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/AdaptiveShellTest.kt @@ -0,0 +1,129 @@ +package org.libremediaconverter + +import androidx.activity.ComponentActivity +import androidx.compose.material3.windowsizeclass.WindowWidthSizeClass +import androidx.compose.ui.test.junit4.v2.createAndroidComposeRule +import androidx.compose.ui.test.onNodeWithTag +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.performClick +import androidx.media3.common.util.UnstableApi +import androidx.work.Data +import org.junit.After +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.convert.ConversionDependencies +import org.libremediaconverter.convert.installTestWorkManager +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.ui.TestTags +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment + +/** + * Which navigation affordance the shell actually renders, and which screen it actually shows. + * + * Assertion gaps rather than coverage gaps, both of them, and that is why they lasted. + * `AppRootRestorationTest` already drives `AppRoot` at `Compact` and `Expanded`, so JaCoCo is green + * on `useRail` -- but it asserts only that the *selected tab* survives recreation, through a stub + * `content` composable. Nothing anywhere queried for a rail or a bar, and nothing rendered the real + * screens. Two consequences, both measured before this file existed: + * + * - **Transposing the `NavigationRail` and `NavigationBar` bodies passed the entire suite.** + * - **Transposing `Content`'s two arms passed it too** -- a tablet showing the phone chrome, or the + * Convert tab opening the Join screen, and 546 tests with nothing to say about either. + * + * `AppRoot`'s own KDoc is why this matters more than it looks: from targetSdk 37 the app is resized + * and rotated whether or not it is ready, so the width class is not a preference, it is whatever + * the system hands over. + * + * ## Two things this needed that the rest of the suite does not + * + * **`createAndroidComposeRule`, not `createComposeRule`.** Rendering `AppRoot` with its *default* + * content reaches `ConverterScreen`'s `viewModel = viewModel()`, which needs a + * `ViewModelStoreOwner`; the plain rule supplies none. It works because both ViewModels are + * `@JvmOverloads constructor(app: Application, …)`, so `AndroidViewModelFactory` can build them, + * and because `app/build.gradle.kts` already puts `ui-test-manifest`'s `ComponentActivity` in the + * merged manifest the unit tests build against -- which that file says in terms. + * + * **Tags on the two bars.** They are in `TestTags`, applied inside `main`, for the reason that + * file's KDoc gives: a tag the test hands down proves only that the test set it. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class AdaptiveShellTest { + + @get:Rule + val composeRule = createAndroidComposeRule() + + @Before + fun setUp() { + val app = RuntimeEnvironment.getApplication() + installTestWorkManager(app, Data.EMPTY) + // The real screens are composed here, so their ViewModels are real too. Neither test is + // about probing or publishing; left alone they would reach the FFprobe loader and this + // machine's codec list, and decide things no assertion mentions. + ConversionDependencies.probe = { _, _ -> InputProbe() } + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `a phone gets the bottom bar and a tablet gets the rail`() { + setShell(WindowWidthSizeClass.Compact) + + composeRule.onNodeWithTag(TestTags.Shell.NAVIGATION_BAR).assertExists() + composeRule.onNodeWithTag(TestTags.Shell.NAVIGATION_RAIL).assertDoesNotExist() + } + + @Test + fun `an expanded window gets the rail`() { + setShell(WindowWidthSizeClass.Expanded) + + composeRule.onNodeWithTag(TestTags.Shell.NAVIGATION_RAIL).assertExists() + composeRule.onNodeWithTag(TestTags.Shell.NAVIGATION_BAR).assertDoesNotExist() + } + + /** + * The width class no test had ever passed. + * + * `useRail` is `!= Compact`, so Medium takes the rail with Expanded. Narrowing it to + * `== Expanded` is a one-character change that breaks every tablet and unfolded foldable and + * nothing else -- and until this test, nothing in either source set used `Medium` at all. + */ + @Test + fun `a medium window is a rail window, not a phone`() { + setShell(WindowWidthSizeClass.Medium) + + composeRule.onNodeWithTag(TestTags.Shell.NAVIGATION_RAIL).assertExists() + composeRule.onNodeWithTag(TestTags.Shell.NAVIGATION_BAR).assertDoesNotExist() + } + + /** + * The mapping every other test stubs out: which screen each destination actually opens. + * + * Matched on each screen's own "choose a file" affordance rather than on a title, because those + * tags are applied by the screens themselves -- so this fails if the destinations are + * transposed, and it fails for the right reason. + */ + @Test + fun `Convert opens the converter and Join opens the join screen`() { + setShell(WindowWidthSizeClass.Compact) + + composeRule.onNodeWithTag(TestTags.Converter.CHOOSE_FILE).assertExists() + composeRule.onNodeWithTag(TestTags.Join.CHOOSE_FILES).assertDoesNotExist() + + composeRule.onNodeWithText(Destination.JOIN.label).performClick() + + composeRule.onNodeWithTag(TestTags.Join.CHOOSE_FILES).assertExists() + composeRule.onNodeWithTag(TestTags.Converter.CHOOSE_FILE).assertDoesNotExist() + } + + /** [AppRoot] with its real content, which is the half nothing else composes. */ + private fun setShell(width: WindowWidthSizeClass) { + composeRule.setContent { AppRoot(width) } + } +} diff --git a/app/src/test/java/org/libremediaconverter/ui/TagTableUniquenessTest.kt b/app/src/test/java/org/libremediaconverter/ui/TagTableUniquenessTest.kt index 333b051..bd59f2f 100644 --- a/app/src/test/java/org/libremediaconverter/ui/TagTableUniquenessTest.kt +++ b/app/src/test/java/org/libremediaconverter/ui/TagTableUniquenessTest.kt @@ -28,6 +28,7 @@ class TagTableUniquenessTest { fun `every tag constant has its own value`() { val tags = tagsIn( TestTags::class.java, + TestTags.Shell::class.java, TestTags.Converter::class.java, TestTags.Join::class.java, ) -- 2.47.3