From fea88a281f41f0c64ea311a5e791967676b7af5a Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 24 Aug 2026 16:10:38 -0500 Subject: [PATCH] Say in tests what the file card says when it does not know "Size unknown" is the line a stream fixing D5 reported as untestable. It is two assertTextEquals calls, and it needed two rather than one: the size line renders independently of the probe, so it is asserted with a probe and without one. That independence is the contract, and a test of the probed case alone would leave the branch a user hits first -- the card is on screen before the probe finishes -- unguarded. The rest of the card degrades in words the same way, and none of it was covered: the four InputKind branches, "No video track", "No audio track", describeVideo's "Unknown", and the two `> 0` guards that drop the dimension and length rows rather than printing 0 and 0:00. Each guard gets a case on both sides, because the present side alone stays green when the guard is deleted -- what deleting it produces is "Size: 0x0" and "Length: 0:00", the same invented-measurement defect as "0 B". The four pure helpers go in a plain JVM class beside it, with formatBytes pinned at each threshold and one byte below it. A `>=` quietly becoming a `>` is only visible from a value sitting exactly on the boundary. Two things the issue could not have known: - Its second acceptance criterion, "delete the return@Column and watch the Reading... test go red", cannot happen -- it does not compile. The early return is what smart-casts `probe` non-null, so ten uses below it fail with "Only safe (?.) or non-null asserted (!!.) calls are allowed on a nullable receiver". The exit is enforced by the compiler, not by a test. Both compilable regressions someone would land instead are covered and were run red. - CodecNames.describeAudio has no UNPARSEABLE arm, unlike describeVideo, so it answers the raw sentinel rather than "Unrecognised". Unreachable today, because the UNPARSEABLE kind renders the explanatory line instead of rows. Left alone; recorded on the PR for R38.5. The divider's absence is not asserted and cannot be: Material 3 renders it as a Box with no semantics modifier, so it contributes no node. What is asserted is everything it precedes, plus the card's child count. The class KDoc says so. Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/ConverterFormattersTest.kt | 152 +++++++++++ .../convert/FileCardTest.kt | 254 ++++++++++++++++++ 2 files changed, 406 insertions(+) create mode 100644 app/src/test/java/org/libremediaconverter/convert/ConverterFormattersTest.kt create mode 100644 app/src/test/java/org/libremediaconverter/convert/FileCardTest.kt diff --git a/app/src/test/java/org/libremediaconverter/convert/ConverterFormattersTest.kt b/app/src/test/java/org/libremediaconverter/convert/ConverterFormattersTest.kt new file mode 100644 index 0000000..49eaa99 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/ConverterFormattersTest.kt @@ -0,0 +1,152 @@ +package org.libremediaconverter.convert + +import org.junit.Assert.assertEquals +import org.junit.Test +import org.libremediaconverter.model.AudioCodec +import org.libremediaconverter.model.Container +import org.libremediaconverter.model.EnginePreference +import org.libremediaconverter.model.OutputSpec +import org.libremediaconverter.model.VideoCodec + +/** + * The four pure helpers behind the converter screen's prose, pinned at the points where they + * change what they say. + * + * No Compose rule and no Robolectric: these are `String` in, `String` out, and running them under a + * device sandbox would buy nothing while hiding the boundaries in a rendered tree. + * + * The defect each group bites on: + * + * - **[formatBytes] picks a unit by comparing against three thresholds.** Every one of them is a + * `>=`, and a `>` would move a file sitting exactly on a boundary into the unit below -- `1 GB` + * shown as `1000.0 MB`. Only a value *on* the threshold can tell the two apart, so each of the + * three is asserted at the boundary and one below it. The unit prefixes are decimal, matching + * what the file manager and the provider report, not powers of two. + * - **[formatDuration] has no hours field.** An hour-long recording reads `60:00`, and that is the + * contract rather than an oversight -- the row is a length, not a clock. Pinned so that adding + * hours is a deliberate change with a red test in front of it instead of a silent reformat. + * - **[describe] builds the suggestion-chip label out of up to three parts**, and the parts are + * conditional: [VideoCodec.NONE] and [AudioCodec.NONE] drop out entirely, so an image output + * with neither track has to render as the container alone rather than as a container followed + * by a dangling separator. + * - **[EnginePreference] carries no `label` property**, unlike every other enum the screen + * renders; its three display strings live in a `when` in the screen file. Adding a constant is + * caught by the compiler because that `when` is exhaustive, but nothing stops two constants + * being given the same string, which is what the distinctness assertion is for. + */ +class ConverterFormattersTest { + + @Test + fun `bytes below a kilobyte are counted exactly`() { + assertEquals("0 B", formatBytes(0)) + assertEquals("1 B", formatBytes(1)) + assertEquals("999 B", formatBytes(999)) + } + + @Test + fun `each unit starts exactly on its threshold rather than one byte past it`() { + assertEquals("1 kB", formatBytes(1_000)) + assertEquals("1.0 MB", formatBytes(1_000_000)) + assertEquals("1.0 GB", formatBytes(1_000_000_000)) + } + + /** + * One byte below each threshold, which is the half a `>=` to `>` change leaves alone. Both + * halves are needed: the boundary values alone would still pass if the comparison let + * everything through. + */ + @Test + fun `a value just below a threshold stays in the smaller unit`() { + assertEquals("999 B", formatBytes(999)) + assertEquals("1000 kB", formatBytes(999_999)) + assertEquals("1000.0 MB", formatBytes(999_999_999)) + } + + @Test + fun `a real file size reads as one decimal place`() { + assertEquals("12.3 MB", formatBytes(12_345_678)) + assertEquals("1.5 GB", formatBytes(1_500_000_000)) + } + + @Test + fun `a duration is minutes and zero-padded seconds`() { + assertEquals("0:00", formatDuration(0)) + assertEquals("0:01", formatDuration(1_000)) + assertEquals("0:59", formatDuration(59_000)) + assertEquals("1:00", formatDuration(60_000)) + assertEquals("1:30", formatDuration(90_000)) + } + + /** Sub-second remainders are dropped rather than rounded up into the next second. */ + @Test + fun `a partial second does not become a whole one`() { + assertEquals("0:00", formatDuration(999)) + assertEquals("0:59", formatDuration(59_999)) + } + + /** No hours field, deliberately: an hour is `60:00` and two hours are `120:00`. */ + @Test + fun `an hour and beyond keeps counting in minutes`() { + assertEquals("60:00", formatDuration(3_600_000)) + assertEquals("61:01", formatDuration(3_661_000)) + assertEquals("120:00", formatDuration(7_200_000)) + } + + @Test + fun `a spec with both tracks names the container and joins the two codecs`() { + assertEquals( + "MP4 · H.264 + AAC", + describe(OutputSpec(Container.MP4, VideoCodec.H264, AudioCodec.AAC)), + ) + } + + @Test + fun `a track set to none is left out instead of being named none`() { + assertEquals( + "MP3 · MP3", + describe(OutputSpec(Container.MP3, VideoCodec.NONE, AudioCodec.MP3)), + ) + assertEquals( + "MP4 · H.264", + describe(OutputSpec(Container.MP4, VideoCodec.H264, AudioCodec.NONE)), + ) + } + + /** An image output has neither track, so there is nothing for the separator to separate. */ + @Test + fun `a spec with no tracks at all is the container alone, with no trailing separator`() { + assertEquals("GIF", describe(OutputSpec(Container.GIF, VideoCodec.NONE, AudioCodec.NONE))) + assertEquals( + "PNG frames", + describe(OutputSpec(Container.IMAGE_SEQUENCE, VideoCodec.NONE, AudioCodec.NONE)), + ) + } + + /** `Copy` is a codec here, not the absence of one, so a remux describes both tracks. */ + @Test + fun `a remux names copy on both tracks rather than dropping them`() { + assertEquals( + "Matroska · Copy + Copy", + describe(OutputSpec(Container.MKV, VideoCodec.COPY, AudioCodec.COPY)), + ) + } + + @Test + fun `each engine preference has the wording the chips show`() { + assertEquals("Automatic", EnginePreference.AUTO.label()) + assertEquals("Prefer hardware", EnginePreference.PREFER_HARDWARE.label()) + assertEquals("Force software", EnginePreference.FORCE_SOFTWARE.label()) + } + + /** + * Two constants sharing a label would render as two identical chips, one of which the user + * could not choose deliberately. The exhaustive `when` cannot catch that; this does. + */ + @Test + fun `no two engine preferences render the same chip`() { + val labels = EnginePreference.entries.map { it.label() } + + assertEquals(EnginePreference.entries.size, labels.toSet().size) + assertEquals(emptyList(), labels.filter { it.isBlank() }) + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/FileCardTest.kt b/app/src/test/java/org/libremediaconverter/convert/FileCardTest.kt new file mode 100644 index 0000000..703f932 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/FileCardTest.kt @@ -0,0 +1,254 @@ +package org.libremediaconverter.convert + +import android.net.Uri +import androidx.compose.ui.test.assertCountEquals +import androidx.compose.ui.test.assertTextEquals +import androidx.compose.ui.test.onChildren +import androidx.compose.ui.test.onNodeWithTag +import androidx.media3.common.util.UnstableApi +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.createDrainedComposeRule +import org.libremediaconverter.model.AudioCodec +import org.libremediaconverter.model.Container +import org.libremediaconverter.model.InputKind +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.model.VideoCodec +import org.libremediaconverter.ui.TestTags +import org.robolectric.RobolectricTestRunner + +/** + * What the source-info card says when it does not know something. + * + * The defect is a card that invents an answer instead of admitting it has none. Two of them are + * live here and neither had a test before this file: + * + * - **`InputFile.sizeBytes` is nullable and the card is the reader that has to say so in words.** + * `sizeBytes` used to be `0L` for "nobody told me", and [UnknownInputSizeTest] records what that + * cost at the space check. The card is the other reader, and its failure mode is the mirror + * image: hand the null to `formatBytes` and it renders `"0 B"` -- a measurement, shown to the + * user, that no provider ever made. It renders **independently of the probe**, which is why the + * same assertion appears twice below, with the probe present and absent. That independence is + * the contract; a test covering only the probed case would leave the branch a user actually hits + * first -- the card is on screen before the probe finishes -- unguarded. + * - **The codec rows degrade in words too.** `CodecNames.describeVideo`/`describeAudio` answer + * `"Unknown"` for a codec nothing named, the `VIDEO` branch answers `"No audio track"` for a file + * with no audio, and the two `> 0` guards drop the dimension and length rows rather than printing + * `0` and `0:00`. Each of those has a case below on **both** sides of the guard, because a test + * of the present side alone stays green with the guard deleted. + * + * ### What cannot be asserted here, so that it is a decision rather than an omission + * + * The `probe == null` branch exits before `HorizontalDivider`, and **the divider's absence is not + * observable from a test**: Material 3 renders it as a `Box` with no semantics modifier, so it + * contributes no node to the semantics tree at all. What is asserted instead is everything the + * divider precedes -- no detail row for any label the four kind branches can emit -- plus the + * card's child count, which pins "these three texts and nothing else" without having to enumerate. + * + * The early exit itself is enforced by the compiler rather than by this file, which the PR body + * records: deleting `return@Column` un-smart-casts `probe`, and the `probe.kind` below it stops + * compiling. The mutation that reddens the test here is the compilable form of that regression -- + * defaulting the null away with `?: InputProbe()` and letting the kind rows render. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class FileCardTest { + + @get:Rule + val composeRule = createDrainedComposeRule() + + @Test + fun `a file no provider could measure says so in words rather than showing a zero`() { + setFileCard(input(sizeBytes = null, probe = VIDEO_PROBE)) + + composeRule.onNodeWithTag(TestTags.Converter.FILE_CARD_BYTES) + .assertTextEquals("Size unknown") + } + + /** + * The same line, with no probe at all. Separate from the case above rather than folded into + * it because `setContent` may only be called once per rule, and because two independent reds + * are the evidence that the size line does not depend on the probe. + */ + @Test + fun `the size line says the same thing while the probe is still running`() { + setFileCard(input(sizeBytes = null, probe = null)) + + composeRule.onNodeWithTag(TestTags.Converter.FILE_CARD_BYTES) + .assertTextEquals("Size unknown") + } + + @Test + fun `a size that was reported is formatted rather than replaced by the unknown line`() { + setFileCard(input(sizeBytes = 12_345_678L, probe = VIDEO_PROBE)) + + composeRule.onNodeWithTag(TestTags.Converter.FILE_CARD_NAME).assertTextEquals("clip.mkv") + composeRule.onNodeWithTag(TestTags.Converter.FILE_CARD_BYTES).assertTextEquals("12.3 MB") + } + + /** + * The note and the emptiness are one behaviour, so they are one test: a regression that keeps + * the note but renders the rows anyway would leave a note-only test green. + */ + @Test + fun `while the probe is still running the card shows the reading note and nothing else`() { + setFileCard(input(probe = null)) + + composeRule.onNodeWithTag(TestTags.Converter.FILE_CARD_NOTE) + .assertTextEquals("Reading…") + assertNoDetailRows() + // Name, size, note. Catches a row whose label is not in EVERY_ROW_LABEL as well. + composeRule.onNodeWithTag(TestTags.Converter.FILE_CARD).onChildren().assertCountEquals(3) + } + + @Test + fun `a file nothing could read gets the explanatory line instead of unknown codecs`() { + setFileCard(input(probe = InputProbe(kind = InputKind.UNPARSEABLE))) + + composeRule.onNodeWithTag(TestTags.Converter.FILE_CARD_NOTE) + .assertTextEquals("Could not identify this file. It will be converted with FFmpeg.") + assertNoDetailRows() + } + + @Test + fun `an image gets its type and its pixel dimensions`() { + setFileCard(input(probe = InputProbe(kind = InputKind.IMAGE, width = 1920, height = 1080))) + + assertRow("Type", "Image") + assertRow("Size", "1920×1080") + } + + /** The `width > 0` guard, from the side that would print `0×0` if it were dropped. */ + @Test + fun `an image whose dimensions nothing reported gets the type row alone`() { + setFileCard(input(probe = InputProbe(kind = InputKind.IMAGE))) + + assertRow("Type", "Image") + assertNoRow("Size") + } + + @Test + fun `an audio-only file says it has no video track rather than leaving the row blank`() { + setFileCard( + input( + probe = InputProbe( + audioCodec = "aac", + hasVideo = false, + durationMs = 90_000, + kind = InputKind.AUDIO_ONLY, + container = Container.MP3, + ), + ), + ) + + assertRow("Container", Container.MP3.label) + assertRow("Video", "No video track") + assertRow("Audio", AudioCodec.AAC.label) + assertRow("Length", "1:30") + assertNoRow("Type") + assertNoRow("Size") + } + + /** + * Everything the audio branch can fail to know, at once: no container, no codec name, no + * duration. Each degrades in its own words, and the length row disappears rather than + * claiming `0:00`. + */ + @Test + fun `an audio-only file nothing else could describe degrades one row at a time`() { + setFileCard(input(probe = InputProbe(hasVideo = false, kind = InputKind.AUDIO_ONLY))) + + assertRow("Container", "Unknown") + assertRow("Video", "No video track") + assertRow("Audio", "Unknown") + assertNoRow("Length") + } + + @Test + fun `a video file composes its codec with its dimensions on one row`() { + setFileCard(input(probe = VIDEO_PROBE)) + + assertRow("Container", Container.MP4.label) + assertRow("Video", "${VideoCodec.H264.label} · 1920×1080") + assertRow("Audio", AudioCodec.AAC.label) + assertRow("Length", "1:30") + } + + /** + * `"No audio track"` rather than `describeAudio(null)`'s `"Unknown"`. The video branch knows + * the difference between a track it could not name and a track that is not there; the audio + * branch above cannot, because a file with no audio is not audio-only. + */ + @Test + fun `a video file with no audio track says so instead of naming an unknown codec`() { + setFileCard(input(probe = VIDEO_PROBE.copy(audioCodec = null))) + + assertRow("Audio", "No audio track") + } + + /** Both `> 0` guards on the video branch, plus the codec name nothing supplied. */ + @Test + fun `a video file missing its codec, dimensions and duration omits them rather than faking them`() { + setFileCard( + input( + probe = VIDEO_PROBE.copy( + videoCodec = null, + width = 0, + height = 0, + durationMs = 0, + ), + ), + ) + + assertRow("Video", "Unknown") + assertNoRow("Length") + } + + /** + * The row is one node, not a label node beside a value node. A test matching on `"Container"` + * alone would pass against either shape. + */ + @Test + fun `a detail row renders its label and its value as a single node`() { + composeRule.setContent { DetailRow("Container", "Matroska") } + + composeRule.onNodeWithTag(TestTags.Converter.detailRow("Container")) + .assertTextEquals("Container: Matroska") + } + + private fun setFileCard(input: InputFile) = composeRule.setContent { FileCard(input) } + + private fun input(sizeBytes: Long? = 12_345_678L, probe: InputProbe? = VIDEO_PROBE) = InputFile( + uri = Uri.parse("content://test/clip.mkv"), + displayName = "clip.mkv", + sizeBytes = sizeBytes, + probe = probe, + ) + + private fun assertRow(label: String, value: String) { + composeRule.onNodeWithTag(TestTags.Converter.detailRow(label)) + .assertTextEquals("$label: $value") + } + + private fun assertNoRow(label: String) { + composeRule.onNodeWithTag(TestTags.Converter.detailRow(label)).assertDoesNotExist() + } + + private fun assertNoDetailRows() = EVERY_ROW_LABEL.forEach(::assertNoRow) + + private companion object { + /** Every label the four kind branches can emit, so absence can be asserted exhaustively. */ + val EVERY_ROW_LABEL = listOf("Container", "Video", "Audio", "Length", "Type", "Size") + + val VIDEO_PROBE = InputProbe( + videoCodec = "h264", + audioCodec = "aac", + durationMs = 90_000, + kind = InputKind.VIDEO, + container = Container.MP4, + width = 1920, + height = 1080, + ) + } +} -- 2.47.3