diff --git a/.github/workflows/status_check.yml b/.github/workflows/status_check.yml index f650df4..1c95fff 100644 --- a/.github/workflows/status_check.yml +++ b/.github/workflows/status_check.yml @@ -156,7 +156,37 @@ jobs: key: gradle-${{ runner.os }}-${{ hashFiles('**/*.gradle.kts', 'gradle/libs.versions.toml', 'gradle/wrapper/gradle-wrapper.properties') }} restore-keys: gradle-${{ runner.os }}- + # Shell is the other language in this repo -- four scripts, one of them the CI + # entry point itself -- and nothing was checking it. `git ls-files` rather than a + # fixed list, so a script added later is covered without editing this workflow. + # + # Full severity, `info` included. The findings it raises today are answered with + # targeted `disable` directives carrying their reason, the same way + # config/detekt/detekt.yml carries only the rules this codebase legitimately + # breaks. A blanket --severity=warning would have hidden them and the next real + # one alike. + # + # PINNED BY DIGEST, for the reason CLAUDE.md already gives for pinning ktlint, + # detekt and JaCoCo: a new rule in a linter makes files nobody touched stop + # passing, so CI goes red on a PR whose diff cannot explain it. That is not + # hypothetical here. The first cut of this step used the runner's ambient + # shellcheck, which is 0.9.0, and 0.9.0 reports a trap handler as seven + # unreachable commands (SC2317) where 0.11.0 reports it once on the declaration + # (SC2329) -- same script, same directive, different answer, and a red build on + # the PR that introduced the step. The version is printed so a finding that + # appears out of nowhere can be tied to a bump of this line. + - name: shellcheck + env: + SHELLCHECK: koalaman/shellcheck@sha256:61862eba1fcf09a484ebcc6feea46f1782532571a34ed51fedf90dd25f925a8d + run: | + docker run --rm "$SHELLCHECK" --version + git ls-files -z '*.sh' | xargs -0 -r docker run --rm -v "$PWD:/mnt" "$SHELLCHECK" + + # `!cancelled()` rather than a plain sequence: a shellcheck failure above must not + # cost the ktlint/detekt/lint lists. Same reason this step passes --continue -- one + # round trip should produce every list, not stop at the first. - name: ktlint, detekt and Android lint + if: '!cancelled()' run: ./gradlew :app:ktlintCheck :app:detekt :app:lintDebug --continue --stacktrace # The XML matters as much as the HTML: it is the one that can be diffed between diff --git a/CLAUDE.md b/CLAUDE.md index af6e7b0..33d0ff9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -123,6 +123,42 @@ install for code that can never run — and on API 37 the full APK does not fit - `kotlin.code.style=official`. Gradle stays Kotlin DSL. +- **File one-off issues with `tools/github/file-issue.sh`, not `gh issue create`.** `gh issue + create` does not touch the project board, so the issue exists, carries its labels, and is + invisible in the Kanban — indistinguishable from never having been filed. Measured 2026-08-24: + eight issues filed as a scripted batch all reached the board; one filed as a one-off minutes + later did not. A batch carries the board step in its loop; **one-offs are where it slips**, which + is what the script is for. It resolves the project and Status ids by name rather than caching + them, and it **reads the item back** — a mutation returning 200 is not evidence the board shows + what was asked for. Exit 3 means the issue was created but did not reach the board, and prints + the number so it cannot be lost quietly. + + `above-cut` and `backlog` are **labels from the 2026-08-22 triage pass** — "worked autonomously + overnight" and "held for manual review". They are not board columns. Status carries board state; + do not put a cut label on a newly filed ticket. + +- **shellcheck runs in CI**, inside the Static analysis job, over `git ls-files '*.sh'` so a new + script is covered without editing the workflow. It runs at full severity, `info` included: the + two findings that raises today are answered with targeted `disable` directives carrying their + reason, exactly as `config/detekt/detekt.yml` carries only the rules this codebase legitimately + breaks. Do not silence it with `--severity=warning` — that hides the next real finding too. + **It is pinned by image digest, and joins ktlint/detekt/JaCoCo in the "Dependency versions" + rule above** — for exactly the reason stated there, demonstrated the day it was added. The first + cut used the runner's ambient shellcheck. That is **0.9.0**, while the container used to check + locally was 0.11.0, and the two disagree about how to report a trap handler: 0.11.0 says + `SC2329` once on the declaration, 0.9.0 says `SC2317` on each of seven lines in the body. Same + script, same directive, one green and one red. Directives that must survive both name both codes. + + Locally, use the same pin rather than whatever is installed: + `podman run --rm -v "$PWD:/mnt:z" docker.io/koalaman/shellcheck@sha256:61862eba... ` + (the digest is in `status_check.yml`; there is no shellcheck system package on this host). + + **It does not cover inline `run:` blocks in the workflows**, and a good deal of this repo's bash + lives there. `actionlint` does cover them — it runs shellcheck over each `run:` — and reports one + pre-existing `info` finding in `build.yml`. It is not wired in because every action here is + pinned by SHA, and actionlint's usual installer is a `curl | bash` off a moving branch; doing it + properly means pinning a container digest. Tracked separately rather than bolted on. + ## Dependency versions Libraries **float on minor + patch** (`coreKtx = "1.+"`). Three groups deliberately do not: diff --git a/app/src/test/java/org/libremediaconverter/convert/AdvancedPanelSavedStateTest.kt b/app/src/test/java/org/libremediaconverter/convert/AdvancedPanelSavedStateTest.kt new file mode 100644 index 0000000..4961381 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/AdvancedPanelSavedStateTest.kt @@ -0,0 +1,159 @@ +package org.libremediaconverter.convert + +import android.os.Bundle +import android.os.Parcel +import android.os.Parcelable +import androidx.compose.runtime.CompositionLocalProvider +import androidx.compose.runtime.MutableState +import androidx.compose.runtime.saveable.LocalSaveableStateRegistry +import androidx.compose.runtime.saveable.SaveableStateRegistry +import androidx.compose.ui.test.onNodeWithTag +import androidx.compose.ui.test.performClick +import androidx.media3.common.util.UnstableApi +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +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.OutputSpec +import org.libremediaconverter.model.Validation +import org.libremediaconverter.model.VideoCodec +import org.libremediaconverter.ui.TestTags +import org.robolectric.RobolectricTestRunner + +/** + * What `AdvancedPickerTest`'s restoration test cannot see. + * + * `StateRestorationTester` saves into an **in-memory map**, never a `Bundle`. That is enough to + * discriminate `rememberSaveable` from `remember`, and it is where it stops: the map holds object + * references, so a value the platform could never parcel goes in and comes back out looking green. + * `AppRootRestorationTest` has the same blind spot and `DestinationSaverTest` is the split it + * prompted; this is that split for `expanded`, the only `rememberSaveable` on either screen's + * leaves. + * + * ### The saved representation is not the Boolean + * + * `var expanded by rememberSaveable { mutableStateOf(false) }` passes no `stateSaver`, so + * `autoSaver` saves **the `MutableState` itself**, not the `false` inside it. That works only + * because `mutableStateOf` on Android returns a `Parcelable` implementation -- the same call on a + * plain JVM returns one that is not. So what stands between an open panel and a rotation that + * closes it is a platform-specific detail of a factory function nothing here names directly, and + * an in-memory map cannot tell the two apart. + * + * Pinning it is the move `DestinationSaverTest` makes about names versus ordinals. Passing an + * explicit `stateSaver` would save a bare `Boolean` instead and is a perfectly reasonable edit -- + * it is just not the one in the tree, and it should be made on purpose rather than discovered + * after a rotation. + * + * ### Shared bite, stated rather than implied + * + * `rememberSaveable` -> `remember` empties the registry, so it reddens this file *and* the + * restoration test in `AdvancedPickerTest`. Both failures belong in any report of that mutation. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class AdvancedPanelSavedStateTest { + + // Not `createComposeRule()` directly: see [drainEscapedCoroutineErrors]. + @get:Rule + val composeRule = createDrainedComposeRule() + + /** + * `canBeSaved = { true }` deliberately. + * + * A predicate mirroring what a `Bundle` accepts would be a hand-written copy of the thing + * under test, and a `false` from it *drops* the entry silently -- so the test would fail by + * finding nothing saved, which is also how a `remember` regression fails. Two causes, one + * symptom, is not a test. The type is checked on the way out instead. + */ + private val registry = SaveableStateRegistry(restoredValues = null, canBeSaved = { true }) + + @Test + fun `the panel registers its open state with the registry, and nothing else`() { + setPicker() + + // Collapsed is a saved value, not an absent one: `rememberSaveable` registers its provider + // on first composition, whatever the state happens to be. Exactly one, because `expanded` + // is the only saveable in the subtree -- a second would mean something else began saving. + assertEquals(1, savedValues().size) + + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_TOGGLE).performClick() + + val saved = theOneSavedValue() + assertTrue("saved as ${saved?.javaClass?.name}", saved is MutableState<*>) + assertEquals(true, (saved as MutableState<*>).value) + } + + @Test + fun `the open panel survives a real Parcel, not just an in-memory map`() { + setPicker() + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_TOGGLE).performClick() + + val saved = theOneSavedValue() + + // The claim the restoration test cannot make. A `MutableState` that was not `Parcelable` + // would satisfy `StateRestorationTester` and then be dropped by the platform. + assertTrue("saved as ${saved?.javaClass?.name}", saved is Parcelable) + + val restored = throughARealBundle(saved as Parcelable) + + assertTrue("restored as ${restored.javaClass.name}", restored is MutableState<*>) + assertEquals(true, (restored as MutableState<*>).value) + } + + private fun setPicker() { + composeRule.setContent { + CompositionLocalProvider(LocalSaveableStateRegistry provides registry) { + AdvancedPicker( + spec = OutputSpec(Container.MP4, VideoCodec.H264, AudioCodec.AAC), + validation = Validation.Valid, + onContainer = {}, + onVideoCodec = {}, + onAudioCodec = {}, + onSuggestion = {}, + ) + } + } + } + + /** Every value the picker hands the host to persist, keys dropped -- they are positional. */ + private fun savedValues(): List = composeRule.runOnIdle { registry.performSave().values.flatten() } + + /** + * The single saved value, asserted rather than assumed. + * + * `single()` on an empty list throws `NoSuchElementException: List is empty`, which names + * neither the panel nor the registry -- and an empty registry is exactly how the + * `rememberSaveable` -> `remember` regression shows up here. + */ + private fun theOneSavedValue(): Any? { + val values = savedValues() + assertEquals("the panel should register exactly one saved value", 1, values.size) + return values.first() + } + + /** A write and a read through a real `Parcel`, which is what the tester's map stands in for. */ + private fun throughARealBundle(value: Parcelable): Parcelable { + val bundle = Bundle().apply { putParcelable(KEY, value) } + val parcel = Parcel.obtain() + return try { + parcel.writeBundle(bundle) + parcel.setDataPosition(0) + val restored = requireNotNull(parcel.readBundle(javaClass.classLoader)) { + "the Bundle did not survive the Parcel" + } + requireNotNull(restored.getParcelable(KEY, Parcelable::class.java)) { + "the saved state did not survive the Parcel" + } + } finally { + parcel.recycle() + } + } + + private companion object { + const val KEY = "expanded" + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/AdvancedPickerTest.kt b/app/src/test/java/org/libremediaconverter/convert/AdvancedPickerTest.kt new file mode 100644 index 0000000..342b54e --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/AdvancedPickerTest.kt @@ -0,0 +1,321 @@ +package org.libremediaconverter.convert + +import androidx.compose.ui.test.assertIsDisplayed +import androidx.compose.ui.test.assertTextEquals +import androidx.compose.ui.test.hasAnyAncestor +import androidx.compose.ui.test.hasTestTag +import androidx.compose.ui.test.hasText +import androidx.compose.ui.test.junit4.StateRestorationTester +import androidx.compose.ui.test.onAllNodesWithTag +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 org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +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.ContainerCapabilities +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.model.OutputSpec +import org.libremediaconverter.model.Validation +import org.libremediaconverter.model.VideoCodec +import org.libremediaconverter.ui.TestTags +import org.robolectric.RobolectricTestRunner + +/** + * The gate over the Advanced chips, and the error card that deliberately sits outside it. + * + * Two defects, and they pull in opposite directions. + * + * The first is the chips escaping the gate, or never being reachable through it. `AdvancedPicker` + * is the one leaf on this screen that is not stateless -- `expanded` is its own `rememberSaveable` + * -- and Container, Video and Audio live inside `AnimatedVisibility(visible = expanded)`. Nothing + * else on the screen hides anything, so a refactor that flattened the panel, or wired the toggle to + * a state nobody reads, would render an app that looks reasonable in a screenshot and is wrong. + * + * The second is the opposite mistake, and it is the one this file exists for: **moving the + * `ValidationError` call inside the `AnimatedVisibility`**. It is invoked after that block, so an + * invalid spec explains itself and offers one-tap fixes *while the section is collapsed*. That is + * the only route out of an invalid spec for a user who never opened Advanced -- and since the only + * way to reach an invalid spec is through Advanced, hiding the way out behind the same toggle looks + * locally sensible and is a trap. Tidying the two `if` blocks into one is a plausible edit, it + * compiles, and until this file existed nothing went red. Every assertion about the error card here + * therefore runs with the toggle untouched, and asserts the panel is absent in the same test, so a + * future `expanded = true` default cannot quietly satisfy it either. + * + * The invalid specs come from [ContainerCapabilities.validate] rather than from a hand-built + * [Validation.Invalid], so the messages and the suggestions are the real pairing. A hand-built one + * would keep passing after `validate` stopped producing anything like it. + * + * Node location is by the three separate chip-row tags, never by text. `"Copy"` and `"None"` are + * each both a [VideoCodec] and an [AudioCodec], and `"MP3"` and `"FLAC"` are each both a + * [Container] and an [AudioCodec], so a text matcher over the open panel is ambiguous for four + * chips -- which is what the separate tags are for. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class AdvancedPickerTest { + + // Not `createComposeRule()` directly: see [drainEscapedCoroutineErrors]. + @get:Rule + val composeRule = createDrainedComposeRule() + + private val restoration = StateRestorationTester(composeRule) + + private val containers = mutableListOf() + private val videoCodecs = mutableListOf() + private val audioCodecs = mutableListOf() + private val applied = mutableListOf() + + // --- the expand gate ---------------------------------------------------- + + @Test + fun `the three chip rows appear only while the panel is expanded`() { + setPicker() + + assertPanelHidden() + + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_TOGGLE).performClick() + + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_PANEL).assertExists() + ROW_TAGS.forEach { composeRule.onNodeWithTag(it).assertExists() } + + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_TOGGLE).performClick() + + // The exit transition outlives the click, so absence has to be waited for rather than + // asserted straight away -- unlike the initial collapsed state, which has no animation + // in flight. + composeRule.waitUntil { nodeCount(TestTags.Converter.ADVANCED_PANEL) == 0 } + assertPanelHidden() + } + + /** The toggle is the only affordance the collapsed picker offers, so it has to say so. */ + @Test + fun `the toggle names the direction it will move in`() { + setPicker() + + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_TOGGLE).assertTextEquals("Advanced") + + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_TOGGLE).performClick() + + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_TOGGLE) + .assertTextEquals("Hide advanced") + } + + /** + * The four colliding labels, one per row. + * + * `"Copy"` is a video codec *and* an audio codec; `"MP3"` is a container *and* an audio codec. + * Clicking each through its own row is what proves the rows are wired to different callbacks + * -- a picker that handed every chip to `onAudioCodec` would look identical on screen. + */ + @Test + fun `each chip row reports to its own callback, including the labels that collide`() { + setPicker() + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_TOGGLE).performClick() + + chipIn(TestTags.Converter.ADVANCED_VIDEO_CHIPS, "Copy").performClick() + + assertEquals(listOf(VideoCodec.COPY), videoCodecs) + assertEquals(emptyList(), audioCodecs) + + chipIn(TestTags.Converter.ADVANCED_AUDIO_CHIPS, "Copy").performClick() + + assertEquals(listOf(AudioCodec.COPY), audioCodecs) + + chipIn(TestTags.Converter.ADVANCED_CONTAINER_CHIPS, "MP3").performClick() + + assertEquals(listOf(Container.MP3), containers) + // Still only the one audio click. `MP3` is an AudioCodec label too, and the container row + // must not be reporting through that callback. + assertEquals(listOf(AudioCodec.COPY), audioCodecs) + } + + // --- the error card, which is outside the gate -------------------------- + + /** + * The headline case. Dropping both tracks is reachable from the collapsed screen -- the + * `None`/`None` pair is set inside Advanced, but the user can close it again -- and the + * explanation has to still be there. + */ + @Test + fun `an empty output explains itself while the section is collapsed`() { + val spec = OutputSpec(Container.MP4, VideoCodec.NONE, AudioCodec.NONE) + val invalid = invalidFor(spec) + + assertEquals("This would produce an empty file — keep at least one track.", invalid.message) + + setPicker(spec, invalid) + + assertPanelHidden() + composeRule.onNodeWithTag(TestTags.Converter.VALIDATION_ERROR).assertExists() + composeRule.onNodeWithText(invalid.message).assertIsDisplayed() + } + + @Test + fun `a codec the container cannot hold explains itself while the section is collapsed`() { + val spec = OutputSpec(Container.WEBM, VideoCodec.H264, AudioCodec.OPUS) + val invalid = invalidFor(spec) + + assertEquals("WebM cannot hold H.264 video.", invalid.message) + + setPicker(spec, invalid) + + assertPanelHidden() + composeRule.onNodeWithText(invalid.message).assertIsDisplayed() + } + + /** + * Clicking a suggestion, with the toggle never touched. + * + * The second suggestion rather than the first, and its count pinned first: with one suggestion + * a picker that handed every chip `suggestions[0]` would pass, and `onNodeWithTag` on a + * suggestion index that no longer exists reports an unhelpful matcher failure rather than + * saying the list shrank. + */ + @Test + fun `a suggestion chip applies its own spec without the section ever being opened`() { + val spec = OutputSpec(Container.WEBM, VideoCodec.H264, AudioCodec.OPUS) + val invalid = invalidFor(spec) + + assertEquals(2, invalid.suggestions.size) + val second = invalid.suggestions[1] + + setPicker(spec, invalid) + + assertPanelHidden() + composeRule.onNodeWithTag(TestTags.Converter.suggestion(1)).assertTextEquals(describe(second)) + composeRule.onNodeWithTag(TestTags.Converter.suggestion(1)).performClick() + + assertEquals(listOf(second), applied) + // What the chips offer is what `validate` said would work, not a repair of the test's own. + assertTrue( + "suggestion $second should itself validate", + ContainerCapabilities.validate(second, PROBE).isValid, + ) + } + + /** A valid spec has nothing to say, collapsed or not. */ + @Test + fun `a valid spec renders no error card`() { + setPicker() + + composeRule.onNodeWithTag(TestTags.Converter.VALIDATION_ERROR).assertDoesNotExist() + + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_TOGGLE).performClick() + + composeRule.onNodeWithTag(TestTags.Converter.VALIDATION_ERROR).assertDoesNotExist() + } + + // --- recreation --------------------------------------------------------- + + /** + * `expanded` is the only `rememberSaveable` on either screen's leaves. + * + * `MainActivity` declares no `configChanges`, so a rotation destroys and rebuilds the whole + * composition. A panel the user opened, set three chips in, and left open must not close + * itself on the way back. `remember` would. + * + * What this cannot see is the saved *representation* -- `StateRestorationTester` saves into an + * in-memory map rather than a `Bundle`. `AdvancedPanelSavedStateTest` covers that half. + */ + @Test + fun `an open panel is still open after recreation`() { + restoration.setContent { + AdvancedPicker( + spec = VALID_SPEC, + validation = Validation.Valid, + onContainer = {}, + onVideoCodec = {}, + onAudioCodec = {}, + onSuggestion = {}, + ) + } + + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_TOGGLE).performClick() + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_PANEL).assertExists() + + restoration.emulateSavedInstanceStateRestore() + + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_PANEL).assertExists() + ROW_TAGS.forEach { composeRule.onNodeWithTag(it).assertExists() } + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_TOGGLE) + .assertTextEquals("Hide advanced") + } + + /** The default has to survive too, or the panel would spring open on every rotation. */ + @Test + fun `a collapsed panel is still collapsed after recreation`() { + restoration.setContent { + AdvancedPicker( + spec = VALID_SPEC, + validation = Validation.Valid, + onContainer = {}, + onVideoCodec = {}, + onAudioCodec = {}, + onSuggestion = {}, + ) + } + + assertPanelHidden() + + restoration.emulateSavedInstanceStateRestore() + + assertPanelHidden() + } + + // --- helpers ------------------------------------------------------------ + + private fun setPicker(spec: OutputSpec = VALID_SPEC, validation: Validation = Validation.Valid) { + composeRule.setContent { + AdvancedPicker( + spec = spec, + validation = validation, + onContainer = { containers += it }, + onVideoCodec = { videoCodecs += it }, + onAudioCodec = { audioCodecs += it }, + onSuggestion = { applied += it }, + ) + } + } + + /** The whole panel, by every tag it owns, so a partial escape counts as a failure. */ + private fun assertPanelHidden() { + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_PANEL).assertDoesNotExist() + ROW_TAGS.forEach { composeRule.onNodeWithTag(it).assertDoesNotExist() } + } + + private fun nodeCount(tag: String) = composeRule.onAllNodesWithTag(tag).fetchSemanticsNodes().size + + private fun chipIn(rowTag: String, label: String) = + composeRule.onNode(hasText(label) and hasAnyAncestor(hasTestTag(rowTag))) + + private fun invalidFor(spec: OutputSpec): Validation.Invalid { + val validation = ContainerCapabilities.validate(spec, PROBE) + return validation as? Validation.Invalid + ?: throw AssertionError("$spec was expected to be invalid, but validate said $validation") + } + + private companion object { + val ROW_TAGS = listOf( + TestTags.Converter.ADVANCED_CONTAINER_CHIPS, + TestTags.Converter.ADVANCED_VIDEO_CHIPS, + TestTags.Converter.ADVANCED_AUDIO_CHIPS, + ) + + val VALID_SPEC = OutputSpec(Container.MP4, VideoCodec.H264, AudioCodec.AAC) + + /** An ordinary H.264/AAC MP4, so the suggestions have a real source to repair towards. */ + val PROBE = InputProbe( + videoCodec = "h264", + audioCodec = "aac", + durationMs = 90_000, + container = Container.MP4, + ) + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/ConverterPickerSelectionTest.kt b/app/src/test/java/org/libremediaconverter/convert/ConverterPickerSelectionTest.kt new file mode 100644 index 0000000..3fdbbb6 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/ConverterPickerSelectionTest.kt @@ -0,0 +1,169 @@ +package org.libremediaconverter.convert + +import androidx.compose.ui.test.SemanticsNodeInteraction +import androidx.compose.ui.test.assertIsNotSelected +import androidx.compose.ui.test.assertIsSelected +import androidx.compose.ui.test.hasAnyAncestor +import androidx.compose.ui.test.hasTestTag +import androidx.compose.ui.test.hasText +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.performClick +import androidx.media3.common.util.UnstableApi +import org.junit.Assert.assertEquals +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.createDrainedComposeRule +import org.libremediaconverter.model.EnginePreference +import org.libremediaconverter.model.OutputFormat +import org.libremediaconverter.model.QualityTier +import org.libremediaconverter.ui.TestTags +import org.robolectric.RobolectricTestRunner + +/** + * Each picker lights the chip it was handed and reports the constant that was pressed. + * + * The defect this bites on is a picker that renders perfectly and answers wrongly. All three are + * the same dozen lines with a different enum substituted, so the failure mode is a copy-paste that + * survives review: an `onClick` that closes over the picker's `selected` parameter instead of the + * chip's own entry hands back one constant no matter which chip was tapped, and an inverted + * `entry == selected` lights every chip except the right one. Neither throws, neither changes the + * set of labels on screen, and a test that only asserted "the callback ran" would pass over both. + * + * Clicking every chip in turn and comparing the whole recorded list against `entries` is what makes + * the constant load-bearing rather than the click count -- a hardcoded `onSelect` fires the same + * number of times as a correct one. Selection is asserted over every chip for the same reason: the + * one that should be lit proves nothing on its own, because `!=` lights it too whenever the enum + * has exactly one entry, and lights all its siblings whenever it has more. + * + * Labels come from `OutputFormat.label` and `QualityTier.label`; [label], which the screen owns + * because `EnginePreference` carries no label of its own, supplies the third set. Retyping any of + * them here would turn a rename into a red test that named the wrong cause. + * + * Not covered, deliberately: the `"Output format"`, `"Quality"` and `"Engine"` headings, which are + * untagged `Text` calls with no enum behind them and no behaviour to bite on. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class ConverterPickerSelectionTest { + + @get:Rule + val composeRule = createDrainedComposeRule() + + /** + * The chip carrying [label] inside the row tagged [rowTag]. + * + * By ancestor rather than by direct child: how many semantics nodes Material 3 puts between a + * `FlowRow` and its chips is that library's business, and a matcher that assumed "one" would + * break on an upgrade that changed nothing this test is about. + */ + private fun chipIn(rowTag: String, label: String): SemanticsNodeInteraction = + composeRule.onNode(hasAnyAncestor(hasTestTag(rowTag)) and hasText(label)) + + private fun assertOnlySelected(rowTag: String, labels: List, selected: String?) { + labels.forEach { label -> + val chip = chipIn(rowTag, label) + if (label == selected) chip.assertIsSelected() else chip.assertIsNotSelected() + } + } + + @Test + fun `the format picker lights the selected format and no other`() { + composeRule.setContent { FormatPicker(OutputFormat.WEBM_VP9) {} } + + assertOnlySelected( + rowTag = TestTags.Converter.FORMAT_CHIPS, + labels = OutputFormat.entries.map { it.label }, + selected = OutputFormat.WEBM_VP9.label, + ) + } + + /** A spec no preset can express lights nothing, which is what the custom line stands in for. */ + @Test + fun `the format picker lights nothing when the spec is custom`() { + composeRule.setContent { FormatPicker(null) {} } + + assertOnlySelected( + rowTag = TestTags.Converter.FORMAT_CHIPS, + labels = OutputFormat.entries.map { it.label }, + selected = null, + ) + composeRule.onNodeWithText(CUSTOM_SPEC_NOTE).assertExists() + } + + @Test + fun `a selected format hides the custom line`() { + composeRule.setContent { FormatPicker(OutputFormat.MP3) {} } + + composeRule.onNodeWithText(CUSTOM_SPEC_NOTE).assertDoesNotExist() + } + + @Test + fun `clicking a format chip reports that format`() { + val picked = mutableListOf() + composeRule.setContent { FormatPicker(null) { picked += it } } + + OutputFormat.entries.forEach { chipIn(TestTags.Converter.FORMAT_CHIPS, it.label).performClick() } + + assertEquals(OutputFormat.entries.toList(), picked) + } + + @Test + fun `the quality picker lights the selected tier and no other`() { + composeRule.setContent { QualityPicker(QualityTier.BEST) {} } + + assertOnlySelected( + rowTag = TestTags.Converter.QUALITY_CHIPS, + labels = QualityTier.entries.map { it.label }, + selected = QualityTier.BEST.label, + ) + } + + /** The line under the chips describes what was chosen, not whichever tier was written first. */ + @Test + fun `the quality picker explains the tier that is selected`() { + composeRule.setContent { QualityPicker(QualityTier.BEST) {} } + + composeRule.onNodeWithText(QualityTier.BEST.description).assertExists() + composeRule.onNodeWithText(QualityTier.FAST.description).assertDoesNotExist() + } + + @Test + fun `clicking a quality chip reports that tier`() { + val picked = mutableListOf() + composeRule.setContent { QualityPicker(QualityTier.FAST) { picked += it } } + + QualityTier.entries.forEach { chipIn(TestTags.Converter.QUALITY_CHIPS, it.label).performClick() } + + assertEquals(QualityTier.entries.toList(), picked) + } + + @Test + fun `the engine picker lights the selected preference and no other`() { + composeRule.setContent { EnginePicker(EnginePreference.FORCE_SOFTWARE) {} } + + assertOnlySelected( + rowTag = TestTags.Converter.ENGINE_CHIPS, + labels = EnginePreference.entries.map { it.label() }, + selected = EnginePreference.FORCE_SOFTWARE.label(), + ) + } + + @Test + fun `clicking an engine chip reports that preference`() { + val picked = mutableListOf() + composeRule.setContent { EnginePicker(EnginePreference.AUTO) { picked += it } } + + EnginePreference.entries.forEach { chipIn(TestTags.Converter.ENGINE_CHIPS, it.label()).performClick() } + + assertEquals(EnginePreference.entries.toList(), picked) + } + + private companion object { + /** + * Copied byte for byte out of `ConverterScreen.kt` -- it holds a U+2014 em dash, which + * retyped as ASCII would match nothing and fail as "no node found" rather than as a reword. + */ + const val CUSTOM_SPEC_NOTE: String = "Custom — set below." + } +} diff --git a/tools/github/file-issue.sh b/tools/github/file-issue.sh new file mode 100755 index 0000000..a48a866 --- /dev/null +++ b/tools/github/file-issue.sh @@ -0,0 +1,253 @@ +#!/usr/bin/env bash +# +# Files a GitHub issue AND puts it on the project board, as one operation. +# +# Usage: tools/github/file-issue.sh --title TITLE (--body TEXT | --body-file PATH) [options] +# +# --status NAME board column, matched case-insensitively against the board's own +# options; a miss lists what is available. Default: Backlog +# --label NAME repeatable. Passed through to `gh issue create` unchanged. +# --project N project number. Default: $ISSUE_PROJECT_NUMBER, else 6 +# --repo OWNER/NAME default: whatever `gh repo view` resolves in the working directory +# --dry-run resolve and validate everything, create nothing +# +# EXIT CODE: 0 only when the issue exists, is on the board, AND reads back carrying the +# Status that was asked for. 2 for a usage or validation error, before anything is created. +# **3 means the issue was created but did not reach the board** -- the number is printed on +# a line of its own, because that combination is the entire failure this script exists to +# prevent and it must never be quiet. +# +# WHY THIS EXISTS +# +# `gh issue create` does not touch the project board. The issue is created, carries its +# labels, and is invisible in the Kanban -- which looks exactly like a ticket nobody filed. +# Measured 2026-08-24: eight issues filed as a scripted batch all reached the board; one +# filed as a one-off a few minutes later did not, and was caught only because someone went +# looking. A batch carries the board step inside its loop. One-offs are where it slips, so +# one-offs are what this is for. +# +# Adding an item and setting a field value are GraphQL-only. REST can list project items +# and field definitions, but the `fields` array it returns on an item carries Title and +# nothing else -- a REST-only check reports every item's Status as unset, which is why the +# read-back at the end is a GraphQL query rather than the cheaper REST one. +# +# WHAT IT DELIBERATELY DOES NOT DO +# +# It does not cache the project, field or option ids. Resolving them by name costs one +# GraphQL query per run, and it means a renamed or reordered column cannot make this write +# a stale id. The ids are the fragile part; the names are what people actually use. +# +# It does not apply triage labels for you. `above-cut` and `backlog` are labels from one +# specific 2026-08-22 triage pass -- they mean "worked autonomously overnight" and "held for +# manual review", not "this is in the Backlog column". Status carries board state. Pass +# --label only for things that are true about the issue itself. +# +# It does not create the project, the Status field, or a missing option. Anything absent is +# an error to report, not to invent. + +set -euo pipefail + +readonly EXIT_USAGE=2 +readonly EXIT_ORPHANED=3 + +die() { + printf 'file-issue: %s\n' "$1" >&2 + exit "${2:-$EXIT_USAGE}" +} + +title="" +body="" +body_file="" +status="Backlog" +project="${ISSUE_PROJECT_NUMBER:-6}" +repo="" +dry_run=0 +labels=() + +while [ $# -gt 0 ]; do + case "$1" in + --title) [ $# -ge 2 ] || die "--title needs a value"; title="$2"; shift 2 ;; + --body) [ $# -ge 2 ] || die "--body needs a value"; body="$2"; shift 2 ;; + --body-file) [ $# -ge 2 ] || die "--body-file needs a path"; body_file="$2"; shift 2 ;; + --status) [ $# -ge 2 ] || die "--status needs a value"; status="$2"; shift 2 ;; + --label) [ $# -ge 2 ] || die "--label needs a value"; labels+=("$2"); shift 2 ;; + --project) [ $# -ge 2 ] || die "--project needs a number"; project="$2"; shift 2 ;; + --repo) [ $# -ge 2 ] || die "--repo needs OWNER/NAME"; repo="$2"; shift 2 ;; + --dry-run) dry_run=1; shift ;; + -h|--help) awk 'NR > 1 && /^#/ { sub(/^# ?/, ""); print; next } NR > 1 { exit }' "$0" + exit 0 ;; + *) die "unknown argument: $1" ;; + esac +done + +[ -n "$title" ] || die "--title is required" +if [ -n "$body" ] && [ -n "$body_file" ]; then + die "pass --body or --body-file, not both" +fi +[ -n "$body" ] || [ -n "$body_file" ] || die "one of --body or --body-file is required" +if [ -n "$body_file" ] && [ ! -r "$body_file" ]; then + die "--body-file is not readable: $body_file" +fi +case "$project" in + ''|*[!0-9]*) die "--project must be a number, got: $project" ;; +esac + +command -v gh >/dev/null 2>&1 || die "gh is not on PATH" + +if [ -z "$repo" ]; then + repo=$(gh repo view --json nameWithOwner --jq '.nameWithOwner') \ + || die "could not resolve the repository; pass --repo OWNER/NAME" +fi +owner="${repo%%/*}" +[ -n "$owner" ] || die "could not read an owner out of: $repo" + +# --------------------------------------------------------------------------- +# Resolve the board by NAME. Every id below is read fresh; none is hardcoded. +# --------------------------------------------------------------------------- + +# The $names in the query are GraphQL variables, declared by the query and bound by the +# -f flags. Expanding them in the shell would send this shell's idea of $owner to the +# API instead of declaring a parameter -- which is why every query here is single-quoted. +# shellcheck disable=SC2016 +board=$(gh api graphql \ + -f query=' + query($owner: String!, $number: Int!) { + user(login: $owner) { + projectV2(number: $number) { + id + title + field(name: "Status") { + ... on ProjectV2SingleSelectField { id options { id name } } + } + } + } + }' \ + -f owner="$owner" -F number="$project" 2>&1) \ + || die "could not read project $project for $owner. A 403 naming scopes means gh is +missing 'project'; a 403 naming a rate limit is the GraphQL budget, not permissions. The +API said: $board" + +project_id=$(printf '%s' "$board" | jq -r '.data.user.projectV2.id // empty') +field_id=$(printf '%s' "$board" | jq -r '.data.user.projectV2.field.id // empty') +project_title=$(printf '%s' "$board" | jq -r '.data.user.projectV2.title // empty') + +[ -n "$project_id" ] || die "no project number $project under user $owner" +[ -n "$field_id" ] || die "project $project has no single-select field named 'Status'" + +# Case-insensitive match, so "backlog" and "Backlog" both work. The canonical name is +# what gets reported back, so a sloppy argument still produces an exact log line. +option=$(printf '%s' "$board" | jq -r --arg want "$status" ' + .data.user.projectV2.field.options[] + | select((.name | ascii_downcase) == ($want | ascii_downcase)) + | "\(.id)\t\(.name)"' | head -n 1) + +if [ -z "$option" ]; then + printf 'file-issue: no Status option named %s. Available:\n' "$status" >&2 + printf '%s' "$board" | jq -r '.data.user.projectV2.field.options[] | " " + .name' >&2 + exit "$EXIT_USAGE" +fi +option_id="${option%%$'\t'*}" +status_canonical="${option#*$'\t'}" + +printf 'repo %s\n' "$repo" +printf 'board %s (project %s)\n' "$project_title" "$project" +printf 'status %s\n' "$status_canonical" +printf 'labels %s\n' "${labels[*]:-(none)}" +printf 'title %s\n' "$title" + +if [ "$dry_run" -eq 1 ]; then + printf '\ndry run: everything above resolved; nothing was created.\n' + exit 0 +fi + +# --------------------------------------------------------------------------- +# Create. Past this line a failure can leave an issue off the board, so every +# error path prints the number. +# --------------------------------------------------------------------------- + +create_args=(--repo "$repo" --title "$title") +if [ -n "$body_file" ]; then + create_args+=(--body-file "$body_file") +else + create_args+=(--body "$body") +fi +for label in ${labels[@]+"${labels[@]}"}; do + create_args+=(--label "$label") +done + +issue_url=$(gh issue create "${create_args[@]}") || die "gh issue create failed; nothing was filed" +issue_number="${issue_url##*/}" +case "$issue_number" in + ''|*[!0-9]*) die "could not read an issue number out of: $issue_url" ;; +esac + +orphaned() { + printf 'file-issue: %s\n' "$1" >&2 + printf 'file-issue: THE ISSUE EXISTS BUT IS NOT ON THE BOARD. Fix it by hand:\n' >&2 + printf '%s\n' "$issue_url" >&2 + exit "$EXIT_ORPHANED" +} + +content_id=$(gh api "/repos/$repo/issues/$issue_number" --jq '.node_id') \ + || orphaned "could not read the node id for #$issue_number" + +# shellcheck disable=SC2016 # GraphQL variables, as above +item_id=$(gh api graphql \ + -f query=' + mutation($project: ID!, $content: ID!) { + addProjectV2ItemById(input: {projectId: $project, contentId: $content}) { + item { id } + } + }' \ + -f project="$project_id" -f content="$content_id" \ + --jq '.data.addProjectV2ItemById.item.id') \ + || orphaned "could not add #$issue_number to the board" +[ -n "$item_id" ] || orphaned "the board add returned no item id for #$issue_number" + +# shellcheck disable=SC2016 # GraphQL variables, as above +gh api graphql \ + -f query=' + mutation($project: ID!, $item: ID!, $field: ID!, $option: String!) { + updateProjectV2ItemFieldValue(input: { + projectId: $project, itemId: $item, fieldId: $field, + value: {singleSelectOptionId: $option} + }) { projectV2Item { id } } + }' \ + -f project="$project_id" -f item="$item_id" -f field="$field_id" -f option="$option_id" \ + >/dev/null \ + || orphaned "#$issue_number is on the board but its Status could not be set" + +# --------------------------------------------------------------------------- +# Read back. A mutation returning 200 is not evidence the board shows what was +# asked for -- this is the only check that is. +# --------------------------------------------------------------------------- + +# shellcheck disable=SC2016 # GraphQL variables, as above +readback=$(gh api graphql \ + -f query=' + query($item: ID!) { + node(id: $item) { + ... on ProjectV2Item { + content { ... on Issue { number } } + fieldValueByName(name: "Status") { + ... on ProjectV2ItemFieldSingleSelectValue { name } + } + } + } + }' \ + -f item="$item_id") \ + || orphaned "#$issue_number was written but could not be read back" + +seen_number=$(printf '%s' "$readback" | jq -r '.data.node.content.number // empty') +seen_status=$(printf '%s' "$readback" | jq -r '.data.node.fieldValueByName.name // empty') + +if [ "$seen_number" != "$issue_number" ]; then + orphaned "read-back names issue #${seen_number:-}, expected #$issue_number" +fi +if [ "$seen_status" != "$status_canonical" ]; then + orphaned "read-back Status is ${seen_status:-}, expected $status_canonical" +fi + +printf '\n#%s on %s as %s -- verified by read-back\n' \ + "$issue_number" "$project_title" "$seen_status" +printf '%s\n' "$issue_url" diff --git a/tools/local-emulator/run-e2e.sh b/tools/local-emulator/run-e2e.sh index 811e809..ed34159 100755 --- a/tools/local-emulator/run-e2e.sh +++ b/tools/local-emulator/run-e2e.sh @@ -456,6 +456,10 @@ cleanup() { # bash runs a trap only between commands, so this starts when whatever was in the foreground # returns -- which for Ctrl-C is immediately, because the same interrupt reached that command # too. `kill -INT` aimed at this script alone waits for the foreground command to finish. +# Invoked indirectly -- installed as the INT and TERM trap a few lines below. Both codes, +# because shellcheck 0.9.0 reports this as unreachable commands (SC2317) and 0.11.0 as an +# uninvoked function (SC2329); CI pins 0.11.0 but a local install may be either. +# shellcheck disable=SC2317,SC2329 on_signal() { echo echo "interrupted (SIG$1) -- stopping the emulator and removing the AVDs this run created"