Compare commits

...
Author SHA1 Message Date
JMR-dev 865a4a7c8e Merge branch 'main' into docs/benchmark-populate-path 2026-08-25 10:21:23 -05:00
Jason Ross e856679395 Merge pull request #103 from JMR-dev/fix/dead-assertion-probe-test
Delete an assertion that could never fail, and say what guards instead
2026-08-25 10:21:18 -05:00
JMR-dev 40ae524388 Merge branch 'main' into fix/dead-assertion-probe-test 2026-08-25 10:13:12 -05:00
Jason Ross 58a29ab093 Merge pull request #99 from JMR-dev/ci/actionlint
Lint the bash inside the workflows, not only the bash in files
2026-08-25 10:13:00 -05:00
JMR-dev a1d79c212a Merge branch 'main' into ci/actionlint 2026-08-25 09:51:15 -05:00
Jason Ross bc66906dc3 Merge pull request #98 from JMR-dev/test/device-codecs-encode-consequence
Hold the two codec MIME claims that only existed in prose
2026-08-25 09:51:00 -05:00
JMR-dev d37c391c60 Stop telling people to stage the benchmark the one way it cannot be staged
RealMediaBenchmark's class KDoc said:

  Populate with:
    adb push <file>.mp4 /sdcard/Android/data/org.libremediaconverter/files/

Twelve lines below, the `samples` property KDoc -- on `get() = context.filesDir` -- says:

  Internal storage, not the external files dir. Files placed in the external dir by
  `adb push` or `adb shell cp` stay owned by the shell user, and the app then gets
  EACCES trying to read them -- which presents as an unparseable input rather than a
  permission problem.

Different directories, and the second exists specifically to explain why the first fails.
Anyone following the class KDoc stages files the benchmark cannot read, gets a skip, and
reads the skip as "not staged yet" -- the failure mode the property KDoc warns about, walked
into by the instruction in the same file.

The fix is not a corrected command. Restating the mechanism in a second place is what let
these drift, and a replacement command I have not executed would be the same defect with a
fresher date. The class KDoc now names [samples] as the single place that answers it.

Two things added that are checkable rather than remembered: the exact filenames the tests
look for, via [H264_SAMPLE] and [AV1_SAMPLE] -- the old text said `<file>.mp4`, so even the
right directory left you guessing -- and a note that the two skips every green E2E leg
reports are these.

Not claimed: that the benchmark misbehaves on CI. An earlier version of the ticket said so;
it was wrong, and measuring settled it -- both tests report SKIPPED on the gating legs, the
guards work, and "harmless in CI" is accurate. The failure that prompted the look is
Media3EngineTest, tracked as #102.

Closes #101.
2026-08-25 09:45:00 -05:00
JMR-dev 95902a7889 Delete an assertion that could never fail, and say what guards instead
ConversionViewModelProbeFailureTest's pickedProbe() helper held:

  val ready = awaitState(viewModel.state, "Ready with a probe") {
      it is ConversionState.Ready && it.input.probe != null
  }
  assertNull("nothing here should reach a terminal failure", (ready as? ConversionState.Failed))

The predicate requires `Ready`. `Ready` and `Failed` are sibling subtypes of one sealed
interface, so `ready as? Failed` is always null and the assertNull could never fire. R26
filed this PLAUSIBLE on types read; it is measured now.

Flipping the line to assertNotNull failed 3 of the 4 tests in the class -- three, because
pickedProbe() has three callers, which is also why a dead line here was worth removing
rather than shrugging at: it read as coverage in a helper the whole class depends on.

Deleted rather than replaced. There is nothing for a live assertion to add: a pick that
ended in Failed never satisfies the predicate, so awaitState fails on its timeout naming
what it was waiting for -- "Ready with a probe" -- which is a better failure message than
the assertion would have produced. The comment now says that, so the next reader does not
re-add the guard the predicate already is.

This is the ninth vacuous assertion this line of work has turned up, and the pattern is
consistent: they hide in helpers, they pass, and they look like care. The suite is green
before and after, which is exactly the point -- deleting a dead assertion cannot change a
result, and if it had, the line was not dead.

Closes #35.
2026-08-25 09:36:36 -05:00
JMR-dev 3f140fc2b1 Lint the bash inside the workflows, not only the bash in files
The shellcheck step added a few hours ago reads `git ls-files '*.sh'`. That is four files.
It does not read the inline `run:` blocks, and a good deal of this repo's bash lives there:
the release verification in build.yml, the emulator setup and teardown in status_check.yml
and api37-debug.yml. "shellcheck runs in CI" was true of the files and not of the blocks,
and CLAUDE.md said so rather than pretending otherwise.

actionlint closes that half. It parses each workflow and runs shellcheck over every `run:`,
on top of its own checks for expression syntax, `needs:` references, matrix keys and action
input names.

Pinned by digest, for the reason shellcheck is pinned -- a new rule making untouched files
fail is a red build whose diff cannot explain it -- and for a second reason of its own.
actionlint's documented install is

  bash <(curl -s https://raw.githubusercontent.com/.../download-actionlint.bash)

off a moving branch. Running that in a repository that pins every action by SHA would
contradict its own supply-chain posture more than the linter is worth. That is why #70 was
filed instead of bolted onto the shellcheck commit.

It reported exactly one finding, and it is fixed here rather than suppressed: build.yml
parsed `ls` to pick the release APK (SC2012). The glob was already in the line, so a bash
array reads it without the pipe. Gradle's output names have no spaces today, which is the
kind of assumption that holds right up until it does not.

Proved it catches something, rather than trusting a green run: planting `if [ $UNQUOTED =
bad ]` into a build.yml `run:` block produces

  shellcheck reported issue in this script: SC2086:info:4:6:

Removed again afterwards. A linter that cannot be shown to catch a plant is not wired in,
it is just running -- and SC2086 in a `run:` block is invisible to the .sh-file step, which
is the whole argument for this commit.

CLAUDE.md loses the "does not cover inline run: blocks" caveat, because it no longer does.
Both linters verified clean at their pinned digests.

Closes #70.
2026-08-25 00:26:07 -05:00
5 changed files with 49 additions and 9 deletions
+5 -1
View File
@@ -81,7 +81,11 @@ jobs:
- name: Verify the released artifacts
run: |
APK=$(ls app/build/outputs/apk/release/*.apk | head -1)
# A glob, not `ls | head`: the glob is already here, and parsing ls is what
# SC2012 is about. Gradle's names have no spaces today, which is exactly the
# kind of assumption that holds until it does not.
apks=(app/build/outputs/apk/release/*.apk)
APK="${apks[0]}"
# A release that shipped one ABI, or lost 16 KB alignment, would install
# fine on a test device and fail for users or at Play submission. Both are
# cheap to check and expensive to discover later.
+17
View File
@@ -182,6 +182,23 @@ jobs:
docker run --rm "$SHELLCHECK" --version
git ls-files -z '*.sh' | xargs -0 -r docker run --rm -v "$PWD:/mnt" "$SHELLCHECK"
# actionlint closes the half shellcheck cannot see. The step above reads .sh files;
# a good deal of this repo's bash lives in inline `run:` blocks instead -- the release
# verification here, the emulator setup and teardown in this file and in
# api37-debug.yml. actionlint parses each workflow and runs shellcheck over every
# `run:`, on top of its own checks for expression syntax, `needs:` references, matrix
# keys and action input names.
#
# Pinned by digest for the same reason shellcheck is, and with a second reason of its
# own: actionlint's documented install is `bash <(curl -s .../download-actionlint.bash)`
# off a moving branch, which would sit badly in a repo that pins every action by SHA.
- name: actionlint
env:
ACTIONLINT: rhysd/actionlint@sha256:9d36088643581e728c969f35141f88139fec77280b2be23c1f66f8e40e1025e7
run: |
docker run --rm "$ACTIONLINT" -version
docker run --rm -v "$PWD:/repo" -w /repo "$ACTIONLINT" -color
# `!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.
+6 -5
View File
@@ -185,11 +185,12 @@ install for code that can never run — and on API 37 the full APK does not fit
`podman run --rm -v "$PWD:/mnt:z" docker.io/koalaman/shellcheck@sha256:61862eba... <files>`
(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.
**`actionlint` covers the half shellcheck cannot see** — the inline `run:` blocks, where a good
deal of this repo's bash lives. It runs shellcheck over each `run:` plus its own checks on
expression syntax, `needs:` references, matrix keys and action inputs. It sits in the same job,
**pinned by digest** for the reason above and one of its own: its documented installer is a
`curl | bash` off a moving branch, which does not belong in a repo that pins every action by SHA.
Locally: `podman run --rm -v "$PWD:/repo:z" -w /repo docker.io/rhysd/actionlint@sha256:9d360886... -color`.
## Dependency versions
@@ -36,8 +36,20 @@ import java.io.File
* 1. that the hardware path is worth having a second engine for at all, and
* 2. that x264's CRF is worth the GPL licence the app carries for it.
*
* Skips itself when the sample files are absent, so it is harmless in CI. Populate with:
* adb push <file>.mp4 /sdcard/Android/data/org.libremediaconverter/files/
* Skips itself when the sample files are absent, so it is harmless in CI — every green E2E
* leg reports two skips, and these are they.
*
* The two files it looks for, by exact name:
*
* - [H264_SAMPLE] for [hardwareVersusSoftwareOnRealVideo]
* - [AV1_SAMPLE] for [av1InputRoutesAccordingToDeviceDecodeSupport]
*
* **Where they go, and how, is on [samples] — read it before staging anything.** This used to
* carry an `adb push` line naming the external files dir, which [samples] then explains cannot
* work: a pushed file stays owned by the shell user and the app reads EACCES, surfacing as an
* unparseable input rather than a permission error. The instruction and its own refutation sat
* twelve lines apart. It is named in one place now rather than restated here, because restating
* it is what let the two drift.
*/
@UnstableApi
@RunWith(AndroidJUnit4::class)
@@ -140,10 +140,16 @@ class ConversionViewModelProbeFailureTest {
private fun pickedProbe(): InputProbe? {
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
viewModel.onInputPicked(INPUT)
// The predicate is the guard, and it is the only one needed. It requires `Ready`, so a
// pick that ended in `Failed` never satisfies it and `awaitState` fails on its timeout
// naming what it was waiting for -- "Ready with a probe" -- which says more than a
// separate assertion could. A `ready as? ConversionState.Failed` check used to sit here
// and was dead: `Ready` and `Failed` are sibling subtypes of one sealed interface, so
// the cast was always null and the assertNull could never fire. Measured, not assumed --
// flipping it to assertNotNull failed all three callers of this helper.
val ready = awaitState(viewModel.state, "Ready with a probe") {
it is ConversionState.Ready && it.input.probe != null
}
assertNull("nothing here should reach a terminal failure", (ready as? ConversionState.Failed))
return (ready as ConversionState.Ready).input.probe
}