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/convert/MediaProbe.kt b/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt index d878163..b226f95 100644 --- a/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt +++ b/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt @@ -50,19 +50,42 @@ object MediaProbe { ) fun probe(context: Context, uri: Uri): InputProbe { - val extracted = probeWithExtractor(context, uri) - val info = probeWithFFprobe(context, uri) + val merged = merge(probeWithExtractor(context, uri), probeWithFFprobe(context, uri)) + if (merged.kind == InputKind.UNPARSEABLE) { + // Not a failure: an unparseable input is a strong signal that this job belongs on + // FFmpeg. Reporting an unknown codec makes the router say so. + Log.i(TAG, "Neither MediaExtractor nor FFprobe could read $uri; routing to FFmpeg.") + } + return merged + } + /** + * What the two probes together say about one input. + * + * A pure function, and `internal` for the same reason [extractedFrom] is: the precedence rules + * below are the answer to "which probe wins", and until this was pulled out of [probe] the only + * way to ask was to have a real `MediaExtractor` and a real FFprobe **disagree**, which nothing + * on any source set can arrange. `RemuxTest` drives this on a device against committed + * fixtures, but only ever with one probe answering and the other agreeing or also failing -- + * so every elvis here was taken in one direction and never the other. + * + * The rules, each of which is a decision rather than an accident: + * + * - **The extractor wins on codecs.** It is the platform's own view of what it can decode, + * which is the thing the router is about to ask about. FFprobe's name for the same track can + * differ, and the copy planner keys off these strings. + * - **FFprobe alone reports the container.** `MediaExtractor` cannot, which is why [InputProbe] + * carries a nullable one and `CopyPlanner` treats null as "container unknown". + * - **Duration is the larger of the two**, not the first non-zero. Either probe can report zero + * for a file the other times correctly, and a zero duration makes the FFmpeg progress + * percentage undefined. + */ + internal fun merge(extracted: Extracted?, info: FFprobeInfo?): InputProbe { val videoCodec = extracted?.videoCodec ?: info?.videoCodec val audioCodec = extracted?.audioCodec ?: info?.audioCodec val kind = classify(extracted, info) - if (kind == InputKind.UNPARSEABLE) { - // Not a failure: an unparseable input is a strong signal that this job belongs on - // FFmpeg. Reporting an unknown codec makes the router say so. - Log.i(TAG, "Neither MediaExtractor nor FFprobe could read $uri; routing to FFmpeg.") - return UNREADABLE - } + if (kind == InputKind.UNPARSEABLE) return UNREADABLE return InputProbe( videoCodec = videoCodec, @@ -83,7 +106,7 @@ object MediaProbe { * audio file and a corrupt file indistinguishable. The source-info card cannot describe either * honestly until they are separate, and neither can the copy planner. */ - private fun classify(extracted: Extracted?, info: FFprobeInfo?): InputKind = when { + internal fun classify(extracted: Extracted?, info: FFprobeInfo?): InputKind = when { info?.isImage == true -> InputKind.IMAGE extracted == null && info == null -> InputKind.UNPARSEABLE (extracted?.videoCodec ?: info?.videoCodec) != null -> InputKind.VIDEO @@ -162,7 +185,11 @@ object MediaProbe { } } - private class FFprobeInfo( + /** + * `internal` rather than `private` for the same reason [Extracted] is, and it should have been + * from the start: [merge] cannot be named from a test while half its signature is private. + */ + internal class FFprobeInfo( val container: Container?, val videoCodec: String?, val audioCodec: String?, diff --git a/app/src/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/convert/MediaProbeMergeTest.kt b/app/src/test/java/org/libremediaconverter/convert/MediaProbeMergeTest.kt new file mode 100644 index 0000000..173959a --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/MediaProbeMergeTest.kt @@ -0,0 +1,166 @@ +package org.libremediaconverter.convert + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test +import org.libremediaconverter.model.Container +import org.libremediaconverter.model.InputKind +import org.libremediaconverter.model.InputProbe + +/** + * Which of the two probes wins, when they disagree. + * + * [MediaProbe.probe] runs `MediaExtractor` and FFprobe independently and then merges the two, and + * every rule in that merge is a decision. None of them had a test, for a reason that is structural + * rather than an oversight: `RemuxTest` drives the whole thing on a device against committed + * fixtures, but only ever with **one probe answering and the other agreeing or also failing**. + * Nothing on any source set can arrange for a real extractor and a real FFprobe to disagree, so + * every elvis in the merge was taken in one direction and never the other. + * + * Cutting `merge` out of `probe` is what makes the question askable. Both halves of its signature + * had to become `internal` for that -- `Extracted` already was, with a KDoc giving this exact + * reason; `FFprobeInfo` simply never got the same treatment. + */ +class MediaProbeMergeTest { + + /** + * The rule with the loudest failure mode, and `isImageFormat`'s own KDoc names it: a false + * positive here "makes the source-info card describe a video as an image". So the image verdict + * has to beat a real video codec from the extractor, and the ordering that makes it do so is + * the first arm of `classify` rather than anything a reader would infer from the fields. + */ + @Test + fun `an image verdict from FFprobe beats a video codec from the extractor`() { + val merged = MediaProbe.merge( + extracted = extracted(video = "h264"), + info = info(video = "mjpeg", isImage = true), + ) + + assertEquals(InputKind.IMAGE, merged.kind) + } + + @Test + fun `the extractor wins on codecs, because it is the view the router will act on`() { + val merged = MediaProbe.merge( + extracted = extracted(video = "h264", audio = "aac"), + info = info(video = "hevc", audio = "mp3"), + ) + + assertEquals("h264", merged.videoCodec) + assertEquals("aac", merged.audioCodec) + } + + @Test + fun `FFprobe answers for a file the extractor could not open`() { + val merged = MediaProbe.merge(extracted = null, info = info(video = "vp9", audio = "opus")) + + assertEquals("vp9", merged.videoCodec) + assertEquals("opus", merged.audioCodec) + assertEquals(InputKind.VIDEO, merged.kind) + } + + @Test + fun `the extractor answers for a file FFprobe could not read`() { + val merged = MediaProbe.merge(extracted = extracted(video = "h264", audio = "aac"), info = null) + + assertEquals("h264", merged.videoCodec) + assertEquals("aac", merged.audioCodec) + assertNull("only FFprobe can name the container, so it stays unknown here", merged.container) + } + + /** + * The larger of the two, not the first non-zero. + * + * Either probe can report zero for a file the other times correctly, and a zero duration makes + * the FFmpeg progress percentage undefined -- `FFmpegEngine` divides by it. Both orderings are + * asserted because "take the extractor's" and "take the larger" agree in one direction and not + * the other, and only one of them is the rule. + */ + @Test + fun `duration is the longer of the two readings, whichever probe supplied it`() { + assertEquals( + 5_000L, + MediaProbe.merge(extracted(duration = 0L), info(duration = 5_000L)).durationMs, + ) + assertEquals( + 5_000L, + MediaProbe.merge(extracted(duration = 5_000L), info(duration = 0L)).durationMs, + ) + } + + @Test + fun `dimensions come from the extractor, and from FFprobe only when it has none`() { + assertEquals(1920, MediaProbe.merge(extracted(width = 1920), info(width = 640)).width) + assertEquals(640, MediaProbe.merge(extracted = null, info = info(width = 640)).width) + assertEquals(0, MediaProbe.merge(extracted(width = 0), info(width = 0)).width) + } + + @Test + fun `the container comes from FFprobe, which is the only probe that can name one`() { + val merged = MediaProbe.merge(extracted(video = "h264"), info(container = Container.MKV)) + + assertEquals(Container.MKV, merged.container) + } + + @Test + fun `a file with audio and no video is audio-only, not unparseable`() { + val merged = MediaProbe.merge(extracted(video = null, audio = "mp3"), info = null) + + assertEquals(InputKind.AUDIO_ONLY, merged.kind) + assertFalse(merged.hasVideo) + } + + @Test + fun `a file neither probe could open is the one unreadable answer`() { + val merged = MediaProbe.merge(extracted = null, info = null) + + assertEquals(MediaProbe.UNREADABLE, merged) + assertEquals(InputProbe.UNPARSEABLE, merged.videoCodec) + } + + /** + * The arm the ticket was filed for: parsed, and carrying no stream either probe recognised. + * + * Distinct from "neither probe could open it" -- here the extractor opened the file happily and + * found nothing convertible, which is what a container holding only subtitles looks like. It + * has to reach the same [MediaProbe.UNREADABLE] answer, because the router keys off that and + * there is nothing here for Media3 to do either way. + * + * Its input was already being built elsewhere in the suite -- `MediaProbeTrackWalkTest` calls + * `extractedFrom(emptyList())` and gets exactly this -- and had simply never been handed to the + * merge. + */ + @Test + fun `a file that parsed but carries no recognised stream is unreadable too`() { + val merged = MediaProbe.merge(extracted = MediaProbe.extractedFrom(emptyList()), info = null) + + assertEquals(InputKind.UNPARSEABLE, merged.kind) + assertEquals(MediaProbe.UNREADABLE, merged) + } + + @Test + fun `hasVideo follows the codec that survived the merge, not either probe alone`() { + assertTrue(MediaProbe.merge(extracted(video = null), info(video = "vp9")).hasVideo) + assertFalse(MediaProbe.merge(extracted(video = null, audio = "aac"), info(video = null)).hasVideo) + } + + private fun extracted( + video: String? = "h264", + audio: String? = "aac", + duration: Long = 1_000L, + width: Int = 1280, + height: Int = 720, + ) = MediaProbe.Extracted(video, audio, duration, width, height) + + private fun info( + container: Container? = null, + video: String? = "h264", + audio: String? = "aac", + duration: Long = 1_000L, + width: Int = 1280, + height: Int = 720, + isImage: Boolean = false, + ) = MediaProbe.FFprobeInfo(container, video, audio, duration, width, height, isImage) +} 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) 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, )