build.yml's `release` job declares `contents: write`, and nothing checked it. Deleting those
lines leaves actionlint clean and CodeQL silent -- a narrower permission is not an alert --
and the job is `if: startsWith(github.ref, 'refs/tags/v')`, so no pull request and no merge
can exercise it. Measured with the declaration removed: every gating check still passed. The
first thing that would notice is a release failing to publish, at the moment someone is
trying to cut one.
The deletion also looks like tidying. #106 has just put a top-level `permissions: contents:
read` directly above it, so a reader could reasonably take the job-level block for a
duplicate. It is an override, and a comment saying so is not a check.
BackupExclusionsTest is the precedent: configuration rather than code, load bearing, and
unguarded because nothing compiles it.
The part worth reading twice is the second commit-worth of work in here. The test passed,
and then the mutation that is supposed to redden it did not:
BUILD SUCCESSFUL in 614ms
Gradle cannot infer that a test depends on a file outside the source set, so the task stayed
UP-TO-DATE and the test never ran. Under --rerun-tasks the same mutation failed it properly,
which is the tell: the assertion was right and the wiring was not. A guard that does not
re-run when its subject changes is not a guard -- it is a test that will be green on the day
it matters, which is worse than no test because it reads as cover.
Fixed by declaring the workflow as a task input. Verified the whole way round afterwards,
without --rerun-tasks: mutate the file and the task re-runs and fails; restore it and the
task re-runs and passes.
What this pins and what it does not: it asserts the declaration exists in the release job's
block. It cannot assert a release actually publishes -- that needs a tag push, which is the
thing no PR can do. A tripwire against silent removal, not proof the path works, and the
KDoc says so.
Closes#107.
CodeQL alert #1, the only open one on this repository:
actions/missing-workflow-permissions, warning / medium, build.yml:23
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN.
Alerts 2, 3 and 4 were the same rule against status_check.yml and are fixed -- that file
has a top-level block. build.yml declares permissions in exactly one place, the release
job's `contents: write`, and has no top-level default, so the `test` job inherits the
repository setting.
**Nothing is over-privileged today.** The repository default is already `read`
(default_workflow_permissions: read, can_approve_pull_request_reviews: false, read from the
API rather than assumed), so the test job holds a read token now. Saying so matters: this
is hygiene, and a commit that implied it was closing a live hole would be overstating it.
What it buys is that the default CANNOT widen these jobs later without someone editing this
file. That is not invented for the occasion -- it is the argument status_check.yml already
makes, which even names this file:
the token's reach should be readable here, and a default that widens later should not
silently widen these jobs with it. build.yml's release job makes the opposite
declaration for the same reason.
So the principle was decided, applied in two workflows and in one job of this one, and the
top level of build.yml was the gap.
Verified the thing that would actually break: the release job's `contents: write` still
wins. Top level is a default, not a ceiling -- parsed and printed both, test inherits
`contents: read`, release keeps `contents: write`.
Also ran the ticket's mutation, and it found something. Deleting the release job's
`contents: write` leaves actionlint green and CodeQL quiet -- a narrower permission is not
an alert -- so nothing would catch it until a tagged release failed to publish. That is a
separate gap and is filed rather than fixed here.
actionlint clean at the pinned digest. Comment and permissions only; no step, job or
trigger changes.
Closes#100.
R19 raised two things about this comment. One resolved itself: it used to explain why the
matrix had no API 37 row at all, and #56 added the gating row, so that half is gone.
The other survived, and this is it. The comment read
api-level must be "37.0". A bare 37 is not an SDK package and fails during setup
The second sentence is true and was measured -- it cost a run to find. The first overstates
it. What must be true is that the api-level is a POINT release; 37.0 is one of several.
api37-debug.yml's own input descriptions already say so:
API level, as the SDK spells it. 37.0, 37.1, 37.2-beta3, 36 ...
System image target. android-37.1 and 37.2-beta* ship ONLY as google_apis_ps16k
and docs/api-37-emulator-crash.md measures android-37.0 rev 6 and android-37.1 rev 8 side
by side, both aborting. So the repo already knows 37.1 exists and behaves the same; only
this comment implied otherwise.
That matters for the reader it is written for. Someone debugging this row and wondering
whether a newer image helps reads "must be 37.0" as a constraint and stops. The measured
answer is that it does not help, which is a better thing to learn than a rule that is not
one -- and the ps16k-only wrinkle above 37.0 is the detail that would actually bite them.
Comment only. No job, matrix, filter or gating behaviour changes. actionlint clean at the
pinned digest.
Closes#28.
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.
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.
SafPickerRoundTripTest began failing on gating legs at API 33, 34, 35 and 37
ninety minutes after it landed, on diffs that cannot cause it -- two KDoc
comments, a MIME lookup table, a README paragraph. Every failure named the
fixture root, so #93 was filed as a root-discovery race. It was not one, and
finding out what it was took making the test say something else first.
DocumentsUI was fine throughout: its own `ProvidersAccess: Matched roots` names
the fixture authority five times inside the sixty seconds the test spent failing.
What failed was reading any window at all -- 1095 `Retrieving node with selector`
against 1095 `Node not found` on that leg, against 7 and 2 on the green one. So
this now asks whether the app's OWN window is readable before it opens a picker,
and prints the accessibility window list when it is not.
That list named the culprit on the next occurrence:
What it could see: com.android.systemui[type=3], android[type=3]
No TYPE_APPLICATION window at all, on a device that had just logged `Displayed
org.libremediaconverter/.MainActivity`. `android[type=3]` is system_server, and
the same logcat says what it was holding, minutes before this class ran:
ANR in com.google.android.apps.nexuslauncher
Reason: Input dispatching timed out (Application does not have a focused window)
Window{4ed8414 u0 Application Not Responding: com.google.android.apps.nexuslauncher}
The launcher ANRs on a loaded runner emulator and the dialog it leaves behind
never goes away. It is opaque and fullscreen, so AccessibilityWindowManager drops
every application window beneath it -- which is how the app can be Displayed and
unreadable at once, the contradiction that made this look like a SAF bug for six
PRs. Present on both legs examined, API 33 and 34, at the failure timestamp.
So the dialog is dismissed, by resource id rather than by localised button text,
`aerr_wait` first so the app under it is left alone. Waking the device and
rebuilding the UiAutomation connection are kept behind it and are recorded as
measured non-causes rather than as fixes.
A second PickActivity is not a remedy for this either, and that was measured: the
failing leg opened one for the second test, in the same DocumentsUI process, and
read as little from it. The whole pick is still retried, but for a smaller and
separate claim -- a picker whose lists were built before their data arrived, which
#80's node-level re-find cannot reach because it re-acquires a handle inside the
one picker.
One API 37 run failed a step deeper, on the file rather than the root. That shape
has not been reproduced or diagnosed; the reopen covers it because a fresh pick
re-walks from Recent, and the KDoc says that rather than claiming more.
Two things the retry must not become. It must not tolerate an absent root, or
#64's MIME mutation goes vacuous -- so a missing node is reported rather than
retried away, and the mutation was re-run: both tests still fail, still with "the
system picker never showed BySelector [TEXT='\QLMC R38 fixtures\E']", in 126 s and
127 s against the 1200 s wrapper timeout. And it must not decide the picker has
closed by asking the same accessibility window list that is broken -- so the back
presses are counted against Activity.hasWindowFocus, which comes from the
framework.
Each new path was forced on and measured rather than trusted: the injected-failure
run showed the reopen recovering, with four OPEN_DOCUMENT starts for two tests;
the rebuild was forced unconditionally and the suite stayed green, ruling out a
connection that comes back without FLAG_RETRIEVE_INTERACTIVE_WINDOWS; the dialog
dismissal was forced with no dialog present, ruling out a blind click breaking a
healthy run. Dismissing a real ANR dialog has not been observed, because the fault
has never reproduced locally.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Two reasoned decisions were sitting in comments with nothing under them.
`AndroidDeviceCodecs.mimeFor`'s `COPY, NONE -> null` arm explains itself by
naming a consequence at another seam: returning null is what makes `canEncode`
answer true, because a copied or absent track places no demand on the hardware.
#90 pinned the null; nothing pinned the answer. Put a MIME in that arm and a
device with no matching encoder starts refusing stream copies — jobs that encode
nothing — and the router hands FFmpeg a re-mux Media3 could have done. Asserted
now against `forTesting(encoders = emptySet())`, with an H.264 refusal alongside
so a `canEncode` that simply said yes could not satisfy it.
The second is a whole table. `Media3Engine.videoMimeTypeFor` is `VideoCodec ->
MIME` on the same axis as `mimeFor`, and until #85 and #87 widened both to
`internal` no test could see them together. Each had per-arm coverage pinning its
own answers, which is exactly the shape that cannot notice the two tables
describing different codecs: change one arm and its own expectation together and
both suites stay green while the device is asked about H.265 and Transformer is
told to produce H.264.
They do not agree everywhere, and forcing them to would be a regression, so the
test sorts every codec into the three buckets that exist and asserts the fourth
is empty. H.264 and H.265 must match. VP8, VP9 and AV1 are named by the device
table and not by Transformer's, deliberately: `setVideoMimeType` rejects them so
the router never asks Media3, while the device may genuinely own a VP9 encoder
and `canEncode` has to answer about it truthfully. COPY and NONE are named by
neither. Sorting rather than filtering means a convergence fails too, so moving
the line requires saying so in the file.
Audio has no partner — `AndroidDeviceCodecs` enumerates video MIME types only,
so `audioMimeTypeFor` has nothing to cross-check against and a missing audio
encoder is still discovered by failing rather than up front. Named in the KDoc
as unfinished rather than left as an unexplained asymmetry.
Closes#86
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
run-e2e.sh installs a missing system image with
yes | sdkmanager --install "$pkg" > /dev/null 2>&1 || { echo " FAILED to install"; ... }
`yes` never ends. The moment sdkmanager exits and closes the pipe, `yes` dies of SIGPIPE
with 141, and this script runs under `pipefail`, which takes the rightmost non-zero status.
So a package that installed perfectly reported "FAILED to install $pkg" and returned 1.
R32 filed this PLAUSIBLE on shell semantics, unexecuted. It is demonstrated now:
set -o pipefail; yes | true -> 141 (three runs, three times)
set -o pipefail; yes | sh -c 'exit 3' -> 3
${PIPESTATUS[1]} for those two -> 0 and 3
The pipeline status genuinely cannot tell a clean install from a broken one; PIPESTATUS
can. That is the whole change -- no restructuring of the licence flow, so a fresh SDK still
gets its licences accepted exactly as before.
`echo no | avdmanager` eleven lines below is deliberately left alone, and the comment says
so. One line fits the pipe buffer, so echo has already exited before the close and there is
no signal to receive: `echo no | true` measured 0 on five consecutive runs against `yes |
true`'s 141 on three. Only an unbounded producer is exposed. Someone reading this fix later
would otherwise "fix" the echo too and change a line that was never wrong.
Why it went unnoticed: it only misfires when the image is ABSENT, and every existing
checkout already has the images. R32 noted the branch that made this the normal path. The
failure is also silent in the worst way -- the install succeeds, the script says it failed,
and the AVD is then created from a package that is really there.
shellcheck clean at the pinned digest (0.11.0, the version CI runs), bash -n clean.
Closes#41.
Both the dependency declaration and its catalog entry named
EscapedCoroutineErrors.kt as the sole reason kotlinx-coroutines-test is on the
test classpath. That file is gone, and nothing in the gate -- not ktlint, not
detekt, not lint -- fails on prose naming a deleted file, so this would have
survived as a reference a reader could only resolve through git history.
The dependency itself stays, and for a reason worth restating where it is
declared: `runTest` is what registers the collector callback, so the one test
that deliberately lets an error escape is the scope that receives it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
README promised, without qualification:
"Conversions run as durable background work, so they survive leaving the app and
are restored after a restart."
The first half is true and the reattachment work made it truer. The second half has one
exception the sentence does not admit, and it is the case a user is most likely to hit
without understanding it.
When Android refuses a foreground-service start, FailureOutcome retries -- ten attempts on
the default exponential backoff, 30 s doubling to a five-hour clamp, about eight and a half
hours in total -- and then returns FOREGROUND_DENIED on a FAILED job. Reattachment excludes
FAILED (Reattachment.kt:176). So the job is not restored, and neither is the message
explaining why: the user opens the app to an empty screen.
FailureOutcome's own KDoc already says this plainly -- "a user who was not watching when
the eleventh attempt ran will find an empty screen rather than the explanation". The code
was honest and the README was not, which is the wrong way round for the two documents.
The replacement says what actually happens and ends with the thing the user can act on:
reopening the app is what grants permission to run, so a conversion stalled this way should
be started again rather than waited on. That is the same reasoning FOREGROUND_DENIED_MESSAGE
is written on -- "open the app and start it again" is the fix, not filler.
Deliberately not claimed: that the app tells you. It does not, and #16 is the open ticket
for giving a present, willing user a way to make that retry happen now. Writing "you will
be told" here would be the same defect this commit is fixing, one release earlier.
Verified against the current code rather than the finding's date -- R35 was filed as
PLAUSIBLE on 2026-08-22 and both mechanisms it names are still in place.
Closes#44.
`drainEscapedCoroutineErrors()` cleared the collector at rule-construction time
with `runCatching { runTest {} }`, and discarding what it found was the whole
mechanism: it could not tell the one known deposit from an escaped error nobody
had asserted on. That traded a loud, misleading failure for a silent one, which
was acceptable only while exactly one depositor existed and the seam to remove it
did not.
The seam exists now, so the depositor is gone: the OOM is consumed by the test
that raises it. Every Compose class takes the v2 `createComposeRule()` directly,
and a future escaped error fails a test again instead of disappearing.
The two findings the drain's KDoc carried that outlive it: the v2 rule and the
non-v2 `StateRestorationTester` do interoperate -- the note now sits at the two
declarations that pair them -- and a drain could never have been a `@Before`
(the rule's `runTest` wraps it) or a `@BeforeClass` (Robolectric runs that
outside the sandbox classloader, where the collector is a different object).
Full JVM suite run twice in a row with the drain deleted: 373 tests, 0 failures
both times.
Closes#66
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`onInputPicked` hops to a hard-coded `Dispatchers.IO` inside a `launch` with no
exception handler -- deliberate, because a real OutOfMemoryError should reach the
thread's default handler and take the process down. On the JVM there is no such
handler: kotlinx-coroutines-test installs a process-wide collector, once per
classloader and never removed, which keeps the error and rethrows it at whichever
`runTest` starts next. Every Compose rule is a `runTest`, so the OOM raised by
`ConversionViewModelProbeFailureTest` failed some *other* Compose class, and which
one moved between runs of identical, green code.
Naming the dispatcher gives the throw somewhere to land. With the pick inline
inside a `runTest`, the collector's callback belongs to the test that caused the
error, so it is handed over and consumed rather than stored for a stranger.
Both hops of a pick rather than only the probe, which is where this differs from
the seam issue #66 sketched: leaving the metadata query on a real IO thread makes
the coroutine resume on a main looper Robolectric leaves paused, and that bounce
is exactly the asynchrony that made delivery unpredictable.
That buys the assertion the test could not make before -- the real OutOfMemoryError
instance, not an inference from a card that never filled in, which is also what a
probe returning null looks like. Reverting the hop to `Dispatchers.IO` turns it
red: "expected java.lang.OutOfMemoryError to be thrown, but nothing was thrown".
Refs #66
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bare assertTrue/assertFalse report java.lang.AssertionError and nothing else,
so the mutation that proves this test bites -- relaxing the _pipe suffix to a
substring -- went red saying only that a line failed. The format name is the
one thing a reader needs, exactly as the MIME is in the sibling test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two test classes justified using Robolectric by asserting that the alternative does not
exist:
AppRootRestorationTest "The instrumented tests cannot run on the development
host at all (see CLAUDE.md)"
OutputPublisherStagingTest "The instrumented suite cannot run on the development
host, so this is the only place [it] can be caught"
Both were true when written and stopped being true on 2026-08-22, when the segfault was
traced to SwiftShader's Reactor JIT against SELinux execheap rather than to the machine.
tools/local-emulator/run-e2e.sh has run API 33-36 here since.
The first one cites CLAUDE.md as its authority, and PR #73 corrected CLAUDE.md to say the
opposite. So it was no longer merely stale: a reader who followed the reference found the
contradiction, with the citation making the wrong half look verified. That is the worst
version of this -- R14, R15, R20 and R25 were all the same defect, and this is the fifth.
The choice itself was never wrong, which is why the fix is not to move these tests. Both
belong on the JVM, and the honest reason is cost rather than impossibility: neither needs
anything a device supplies, and both run inside the same ./gradlew invocation as every
other unit test instead of booting an emulator. That argument survives the correction; the
premise did not.
The old line also has a second failure mode worth naming. "Nobody can execute this" invites
a reader to skip the local run and let CI decide, which is the opposite of what the
definition-of-done in #51 asks for.
Verified: the string appears nowhere in app/src now, and testDebugUnitTest, ktlintCheck and
detekt are green.
Closes#46.
The FFprobe codec vocabulary is written out in at least four places and none of them
had a test. Two had already drifted apart. `x264`, `hev1`, `x265` and `vp09` resolved
in `CodecNames.videoFromName` and returned null from
`AndroidDeviceCodecs.mimeForCodecName`, so the app identified the codec for the source
card and for routing and then ran the device capability check blind on the same string;
`mpeg4` ran the other way and rendered as a raw name. Nothing could notice, and the
reason is structural: a `when` cannot be enumerated, so no test can ask one table what
the other one knows.
Both are maps now, for that reason alone, and `CodecVocabularyTest` walks the two key
sets. A name added to -- or removed from -- one side alone fails the build. The one
legitimate asymmetry is listed rather than implied: `mpeg4` is decodable input with no
`VideoCodec` to name it, so `CodecNames` is right not to carry it. That list is itself
checked, because otherwise it is an escape hatch -- any future divergence could be waved
through by adding the name to it, and adding `x265` to it now fails.
THIS CHANGES BEHAVIOUR for `x264`, `hev1`, `x265` and `vp09`. A null from
`mimeForCodecName` means "unknown to us: assume the platform can handle it and let a
failed export trigger the FFmpeg fallback", which is the right policy for a name nobody
recognises and the wrong one for a name recognised one file over. A device without the
matching decoder now sends those four to FFmpeg up front instead of spending a doomed
hardware attempt to discover it. No input loses hardware it could have used: each alias
resolves to the MIME its canonical spelling already resolved to, so a device that has
the decoder still answers true. `ConversionRouterTest` still passes and that is not
evidence either way -- every `canDecode` in it is a hand-written stub that never reaches
this table.
#74 is the same family one level down. `describeVideo` answered "Unrecognised" for
`InputProbe.UNPARSEABLE` and `describeAudio` had no such arm, so an unparseable audio
codec would have fallen through to `?: name` -- and the sentinel opens with a NUL, so
the source-info card would have rendered a `Text` beginning with U+0000. The two now
share one body, which is what stops the next arm being added to one side only.
Two corrections to that ticket, taken from the file rather than from the ticket, since
it warns about exactly this:
- It quotes `audioFromName` as opening with `null, InputProbe.UNPARSEABLE -> null`.
It did not; it opened with `null -> null` and the sentinel reached `else`. Naming
the sentinel in the shared lookup therefore changes no answer and is documentation,
not the fix.
- It says `describeVideo`'s arm has no test of its own. It did -- `descriptions stay
readable for unknown and missing codecs` asserts it -- so deleting the shared arm
now reddens three tests across both sides, not one.
Mutations run, each on the full 386-test suite:
add "avc3" to CodecNames only -> CodecVocabularyTest red on two counts,
CodecNamesTest green: 8 tests, 0 failures, which
is the ticket's point about per-table arm tests
delete the UNPARSEABLE arm -> CodecNamesTest red on three, one of them quoting
the NUL back
add "x265" to DECODE_ONLY_NAMES -> CodecVocabularyTest red on the escape hatch
delete "vp09" from the MIME map -> CodecVocabularyTest red on three, which is the
state this commit is fixing
Audio is not cross-checked, and that is a gap rather than a decision: the device
capability check is video-only, so this module has no second audio table to compare
`AUDIO_ALIASES` against. `Media3Engine.audioMimeTypeFor` is the other half and belongs
to #85. `MediaProbe.shortName` (#84) is the fourth table and is untouched here for the
same reason.
Closes#87.
Closes#74.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both tables decide what codec ends up in the user's file, and neither was
exercised. Point H265 at VIDEO_H264 and every hardware HEVC export writes
H.264 into a file the user asked to be H.265: Transformer does as told, the
export succeeds, and the only symptom is a codec nobody chose.
One arm carried an assertion rather than a value -- "Never reached: only an
Encode plan consults this, and COPY/NONE are not Encode" -- which is a claim
about callers parked in a branch of a callee. It is true, and nothing checked
it, so it would have gone on reading as true after it stopped being. Proved
instead: CopyPlanner answers both codecs before the Encode branch and its
fallback draws from ContainerCapabilities.encodableVideo, which contains
neither, so a sweep over every spec the planner can be handed asserts no
Encode plan carries COPY or NONE. Counters guard the sweep, because
`as? Encode ?: let` asserts nothing at all for a Drop or Copy plan.
The audio sibling claim did not survive intact. "MP3 and FLAC have no Android
encoder; the router routes them to FFmpeg" is true and incomplete: one rule,
`audioEncode !in MEDIA3_AUDIO`, diverts Vorbis by identical logic, so three of
the six encodable codecs never reach the table. VORBIS -> AUDIO_VORBIS is a
correct mapping for a request Transformer is never given. The arm stays -- a
right answer in unreachable code costs nothing -- and the comment now says so.
The tables are asked of the router's decisions rather than of its codec sets,
because the comments claim behaviour and a set can be right while the rule
reading it is wrong. Both move to an internal companion object so a JVM test
can reach them without constructing an engine, which would start a real
HandlerThread to answer an enum lookup; #57's precedent, and the JVM test
source set is a friend of main.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MediaProbe's MIME table, its image-demuxer rule and its Int reader are pure
functions with no test at all, and each fails silently rather than loudly.
shortName falls through to substringAfter('/') and reports a plausible-looking
string that CodecNames may or may not still recognise, so a dropped arm turns a
stream-copyable file into a re-encode. isImageFormat is checked before anything
else in classify, so a wrong answer overrides both probes. intOr's runCatching
is the only thing standing between a Float frame rate and losing every other
track property the loop had read.
Widen the three to internal, as #57 did, and say in each KDoc why the shape is
what it is -- the _pipe suffix is not a substring test because yuv4mpegpipe is
raw video, and getInteger casts rather than coerces.
Every format name asserted came from ffprobe rather than from memory: a picked
.png reports png_pipe, a .jpg reports jpeg_pipe, a .y4m reports yuv4mpegpipe.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#80 added SafPickerRoundTripTest's rotation case to @FailsOnEmulatorApi37, because a
real rotation aborts the framework on android-37.0. Three tests carry the marker now --
two in Media3EngineTest, one in SafPickerRoundTripTest -- and five statements still
described two.
Four were counts, and wrong:
status_check.yml "notAnnotation removes the two tests that do not pass"
status_check.yml "The two API 37 tests the gating row above excludes"
CLAUDE.md "the gating leg runs the other 55" (59 - 3 = 56)
CLAUDE.md "do not read a green run as evidence those two tests pass"
The fifth was worse, because it was not a count. The advisory job's header justified its
name with an invariant:
"It is named for WHAT IT RUNS, deliberately. Both tests drive a full H.264 -> H.265
hardware transcode through Media3Engine"
The rotation case drives no transcode. So the comment did not merely miscount -- it
asserted a property of the job's contents that had stopped being true, and that property
was the entire argument for the name.
The name is unchanged, deliberately, and the header now says so instead of implying the
question never arose. This is not a required context, it is red on every PR by design, and
it is one people have learned to look for; renaming a check costs more than the imprecision
does. What replaced the invariant is the honest rule: THE MARKER IS THE DEFINITION, NOT THE
NAME -- this job holds the tests that cannot pass on the API 37 emulator image, whatever
their subject.
Two things stay as they were because they are still true. "the two Media3EngineTest cases
that pass here" is correct: that class has four tests and two carry the marker. And the
decoder theory is still a claim about the Media3 pair alone, so it now says so rather than
being read as covering a rotation failure it has nothing to do with.
Nothing about the job's behaviour changes: same name, same continue-on-error, same marker,
same selection on both rows. Verified: yaml parses, five jobs, matrix still 33/34/35/36/37.
The check to re-run when a test next joins or leaves the marker, which is the event that
broke this twice:
grep -rn "@FailsOnEmulatorApi37" app/src/androidTest --include='*.kt' | grep -v import | grep -c FailsOn
It must equal the number every corrected comment states. It is 3.
Closes#81.
CI found a flake this workstation could not, and fixing it overturned half of what
the previous commit recorded about API 37.
THE FLAKE. UiObject2 caches the AccessibilityNodeInfo it was found with, and
DocumentsUI is still settling when a node first appears -- its list rebinds, the
roots strip lays out, a window animates. If the node is replaced in that gap,
click() throws against the handle rather than missing the target:
androidx.test.uiautomator.StaleObjectException
at androidx.test.uiautomator.UiObject2.getAccessibilityNodeInfo(UiObject2.java:1042)
at androidx.test.uiautomator.UiObject2.click(UiObject2.java:526)
at SafPickerRoundTripTest.pickTheFixture(SafPickerRoundTripTest.kt:223)
It is not intermittent on a COLD emulator -- CI hit it on API 33, 34 and 35, every
one of them, on the first run. It never appeared here because the local emulator had
been warm for an hour. tapPickerNode now re-finds the node and taps again, three
attempts. That retries acquiring a handle to a node that has to be there anyway:
every attempt still goes through awaitPickerNode, which fails outright if it is
absent, so the MIME mutation's bite is untouched. Verified with `pm clear
com.google.android.documentsui` between runs, five for five green on API 34.
AND THE CORRECTION IT FORCED. The previous commit marked the whole class
@FailsOnEmulatorApi37 on the strength of two measured failures. One of them was
this bug. Re-measured with the fix, one method per fresh android-37.0 emulator:
thePickedInputSurvivesARealRotation INSTRUMENTATION_ABORTED:
System has crashed.
pickingAFileThroughTheSystemPickerFillsInTheFileCard PASSED
So a rotation, which rebuilds every surface at once, is what the gralloc mapper does
not survive; starting another app's activity is not. The marker moves to the one
method that earned it, and the picker test runs on the gating API 37 leg like
anything else. The workflow comment, run-e2e.sh and the doc all say that now.
The lesson is worth more than the measurement, and the doc keeps it: an annotation
is a claim about an IMAGE, and a broken test makes every image look broken. Both a
framework abort and a stale node read as "the run fell over". Re-measure after
fixing a test before deciding what the platform did.
Also measured rather than assumed, since it is what keeps the gating leg green: the
runner's annotation filter honours a class-level marker, expanding it to every
method. On API 34, `annotation=` selected exactly 4 tests (2 Media3EngineTest + 2
here) and `notAnnotation=` selected 55 with neither of these in it. CI's own gating
API 37 leg then reported 55 / 0 on the previous push. That is why moving the marker
to a single method is a narrowing rather than a repair.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The API 37 emulator images abort surfaceflinger inside the guest's Gralloc5 mapper,
init SIGKILLs zygote with it, and the framework restarts under the run. run-e2e.sh
and the CI leg disable SystemUI to remove the trigger -- but that removes the IDLE
one, RegionSamplingThread's nav-bar luma sampling. Driving DocumentsUI and rotating
the display are not idle. They are the first things in this suite that generate
surface traffic of their own.
Both tests were measured on android-37.0 under swangle_indirect with SystemUI
disabled and verified quiet, and measured SEPARATELY -- inferring the second from
the first is the mistake docs/api-37-emulator-crash.md opens by correcting. They
fail in the two shapes a framework restart produces:
thePickedInputSurvivesARealRotation
INSTRUMENTATION_ABORTED: System has crashed.
Expected 59 tests, received 50
(5 hasReadColorBufferDma aborts; the framework dies DURING the test, so six
later tests never run and the XML carries a failure with no text at all)
pickingAFileThroughTheSystemPickerFillsInTheFileCard
androidx.test.uiautomator.StaleObjectException
at androidx.test.uiautomator.UiObject2.click(UiObject2.java:526)
(3 aborts; the picker's root node was rebuilt between finding it and tapping it)
Both pass on API 33 and API 36 locally -- whole suite, 59/0/0/2 on each -- which is
the same evidence pattern that made the Media3EngineTest pair the image rather than
the app.
So the class carries @FailsOnEmulatorApi37 and runs on the advisory leg.
THREE PLACES SAID "nothing in this suite touches system UI", and that is what makes
the SystemUI-disable deviation defensible. It is no longer true of the suite, and all
three are corrected rather than left to rot -- the workflow comment, run-e2e.sh's
header, and the doc. The rule they state is being APPLIED, not broken: the thing that
depends on system UI is excluded from the leg that cannot be trusted for it.
Two consequences stated rather than left to be discovered:
- run-e2e.sh applies no annotation filter, unlike CI, so a local `run-e2e.sh 37`
reports these two on top of the Media3 pair AND DOES NOT FINISH. Its totals come
back short and which later tests ran is arbitrary. The summary row now says so;
it previously promised "exactly two failures", which would have read as a
regression in someone else's diff.
- The advisory job is still named "E2E API 37 Media3 hardware transcode", and half
of what it now runs is neither. Renaming a check touches branch protection, so it
is deliberately not done here; the doc records the staleness and the revisit
trigger now says the marker covers two unrelated bugs that can go green apart.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two things nothing in this repo asserted, and they are one test class because
separately the second one asserts nothing new.
THE PICKER. ConverterScreen opens SAF with a MIME filter, and a filter is a thing
that can hide the user's file. Narrow it and the app still builds, still renders,
and still passes every JVM test -- the user taps "Choose file" and gets an empty
picker. The round trip now runs for real: DocumentsUI is driven with UiAutomator to
a fixture root, and the app is asserted to come back with the file.
The file card's name is not the only assertion, because a name proves less than it
looks: it comes from a metadata query, which a URI with no read grant answers just
as well. The "Container: MP4" detail row only appears once something has opened the
file and read its header, so it is what says the picker handed back a URI the app
can USE.
THE ROTATION. MainActivity declares no configChanges, and ConversionViewModel holds
the picked file in a plain MutableStateFlow with NO SavedStateHandle behind it.
Nothing persists it. The only thing that carries it across a rotation is the
ViewModelStore the Activity retains -- which no test anywhere asserted.
Two guards run before that assertion, because both ways it could pass while proving
nothing are silent: the display rotation really changed, and MainActivity really was
a different instance afterwards. Without the second one this is a recomposition test
wearing a rotation's name.
MUTATIONS, RUN RATHER THAN ASSERTED, on a local API 34 emulator.
Narrowing the filter to arrayOf("application/x-lmc-no-such-type") takes the fixture
root out of the picker entirely -- DocumentsUI matches the request against
Root.COLUMN_MIME_TYPES and drops roots that cannot answer -- and both tests fail:
java.lang.IllegalArgumentException: the system picker never showed
BySelector [TEXT='\QLMC R38 fixtures\E']
Making the ViewModel composition-scoped fails ONLY the rotation test:
androidx.compose.ui.test.ComposeTimeoutException: Condition (a node tagged
converter.fileCard.name exists) still not satisfied after 30000 ms
and :app:testDebugUnitTest stays BUILD SUCCESSFUL under it. That divergence is what
#64 exists to establish and what its own comment doubted; the PR body has the
verdict and why the doubt was reasonable.
THE PROVIDER HAD TO BE JAVA. It is the only Java file in the module. A
manifest-declared provider is a component of the instrumentation PACKAGE, so the
system starts a plain org.libremediaconverter.test process for it with only the test
APK on its dex path -- and the test APK is built without the Kotlin stdlib, because
the app APK has it and duplicating it is what checkDebugAndroidTestDuplicateClasses
prevents. The Kotlin draft died on its first query:
java.lang.NoClassDefFoundError: Failed resolution of: Lkotlin/jvm/internal/Intrinsics;
at org.libremediaconverter.saf.FixtureDocumentsProvider.queryDocument
The compiler emits that reference for the null checks on nearly every function, so
no Kotlin dialect avoids it. Same reason nothing in that file imports androidx.
No new test tags: CHOOSE_FILE, FILE_CARD_NAME and detailRow already named both ends.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing in either source set drives SAF as a picker. The only SAF coverage is the
publish side, in OutputPublisherPublishTest, against hand-written ContentProvider
fakes -- so the launcher wiring in ConverterScreen, the MIME filter it passes, and
the grant that comes back have never been executed by a test.
Driving the real picker needs three things this repo did not have.
UiAutomator, because DocumentsUI is another process. Compose's matchers stop at this
process's composition and Espresso's stop at its view hierarchy; neither can see or
tap a window belonging to another package.
It FLOATS, at "2.+", which is the same argument the catalog already makes for work
and lifecycle rather than a new one: androidx.test.uiautomator is inside
floatedGroupPrefixes, so the componentSelection guard makes "+" mean "newest
RELEASED", and that is load-bearing here -- this library publishes 2.4.0-alphas above
its stable, so without the guard the float would be a pin to a prerelease. Resolved
to 2.4.0 (released) on debugAndroidTestRuntimeClasspath, checked rather than assumed.
It is deliberately NOT pinned alongside ktlint/detekt/JaCoCo/Robolectric: those are
pinned because a new rule or a new runtime changes the verdict on files nobody
touched. UiAutomator has no verdict -- it taps what a selector names, and a selector
that stops matching is this repo's test to fix, in a diff that explains itself. The
"2." rather than a bare "+" is the one thing held back: a major is where the selector
API would be free to change under exactly that assumption.
A DocumentsProvider, because DocumentsUI does not browse a filesystem -- it lists what
providers offer it. Writing a file into Downloads would have worked and tested less:
the fixture root declares Root.COLUMN_MIME_TYPES, and DocumentsUI filters the drawer
by it, which is what gives the MIME filter a mutation with a shape rather than "one
file among the hundreds in Downloads was not listed". Its contents are also exactly
one file, where a shared directory accumulates whatever earlier runs left behind.
And the first AndroidManifest.xml this source set has ever had, to declare it -- a
ContentProvider is instantiated by the system and cannot be registered from test
code. In androidTest rather than src/debug so it is installed by the instrumentation
APK only, and never appears in a developer's own file picker.
Two things worth knowing before editing either file. XML comments cannot contain "--",
which the manifest's first draft failed the build on; and "*/" inside a KDoc closes
the comment, which the provider's did. Both are silent in review and loud in the
build.
No test yet, and no new test tag: TestTags.Converter.CHOOSE_FILE and FILE_CARD_NAME
already name both ends of the round trip.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JMR-dev
Jason Ross