Nothing had ever handed InputQuery a cursor row. UnknownInputSizeTest
drives the no-provider case thoroughly -- query returns null, measure()
answers -- so firstRow's body, displayNameOrNull and sizeOrNull had never
executed at all.
Nine tests over FakeSafProvider's RowShape states. What they pin is not
"reads a cursor" but the rule the class exists for: a size nobody could
determine must arrive as null, never 0. Four separate ways a provider
fails to give one -- a null cell, a missing column, a negative value, an
empty cursor -- plus a provider that throws outright, which is the guard
firstRow's KDoc is written for.
Mutations run, all four bite:
drop `takeIf { it >= 0 }` from sizeOrNull -> negative-size test red
drop `!isNull(it)` from sizeOrNull -> null-size test red
drop the runCatching in firstRow -> throwing-provider test red
drop `!isNull(it)` from displayNameOrNull -> GREEN, does not bite
That last one is recorded in the test's KDoc as a named exemption rather
than papered over. Measured: MatrixCursor.getString on a null cell returns
null while getLong returns 0. So the guard is load-bearing on the size path
-- it is what stops a null becoming a real number -- and unfalsifiable on
the name path, where getString already yields null. It stays regardless:
Cursor.getString's contract makes throwing on null implementation-defined,
and a real provider may do what MatrixCursor does not.
InputQuery.kt now has no never-executed lines. Suite 456 -> 465 tests,
branch coverage 63.8% -> 65.6%.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two of #132's items are cursor-shaped -- InputQuery's row reads (#137) and
OutputPublisher.destinationIsKnownEmpty's short-circuits (#140) -- and the
provider that could drive them lived inside OutputPublisherPublishTest and
could only answer correctly. Its row was always (file.name, file.length()).
Moved FakeSafProvider, FakePlainProvider and the registration helper to
FakeProviders.kt, same package, following StagingCleanupSupport.kt and
ParkedPickDispatcher.kt. UnreliableOutputStream stays behind: it serves one
test, which is the line WorkerStubs.kt draws.
Added RowShape, seven ways a provider can answer a metadata query. Column
granularity is deliberate -- OutputPublisher reads only SIZE, InputQuery
reads both and reaches different answers depending on which is bad -- and so
is keeping null, missing, negative and no-row distinct rather than folding
them into one "bad" case. That distinction is the whole reason InputQuery
exists: hasSpaceFor(0) is only "is there 128 MB free", so a size nobody
could determine must not arrive as 0.
No production change. OutputPublisherPublishTest, OutputPublisherStagingTest
and UnknownInputSizeTest pass unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found while decomposing #132 into children. It was item 6 there, and it
looked like the cheapest item on the list: three cold lines, a KDoc with
real user-visible stakes, and a permission Robolectric can flip in one
line.
grep -rn 'areEnabled' app/src returns the declaration and nothing else.
Both workers construct ConversionNotifications and only ever call
build(). So the behaviour the KDoc describes -- warning when progress
will be invisible -- does not happen, and a test would assert that a
function nobody calls returns what the platform told it. Green, vacuous,
and worse than nothing, because it would imply the disabled-notification
case is handled.
Recorded rather than tested, and the summary now names what F1 and F5
have in common: a comment describing behaviour the code lacks, where the
tempting fix freezes the wrong answer.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The ConversionForegroundType note asserted that #122's wedge no longer
kills the API 33 leg, on the evidence of a single run. The PR carrying
this document then wedged that exact leg: 23m08s, "wedged: yes --
gradle was killed after 1200s and never returned", failed: unknown.
Corrected to what the runs actually show: intermittent, not resolved --
five of the last six completed legs passed in ~7 minutes. And the
distinction the wedge row exists to draw is now stated, because it is
what keeps #88's reasoning intact: received: 60 means all sixty tests
still reported, so the API 33 regime was exercised; it is the failed
count that reads "unknown", so the leg could not have reported a break.
Also names what that changes -- a @Config(sdk = 33/34) JVM test is
worth three lines as insurance against a leg that cannot be trusted to
go red, which is a different and much smaller claim than the uncovered
behaviour this first looked like.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#132 holds the seven JVM test gaps from the same read, #133 the three
seam questions. The doc drew the line between them in prose already;
this makes it followable.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four things came out of re-measuring coverage that a test would document
rather than repair, so they go in a doc rather than a ticket:
- F1 FFmpegCommandBuilder emits a Vorbis encoder ContainerCapabilities'
own comment says nothing emits. Traced unreachable through four call
sites, but the interesting reading is the other one: FFmpeg can encode
Vorbis, WebM and OGG carry it, and the picker never offers it.
- F2 ConversionRequest.hardwareEncodeAvailable is written once and read
by nothing; its KDoc describes a Fast-tier preset choice that was
removed, and the router computes the same answer itself.
- F3 ConversionRequest.videoCodec/.audioCodec have no callers anywhere.
Named as NOT a test gap: asserting a delegation restates it.
- F4 Two private guards reachable only by direct call. No action, per
the judgement #88 reached about getForegroundInfo.
Also records two things the read makes look like gaps and are not: the
Compose screens' branch numbers (inflated by compiler-synthesised
recomposition checks; the line figures are 34/383 and 20/143), and
ConversionForegroundType, where #88's premise was re-checked against
#122's wedge and holds -- the API 33 leg completes 60/60 cleanly.
Entry ids are F1-F4 so they cannot be confused with defect-audit.md's
D1-D16, and the confidence vocabulary is deliberately that document's.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It has never been committed, so nothing is wrong today -- but nothing stops it
either, and `git add -A` would stage Kotlin build-session state into history.
It belongs beside /build, .gradle and .cxx, which are the same category and are
already here. Placed with them rather than in a section of its own, and left
without a comment: unlike tools/ffmpeg/out/ and .claude/, there is no non-obvious
choice here to explain.
Verified rather than assumed:
$ git check-ignore -v .kotlin
.gitignore:16:.kotlin .kotlin
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comment described adopting a later build's worker as an edge case. It is the
ordinary CI shape: the worker is found by scanning this daemon's descendants for
GradleWorkerMain, which cannot tell one invocation from the next, and the Unit
tests job runs testDebugUnitTest and jacocoTestReport back to back against one
daemon. Still harmless -- the watchdog only reads and writes -- but a reader
should not have to rediscover that.
Refs #125.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The JVM suite had no timeout of any kind, so #125's Room/WorkManager lock-order
inversion ran until something outside it gave up: 47 minutes locally, and on CI
it would burn the Unit tests job's 30-minute cap and report as a job timeout
with no cause. The deadlock is monitor contention, which no interrupt breaks, so
nothing inside the JVM could have ended it either.
The obvious fix does not work here. A JUnit `Timeout` -- as a rule or as
`@Test(timeout = ...)` -- runs the test body on a separate thread, and every Compose test in this
source set goes through Robolectric's paused main looper. Both forms fail with
"main looper can only be controlled from main thread"; the same tests with the
timeout removed pass, so it is the mechanism and not the probe.
So the bound comes from outside the test JVM, where it moves no threads:
`timeout` on the Test tasks kills the forked worker, and a watchdog jstacks that
worker two minutes earlier. The jstack is the point. Gradle's timeout on its own
kills silently, a timed-out run writes no XML for the class that hung, and the
JVM's own "Found one Java-level deadlock" section naming both monitors is the
only reason #125 could be described at all -- so it goes to stdout as well as to
a file, because the Unit tests job uploads only reports/tests/.
Ten minutes is against the slowest observed passing run, not the typical one:
eight CI samples of the whole invocation ranged 62-90s, so this is ~6.7x that
and a third of the job cap. A timeout that fires on a healthy slow runner turns
a real signal into noise.
Both numbers live in a build script that nothing compiles, so HangBoundTest
reads them back and the build script joins build.yml as a declared input --
without that the guard would go stale on exactly the edit it exists to catch.
Refs #125.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The advisory baseline check has announced a deviation on every PR since #113:
the tree "carries 4 tests marked @FailsOnEmulatorApi37" where it carries three
and FAILS_ON_EMULATOR_API37_BASELINE says three. The fourth is a KDoc in
Media3EngineTest saying the opposite -- "Deliberately not
`@FailsOnEmulatorApi37`: nothing here decodes or encodes" -- which the old
matcher counted because it looked for the string anywhere on any line.
Neither ingredient was wrong on its own, and the number is not the real damage.
#83 added this check so that a new failure joining the known ones could not be
invisible; a notice that is wrong every single time teaches everyone to skim
past deviation notices, which is precisely the signal it was built to create.
Editing the baseline to 4 would have silenced it by breaking it -- the check
would then have been wrong the moment someone added or removed a real marker.
Anchor the pattern at line start and require whitespace or end-of-line after the
name. The second half is the part that is easy to get wrong: "only the
annotation on a line of its own" also stops counting `@FailsOnEmulatorApi37
@Test`, which is legal Kotlin, and undercounting is the dangerous direction --
it hides a genuine new marker, the one thing this exists to catch. Measured
against a fixture carrying every shape at once: the old matcher 5, own-line-only
2, this one 3; on the real tree 4 / 3 / 3, so the baseline is untouched.
`grep -v import` goes too, since `^[[:space:]]*@` cannot match an import.
The check is a pure function of the working tree, so the fixture is committed
and e2e-report-shape-test.sh runs the real report against it -- inside a
throwaway repo root, which the script finds from BASH_SOURCE, so no knob had to
be added that could point the live count somewhere else. The fixture sits under
.github/, where Gradle does not compile it and :app's ktlint and detekt do not
see it; running the report against the real root with it committed still
reports 3.
Every other path through the report is byte-identical to the previous version on
both stdout and the job summary -- passing, failing, wedged, no-run, and
advisory-with-an-unreadable-baseline all diff empty -- and the two advisory legs
differ only by the false line disappearing. No job's status or pass/fail rules
change; the advisory leg stays continue-on-error and stays red by design.
The test is deliberately not wired into CI: adding a step to Static analysis
would add a new way for a gating job to go red, which #120 ruled out. shellcheck
still covers the file, since that step reads `git ls-files '*.sh'`.
Closes#120
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This PR was opened quoting 81.4%, measured on b53f326. Holding it until #116,
#117, #119, #121 and #124 merged was the point: by the time it was ready the
number had moved three points, which is the same staleness the entry is about.
Measured on 93ebfa6 with ./gradlew :app:jacocoTestReport:
LINE 1971/2321 84.9% (81.4% four hours earlier, 69.2% on 2026-08-24)
BRANCH 900/1410 63.8% (60.2%, then 53.2%)
454 JVM tests in 67 classes, all green.
The note now says the entry went stale while it was open, because that is a
better argument for the rule than the rule restating itself.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two accuracy fixes to notes the earlier commits left behind.
The test KDocs carried "roughly 1-in-130" and a 400-leg-attempt denominator.
Both come from earlier comments on #49 that its own census later replaced --
that ticket has three recorded corrections to its rate claims, and a
superseded figure in a permanent comment is the exact thing its author kept
having to fix. What survives the corrections is the count and the spread:
four occurrences, API 33, 35 and 36, every one on attempt 1 and green on
re-run.
The save exemption described a real defect with nowhere to look it up. It is
#123 now, so the KDoc names a number instead of trailing off.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The note claimed `save` is left unguarded because nothing can overwrite what
it writes. That half is true -- the only observation that could belongs to a
job already in a terminal state. The other half was missing: a save whose
copy is still in flight when the user taps Start over lands `Saved` on a
screen they have just cleared.
Guarding it would drop that write instead, which reports nothing for a file
that may genuinely have reached the destination. That is a question about
what the screen should offer during a save, and answering it in a race fix
would be deciding it by accident.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`reattach()` read `_state.value`, found it `Idle`, and then handed the job
to `observe()` -- which launches a *separate* coroutine that cannot write
until its `collect` has resumed with a `WorkInfo`. So the check happened at
one moment and the write landed at another, with a whole pick able to fit
in between: the user tapped, their metadata query suspended, the guard saw
an empty screen, and the finished job from an earlier session wrote over
`Ready(picked)` a moment later.
The comment above that guard said "no suspension point between this check
and the assignment below, so nothing can interleave". There is no
assignment below, and the two lines are in different coroutines. That
sentence is why this sat as flaky CI for two days rather than being read as
the product race it is.
`ScreenOwnership` makes the answer the test already encodes -- the user's
pick wins -- true rather than probable. A claim is taken synchronously when
the user acts; every write that lands after a suspension point checks the
claim it was made under and drops itself if that claim has been superseded.
Dropped, not reordered: a write that is dropped cannot come back later.
Cancelling the superseded observer was never enough on its own. `Job.cancel`
is honoured at the next suspension point, and a collector that has already
resumed and is on its way to `_state.value = ...` has none left; the write
lands anyway. It also cannot help at all in the case reported, where nothing
supersedes the observation until after it has been launched.
`JoinViewModel` had the identical shape and nothing watching it, so it gets
the same fix and the counterpart test that was missing. Its pick dispatcher
becomes injectable for the same reason `ConversionViewModel`'s already was:
without that seam there is no way to ask what happens while a pick is still
in flight.
Closes#49
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The report added by #111 runs on every path out of e2e-run.sh, including the
wedge, and until now it answered a question it had not been asked. On job
98035980326 -- API 34, a docs-only PR -- it printed `received: 59` and
`completed cleanly: yes` six seconds before `##[warning] ... WEDGED`, for a leg
the WEDGE_TIMEOUT had killed 22 minutes in. `completed cleanly` means only
"instrumentation was not aborted", which was true; a reader scanning the table
had to notice a separate warning line to learn the leg had died.
The wedge cannot be read out of the log, which is why it is passed in: a wedge
is gradle never returning, so gradle printed no verdict, no truncation line and
no INSTRUMENTATION_ABORTED, and the log it leaves is the log of a run that just
stops. Only e2e-run.sh saw `timeout` exit 124. It now derives that fact once and
tells the report as E2E_WEDGED_AFTER, and reuses the same variable for
capture_wedge so the two cannot drift.
The table gains a `wedged:` row above `completed cleanly`, and `completed
cleanly` flips to no -- but only where it would have said yes. An abort already
says no and names the abort, which the wedge row does not, and a run that left
no evidence still says unknown; a wedge on top of either prints both facts.
`received`'s source line told the same lie in the same table -- "the run was not
truncated, so every expected test reported" is only "gradle never got as far as
saying so" when the leg was killed -- so it is qualified on that path. The
number itself is unchanged, and so is `failed: unknown`: gradle printed no
summary line, so that count genuinely is not knowable.
Nothing here decides anything. No exit status, no pass/fail rule, no baseline
comparison and no `::notice::` behaviour changes; the leg already failed
correctly and still does.
Verified against captured CI output rather than a live emulator, as #111 was and
for the same reason -- this host cannot run API 37 and cannot wedge on demand.
Four real logs (the wedged leg, a green API 34 leg, a failing gating leg, and an
advisory leg with its baseline deviation) through both versions of the script,
in both env states, comparing stdout and the job summary: only the wedged run
with the signal set differs, byte for byte.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Refusing "copy the video" for a file that has none built its one suggestion by
hand — drop the video track and leave everything else alone. That is valid only
when the audio axis already happened to be fine. For any audio the target cannot
carry (Vorbis or PCM into MP4, MP3 into WebM) the offer is refused in the next
breath, so the Advanced picker showed a one-tap fix leading straight to a second
error. Nothing unsafe shipped — ConversionWorker re-validates — but it is a dead
end, and it contradicted the promise Validation.Invalid makes in its own KDoc.
Route it through the shared repair-and-filter path instead, as every other branch
does. Excluding what the *user* asked for rather than the already-repaired spec
is what keeps the case that worked working: an MP3 into MP4 still gets its copy
offered, because the repair of a copyable track is that same copy.
Only a branch that builds its own list can break that promise at all, since
suggestions() ends by filtering on validate().isValid. The property test now
covers both of them — this one and the image output — rather than reaching them
by luck, which is how a dead-end chip survived two earlier widenings of it. Its
failures name the probe too: three rows share a spec and differ only in the input.
Closes#114
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The KDoc claimed dynamic colour "stays switchable so users can opt back to the
brand palette". Nothing switches it: MainActivity is the only caller and passes
no arguments, so dynamicColor is always true and the two brand-palette branches
are dead. A reader who trusted that sentence would go looking for a setting that
has never existed.
Replace the claim with what is true today and point at #68, which holds the
decision -- add a switch, delete the dead branches along with the template
palette, or replace that palette first. None of the three is taken here.
ThemeKt had no test, so nothing would have caught the branches being swapped
either. Assert what the theme resolves by reading MaterialTheme.colorScheme
inside the content lambda: the two live branches on background luminance, which
is the one thing two schemes off the same device palette do not share, and the
dead pair by passing dynamicColor explicitly. Both KDocs say plainly that the
test is the only thing that passes it, so the coverage is not misread as
evidence a switch exists -- which is the misreading #68 exists to prevent.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
save()'s onFailure keeps the staged file on purpose -- it can be the only copy
of an hour of transcoding, and the destination did not receive it -- and then
handed the screen a Failed carrying a message and nothing else. That branch
rendered exactly one control: "Start over", wired to reset(), which discards
precisely the file the comment above it goes out of its way to keep. The intent
was already written down in main; the UI did not honour it, and the only rescue
was process death followed by reattach -- unadvertised, and bounded by a sweep
that collects anything a day old.
Failed now carries a PendingSave, and only where the failure came from save().
A transcode that died staged nothing and must not sprout a save button, so the
handle is nullable and the observe() arm leaves it null; so does a save that
found the file already gone. The branch renders "Try saving again" above "Start
over", opening the same CreateDocument flow with the same name and type the
first attempt used. A retry that fails again lands back on a carrying Failed
rather than a bare one, so the second failure cannot eat what the first kept.
Start over still deletes from there, and that is a decision rather than an
inheritance: deletion is the user's choice only once the alternative has been
offered. pendingStaged remains the single owner of the delete, so the carried
handle is a view of it rather than a second owner and no path out of the state
can drop a file the old shape could not.
pendingSave() exists so save() and each screen's CreateDocument registration
answer "what would a save target" once instead of twice -- the entry points
cast to Converted/Joined, which answered null for a Failed and fell back to the
current pickers, wrong for any spec edited since the job ran and for every
reattached job.
Both tabs, since JoinViewModel and JoinScreen have the same shape.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CLAUDE.md's own rule is "re-measure before quoting", and the figure it carried
was measured on 2026-08-24 -- before the #52 children, the MediaProbe and codec
tests, and the guards from #100/#107 landed. Quoting it now would understate the
suite by twelve points, which is the same failure the bullet directly below it
was written to describe.
Measured on b53f326 with ./gradlew :app:jacocoTestReport:
LINE 1847/2268 81.4% (was 1519/2194, 69.2%)
BRANCH 837/1390 60.2% (was 53.2%)
against 417 JVM tests in 60 classes, all green.
The denominator moved too, 2194 -> 2268: the same push added production code of
its own, so this is not a pure numerator gain and the note now says so. The
Robolectric/isIncludeNoLocationClasses history is left exactly as it was -- it
explains why every pre-2026-08-24 figure was an artifact, and that is still the
most useful thing in the entry. "That date" is now spelled out, since the
headline date above it has moved and the phrase no longer points at itself.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The API 37 entry names how many instrumented tests there are and how
many the gating leg runs, and this PR adds one. Nothing asserts those
figures, which is exactly why they rot quietly: 59/56 becomes 60/57.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
transcode() posts its work to a HandlerThread, and everything on that
thread has no caller to throw back to: an escaping exception reaches the
thread's uncaught handler and takes the process down, while the
continuation is never resumed. Both halves of that are bad, and the
second is arguably worse — a worker left suspended forever holds a
foreground service.
The guarding was two narrow runCatching blocks, one around
buildTransformer and one around transformer.start, with the two Media3
builders sitting unguarded between them. That gap was not theoretical.
EditedMediaItem.Builder rejects a composition with both tracks removed,
which is exactly what a plan of (Drop, Drop) asks for, and it does so
with a plain IllegalStateException from the constructor.
Validation now refuses the spec that produces such a plan, so neither
the picker nor ConversionWorker will start one. Routing is a separate
question and still answers Media3 for it — a dropped track makes nothing
un-hardware-able — so a request that skips validation still arrives
here: a job queued before the settings changed, or one made through
ConversionWorker.request directly. CopyPlanner's own KDoc already names
that path as the reason it re-checks what validation has checked; this
is the same belt for the same braces.
One guard around the whole body costs nothing on success and turns any
such refusal into a failed job with a reason attached. The export body
moves into startExport, whose contract is the thing that makes one guard
enough: returning normally means the export is running and the listener
owns the continuation, throwing means it never started and the caller
does. Cancellation is still registered before start.
Covered twice on purpose. Robolectric runs the real HandlerThread and
the real Media3 builders, so the JVM test exercises the whole sequence
and can be run anywhere; the instrumented one repeats it against the
real framework. Neither asserts only that the failure is an
IllegalStateException, because withTimeout raises
TimeoutCancellationException and java.util.concurrent.CancellationException
extends IllegalStateException — so that assertion alone calls an
unresumed continuation a pass. Both were written that way first, and
reverting the guard is what exposed it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Validation already refused two ways of asking for an empty file: None on
both codec axes, and Copy for a video track the input does not have. It
missed the third, because it read only the spec. Name H.265 with the
audio off, hand it an MP3, and the spec looks fine — it names a video
codec — while CopyPlanner drops that track anyway, because the *input*
has no video to encode. The plan is (Drop, Drop), the router still says
Media3, and EditedMediaItem.Builder refuses to build a composition with
both tracks removed. It refuses it on Transformer's own HandlerThread,
where the user sees the app die rather than a reason.
Asking the probe as well as the spec catches all three faces with one
guard, and the equivalence is exact rather than approximate: CopyPlanner
drops video for None or for an input with none, and audio for None, so
"(Drop, Drop)" and this condition are the same set. A sweep over every
non-image container by codec by codec against both probes asserts that,
so a new container or codec cannot reopen the gap on an axis nobody
wrote a case for.
This newly refuses a combination the Advanced picker accepts today, and
that is the point: today it crashes. What it must not do is refuse
without a way out. The Copy face had one only nominally — its single
hand-built suggestion was None + None, which validation rejects in the
next breath, so the one-tap fix fixed nothing. All three faces now go
through the shared repair-and-filter path, which for an MP3 into MP4
offers "copy the audio across" and nothing that has to be re-refused.
Repair is also stopped from naming a video codec for a file with no
video track. It used to fall through to the first codec the container
could encode, so the fix offered for an MP3 was "H.264" — a codec
CopyPlanner then drops, making the offer a fiction that happened to
validate.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The job summary is the deliverable #83 asked for -- "readable without opening a
log" -- and GitHub exposes no API that reads a job summary back: the check-run
output for the advisory job returns summary: null, so a write that silently did
not happen would be invisible to everything except a human on the run page. The
step log can be read, so it now carries one line saying which of the two
happened, including the case where GITHUB_STEP_SUMMARY is unset entirely, which
is what running the script by hand looks like.
"A comparison was asked for" and "a number was found to compare against" were one
variable, and collapsing them put the report one refactor away from being the
thing #83 filed. The sed that reads FAILS_ON_EMULATOR_API37_BASELINE is anchored
at the line start, so indenting the const into an object -- or renaming it, or
moving it -- empties it, and the old code then skipped the whole comparison while
the table kept printing exactly as before. Silent, and indistinguishable from a
run that matched.
Now an unreadable baseline is itself a deviation, with the notice naming the
const so the fix is obvious. Verified against the real captured log of run
32865281555 three ways: baseline file absent, const indented into an object, and
the committed file unchanged -- the first two announce, the third stays silent.
That job is continue-on-error and red on every PR by design, which CLAUDE.md
states plainly -- and that instruction is exactly why nobody reads it. Nothing in
a red X separates "the known three" from "the known three plus yours".
A bare failure count would not have fixed it, and this is measured rather than
assumed. The run is usually truncated: seven of eight advisory runs read on
2026-08-25 ended in `Test run failed to complete. Expected 3 tests, received 2.`
with INSTRUMENTATION_ABORTED, and one did not. A count taken from a truncated run
misleads in both directions -- a fourth marked test can still yield the same
number if the abort lands earlier, and the known set getting worse can lower it.
The test XML does not rescue it either, which was the thing worth checking before
building on it: it IS written for an aborted run, and it reports a tidy
tests="3" failures="3" for a run the runner had just described as truncated. So
the XML is the authority on how many results landed, the runner's own output is
the only authority on whether the run finished, and the report reads both and
says which number came from where.
The baseline is one number beside the marker, because the marker means "cannot
pass on this image": the count is both how many tests the advisory leg runs and
how many should fail. A smaller failure count is the interesting direction -- it
means one now passes, which is the documented trigger for deleting the
annotation.
Nothing about the job's status changes. It stays continue-on-error, stays red,
stays out of the required contexts; a deviation is a ::notice::, never an
::error::. The report is a separate script so it can be run against a real log
saved from a real CI run, which is how the comparison was shown to fire.
The gating legs get the shape without the comparison: they run the whole suite,
so comparing there would announce a deviation five times a run -- but a truncated
run reporting fewer results than it ran is what #108 looks like, and "completed
cleanly" is the field that would show it.
Closes#83
R29 found the discriminator claimed "exact across all seven" while r07 is recorded lower
down as "inconclusive rather than ruled out, because no evidence came back from it". A row
this page calls inconclusive cannot also be counted as evidence for the conclusion.
Checking it turned up a second instance of the same over-count, which R29 did not name. The
abort-cadence section said "Measured across the seven runs above" -- but the table records
r07's aborts as **not readable**, because adb wedged before a crash buffer could be taken.
Six runs contributed gaps, not seven.
Both now say six, and both say why. The discriminator paragraph also says what excluding r07
costs, which is nothing: it is a `host` row, so the discriminator predicts it would not boot,
and confirming a prediction with the one run whose evidence did not come back adds no
information in either direction. That is the point R29 made -- claiming six does not weaken
the conclusion -- and it is worth stating in the document rather than only in the ticket,
because the next reader will otherwise wonder whether a run was quietly dropped.
Deliberately left: "four of the seven runs show the directory creation itself is broken
during the loop". That is a count of how many runs showed something, not a claim that all
seven were readable for it, so it survives. Checked rather than assumed, and named here so
the next pass does not re-audit it.
R29's other half -- "state how r07's boot outcome was read" -- is not taken, because I do
not know and inventing a source would be worse than narrowing the claim. Narrowing is the
option R29 offered and the one that can be honest.
Closes#38.
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>
`JoinScreenContent` decides the whole join UI in one `when`, and until R38.5 gave it a state
parameter nothing could ask it anything: `Waiting` follows a denied foreground start and `Joined`
follows a finished `ConcatWorker` run, so neither is reachable by driving a real `JoinViewModel`.
`JoinScreenContentTest` used that seam to prove it exists, on one state. This is the matrix behind
it -- seven states, each pinned to the affordance it offers and the callback that affordance is
wired to, asserting on the value handed back rather than on something merely having fired.
Two of the thirteen assert things nothing else in the suite has ever asked.
The rows are read back sorted by their position on screen and compared as an ordered list. A join
is the one flow where the order of the inputs is the content of the output -- the empty state
promises "in the order you want them" -- and `JoinLeafTagsTest` proves only that a row tags itself
with the file it shows, which a reversed list would satisfy just as well.
The progress bar is asserted to be indeterminate, not merely present. It carries no percentage on
purpose, because FFmpeg reports progress against one input's duration and that means nothing across
a concatenation; the converter screen's bar is determinate, so "there is a bar" is exactly the
assertion that would let a fabricated percentage land here unnoticed.
Three mutations, each reverted after:
- `Text(s.message)` -> `Text("")` in `Failed`: "a failed join renders the message it carries" fails
with `could not find any node that satisfies: (Text + InputText + EditableText contains 'The
second file has no audio track, so joining stopped.')`.
- `when (s.strategy)` -> `when (ConcatStrategy.STREAM_COPY)` in `Joined`: "a re-encoded join says
the files differed" fails on the copy for the branch that no longer runs.
- `s.inputs.forEach` -> `s.inputs.reversed().forEach` in `Ready`: the ordering test fails
`expected:<[join.fileRow:intro.mp4, join.fileRow:middle.mp4, join.fileRow:outro.mp4]> but
was:<[join.fileRow:outro.mp4, join.fileRow:middle.mp4, join.fileRow:intro.mp4]>`.
Test-only: no file under `app/src/main` changes, and no tag is added to `TestTags`, because every
string these states render is either already tagged or unambiguous as text. The typographic
characters in the asserted copy -- U+2026 in "Joining N files...", U+2014 in the `Joined` and
Paused lines -- were checked byte-for-byte against `JoinScreen.kt` rather than retyped; an ASCII
lookalike compiles and then quietly matches nothing.
Closes#63.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The screen's state machine had a seam and no matrix behind it. Every arm of
the `when` returns `Unit`, so an arm can render anything at all and still
compile -- a button offered where it cannot work, a state's own data never
reaching the node meant to show it, an affordance wired to the wrong
callback. The leaf tests cannot see any of that: they compose `FileCard`,
`AdvancedPicker` and the three pickers directly and never hold a
`ConversionState`.
The arm worth guarding most is `Ready`'s `enabled = validation.isValid`. The
Advanced picker deliberately lets an impossible container / codec pair be
selected, so that one expression is all that stands between an invalid spec
and a job that cannot succeed. `enabled = true` compiles, renders an
identical screen apart from one colour, and passed the whole suite before
this.
Callbacks are asserted over the complete log rather than one at a time, so a
case reads "this one fired and nothing else". A bare "the callback ran" check
stays green on an arm that fires the right callback for the wrong reason.
The routing chip needed a tag to be locatable at all: its text comes from the
finished job, so a text matcher would have to name a routing explanation the
screen does not own. That is the only production change here.
Not asserted, deliberately: `Failed`'s error colour, which Compose publishes
nowhere in the semantics tree; and the three absent `FileCard`s, which are
compile-guarded -- `Idle`, `Saved` and `Failed` carry no `input` -- so those
lines state the intent without being what enforces it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ConverterScreen` and `JoinScreen` each inlined their whole `when (state)`
inside the public entry point, and state arrived only as `viewModel.state`.
That left four of the twelve state branches across the two screens with no
test that could ever reach them: driving a real ViewModel needs a WorkManager
and a media probe in the constructor, and even then `Waiting` follows a denied
foreground start and `Converted`/`Joined` follow a worker run that has already
succeeded.
So the `when` moves into `ConverterScreenContent` and `JoinScreenContent`,
which take the state, the settings, the validation and an actions holder. The
entry points keep the three launchers and `collectAsStateWithLifecycle` and
nothing else.
The callbacks travel in `ConverterActions` / `JoinActions` rather than as loose
parameters because detekt's `LongParameterList` sits at its default threshold
of six and `config/detekt/detekt.yml` does not relax it for `@Composable` --
`AdvancedPicker` already sits exactly on it. Twelve flat parameters would turn
a clean detekt run red; data classes are exempt from the rule.
Nothing else changed. The body was cut and pasted rather than retyped, so the
U+2026, U+2014 and U+00B7 characters the leaf tests match on are the same
bytes, and `is Idle -> Unit` in the nested `when` -- permanently unreachable,
and deliberately kept -- survives the move. The diff stops above `FormatPicker`
in one file and above `FileRow` in the other, which is why the leaf suites
#57-#60 landed pass unedited: every one of them composes a leaf directly and
none references either entry point.
The two new tests are the bite, one per screen and one per direction of the
seam: a `Converted` / `Joined` state renders Save, and tapping Save hands back
the name the finished job chose. The state matrix itself is #62 and #63.
The three #52 test PRs landed 56 new tests and the coverage figure moved 29.8% -> 29.7%.
That looked like the tests being worthless. It was the measurement.
Robolectric loads every class it touches through its own sandbox classloader, and those
classes arrive with no source location. JaCoCo skips no-location classes unless told
otherwise, and nothing here told it. So not one Robolectric test has ever contributed
coverage in this repo -- and Robolectric is what exercises the framework edge: both
workers, the publisher, both ViewModels, every Compose screen.
Same commit, same 335 tests, same 0 failures, only the block below added:
LINE 652/2194 29.7% -> 1519/2194 69.2%
BRANCH 425/1424 29.8% -> 758/1424 53.2%
OutputPublisher 0.0% -> 97.5% MainActivityKt 6.8% -> 86.4%
ConversionViewModel 0.0% -> 85.4% ConverterScreenKt 6.6% -> 62.8%
The discriminator, so this is not cargo cult: inside ConverterScreenKt, `describe` is the
one non-Composable and is exercised by a plain JVM test. It reported 8/8 covered while
every @Composable in the same class reported 0 -- including ones whose mutations
demonstrably failed the build when reverted. Across files the split is exactly
Robolectric-vs-not: StagingSweep, tested purely, 100%; OutputPublisher, ConversionViewModel
and FailureOutcome, tested under Robolectric, 0%.
`excludes = listOf("jdk.internal.*")` is not decoration. Without it JaCoCo walks
JDK-internal classes Robolectric has no location for either and the test JVM dies rather
than reporting a number.
CLAUDE.md's coverage bullet is rewritten, because it was wrong twice over. The figure was
an artifact, and the explanation attached to it -- that coverage fell as the suite grew
from 11 test files to 43 because the denominator outran the numerator on framework-edge
code "the JVM cannot reach" -- described a cause that does not exist. The JVM reaches that
code fine. The new tests were disproportionately Robolectric, so each one added denominator
and no numerator: the measurement was punishing precisely the tests that were hardest to
write, and the conclusion drawn from it was that writing them had not helped.
Mutation, run both ways on this branch: remove the block and jacocoTestReport collapses
back to 29.7% / 29.8%; restore it and it returns to 69.2% / 53.2%.
Two things that were true stay true. There is still no coverage gate, and a floor still
needs a settled baseline -- this one just moved 39 points in one build change. And
"re-measure before quoting it" was already written down; following it is the only reason
this was found.
CLAUDE.md carried three claims about instrumented tests. All three were false, one of
them contradicted a paragraph forty lines below it in the same file, and a subagent
working on #58 hit the contradiction and had to stop and flag it rather than trust the
project's own instructions. That is the cost being paid here: this file is what every
contributor and every agent reads first.
"Instrumented tests do not run locally" -- they do, API 33-36, since 22c7914.
"Emulators segfault on this host" -- solved 2026-08-22; it was SwiftShader's
Reactor JIT meeting SELinux execheap, not a
broken machine, and another renderer avoids
it. docs/local-emulator.md is titled
"Emulators do run on this host".
"CI's matrix therefore stops at API 36" -- the matrix has been 33/34/35/36/37 since
#56 merged, with a gating API 37 leg.
The contradiction was the worst of it. The testing-norm section added in #51 says "E2E is
runnable locally now", so the file simultaneously told you the emulator works and that it
segfaults on every AVD. A reader has no way to tell which half is current, and the wrong
half is the one that stops work: an agent that believes emulators are impossible here does
not try, and the local e2e half of the definition-of-done in #51 quietly stops being
enforceable.
The replacement says what is true now and names what is still true and why -- API 37 still
needs the manual Pixel check before a release, because the two advisory tests are the one
thing CI cannot answer for. It also states plainly that the advisory job is red on every
PR by design, which is the other thing agents keep rediscovering the hard way: three
separate subagents have now flagged that failure as possibly theirs.
The norm bullet now points at the section rather than re-arguing it, so there is one place
to correct next time rather than two that can drift apart again.
Same defect class as R14, R15, R20 and R25, all of which were documentation claims this
repo's own review falsified. The pattern is not that the docs were careless; it is that
they were written at a moment and the moment moved.
The step added in the previous commit went red on its own PR, and the reason is the one
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." Here it was not even a new rule, just a different version of the same tool.
The runner's ambient shellcheck is 0.9.0. The container used to check locally was 0.11.0.
They disagree about how to report `on_signal`, which is installed as the INT and TERM trap
eleven lines below its declaration and so is never called by name:
0.11.0 SC2329, once, on the function declaration -- "never invoked"
0.9.0 SC2317, seven times, one per command in the body -- "appears to be unreachable"
The disable directive named SC2329, so 0.11.0 was silent and 0.9.0 reported seven findings.
Nothing about the script was wrong; the local check simply was not the check CI ran.
Two changes, because either alone still leaves a way to be surprised:
- CI runs shellcheck from an image pinned by digest, so an upgrade is a line in this
file that someone chose, not something that arrives on a Tuesday. The version is
still printed, so a finding out of nowhere can be tied to that line.
- The directive names SC2317 and SC2329 both, so a contributor whose distro ships 0.9.0
gets the same answer locally as CI gives. Verified against both images: clean under
0.9.0 and clean under 0.11.0.
CLAUDE.md now says to check with the pinned digest rather than with whatever is installed,
which is what would have caught this before the push.
"Size unknown" is the line a stream fixing D5 reported as untestable. It is two
assertTextEquals calls, and it needed two rather than one: the size line renders
independently of the probe, so it is asserted with a probe and without one. That
independence is the contract, and a test of the probed case alone would leave the
branch a user hits first -- the card is on screen before the probe finishes --
unguarded.
The rest of the card degrades in words the same way, and none of it was covered:
the four InputKind branches, "No video track", "No audio track", describeVideo's
"Unknown", and the two `> 0` guards that drop the dimension and length rows
rather than printing 0 and 0:00. Each guard gets a case on both sides, because
the present side alone stays green when the guard is deleted -- what deleting it
produces is "Size: 0x0" and "Length: 0:00", the same invented-measurement defect
as "0 B".
The four pure helpers go in a plain JVM class beside it, with formatBytes pinned
at each threshold and one byte below it. A `>=` quietly becoming a `>` is only
visible from a value sitting exactly on the boundary.
Two things the issue could not have known:
- Its second acceptance criterion, "delete the return@Column and watch the
Reading... test go red", cannot happen -- it does not compile. The early return
is what smart-casts `probe` non-null, so ten uses below it fail with "Only safe
(?.) or non-null asserted (!!.) calls are allowed on a nullable receiver". The
exit is enforced by the compiler, not by a test. Both compilable regressions
someone would land instead are covered and were run red.
- CodecNames.describeAudio has no UNPARSEABLE arm, unlike describeVideo, so it
answers the raw sentinel rather than "Unrecognised". Unreachable today, because
the UNPARSEABLE kind renders the explanatory line instead of rows. Left alone;
recorded on the PR for R38.5.
The divider's absence is not asserted and cannot be: Material 3 renders it as a
Box with no semantics modifier, so it contributes no node. What is asserted is
everything it precedes, plus the card's child count. The class KDoc says so.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`AdvancedPicker` is the one leaf on the converter screen that carries its own
state, and `ValidationError` is deliberately invoked after the
`AnimatedVisibility` that gates the chip rows -- 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, it was
completely untested, and folding the two `if` blocks into one is a plausible
tidy-up that compiles.
`AdvancedPickerTest` covers the gate in both directions, clicks each of the
four colliding chip labels through its own row tag, and does every assertion
about the error card with the toggle untouched.
`AdvancedPanelSavedStateTest` is the `DestinationSaverTest` split for
`expanded`: `StateRestorationTester` saves into an in-memory map, so it proves
`rememberSaveable` is in use and nothing about the representation. Driving a
real `SaveableStateRegistry` shows the picker saves the `MutableState` itself
rather than the `Boolean`, which only survives a rotation because
`mutableStateOf` on Android returns a `Parcelable` one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two gaps, both found the same way -- by something going wrong quietly.
`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.
On 2026-08-24 eight issues filed as a scripted batch all reached the board and one filed
as a one-off minutes later did not; it surfaced only because someone went looking for it.
A batch carries the board step inside its loop. One-offs are where it slips, so
tools/github/file-issue.sh is for one-offs.
Three things it does that a two-command shell snippet would not:
- Resolves the project, Status field and option ids BY NAME, every run. Caching them
is the obvious optimisation and the wrong one -- a renamed or reordered column would
then have this writing a stale id into the board with no error anywhere.
- Reads the item back. A mutation returning 200 says the request was accepted, not that
the board shows what was asked for; the read-back is the only step that checks the
claim this script exists to make. It is a GraphQL query because REST cannot do it --
the `fields` array REST returns on a project item carries Title and nothing else, so
a REST-only check reports every item's Status as unset.
- Exits 3, loudly, with the issue number on a line of its own, when the issue was
created but the board step failed. That exact combination is the failure being
prevented; it must never be the quiet path.
Shell was the other language here with nothing checking it -- four scripts, one of them
the CI entry point. shellcheck now runs in the Static analysis job over
`git ls-files '*.sh'`, so a script added later is covered without editing the workflow,
and it runs at full severity with `info` included.
That raises two findings today and both are the tool being wrong, so both are answered
with a targeted `disable` carrying its reason rather than by lowering the severity:
run-e2e.sh's `on_signal` is reported as never invoked when it is installed as the INT and
TERM trap eleven lines below it, and the `$names` inside file-issue.sh's queries are
GraphQL variables that must not expand -- expanding them would send the shell's idea of
$owner to the API instead of declaring a parameter. A blanket --severity=warning would
have hidden both, and the next real finding with them.
The gradle step gains `if: !cancelled()` so a shellcheck failure cannot cost the
ktlint/detekt/lint lists -- the same reason that step already passes --continue.
Not covered, deliberately: shellcheck here reads .sh files, not the inline `run:` blocks
in the workflows, where a good deal of this repo's bash actually lives. actionlint does
read them, and finds one pre-existing info-level issue in build.yml. Wiring it in means
pinning a container digest, because every action here is pinned by SHA and actionlint's
usual installer is a curl-pipe-bash off a moving branch. Its own ticket, not this commit.
The format, quality and engine pickers are the same dozen lines with a
different enum substituted, and both ways they can go wrong are silent.
An onClick that closes over the picker's `selected` parameter instead of
the chip's own entry returns one constant for every chip; an inverted
`entry == selected` lights every chip but the right one. Neither throws,
neither changes the labels on screen, and a test that only asserted the
callback ran would pass over the first of them.
So each click test presses every chip in the row and compares the whole
recorded list against `entries`, which makes the constant load-bearing
rather than the click count, and each selection test asserts over every
chip rather than only the one that should be lit.
Verified by mutation, not by the suite going green:
- `onSelect(format)` -> `onSelect(OutputFormat.MP4_H264)` fails with
`expected:<[MP4_H264, MP4_H265, WEBM_VP9, ...]> but was:<[MP4_H264,
MP4_H264, MP4_H264, ...]>`
- `format == selected` -> `format != selected` fails both format
selection tests on `Selected = 'true'` for a chip that should not be
- `onSelect(preference)` -> `{}` fails with `expected:<[AUTO,
PREFER_HARDWARE, FORCE_SOFTWARE]> but was:<[]>`
- `selected.description` -> `QualityTier.FAST.description` fails the
quality prose test on the missing BEST line
Labels are read off the enums so a reword cannot redden this file for
the wrong reason. `EnginePreference` has no label of its own, so the
screen's own `label()` supplies that set. The one display literal with
no symbol behind it, the custom-spec line, was copied out of the source
byte for byte because it holds a U+2014 that would fail silently if
retyped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Kotlin `private` on a top-level declaration is file-scoped, so every leaf
composable in the two screens was invisible even to the JVM test source set,
which is a friend of main. The only three declarations src/test could name were
ConverterScreen, JoinScreen and AppRoot -- there was nothing to write a test
against, which is why #52 could not be started as filed.
`internal` is the same choice MainActivity already documents for Destination:
the unit tests can name it, and it stays invisible to anything outside the
module. Eleven declarations in ConverterScreen and FileRow in JoinScreen.
The tag table is the other half. Tests reference a symbol rather than a literal,
which is what keeps the five children that follow independent: "Cancel", "Start
over" and "Save file" are each rendered by both screens and by more than one
state branch, so rewording one would otherwise redden several PRs at once and no
diff would explain why. There were zero testTag, semantics or contentDescription
calls anywhere in main before this.
Every tag is applied inside main. A tag a test hands down as a Modifier proves
only that the test set it -- it would survive the affordance losing its own tag
entirely, which is the vacuous shape CLAUDE.md records nine of in one review.
That is why FileRow and DetailRow derive theirs from data they already hold
rather than taking an index from the call site.
TestTags is public rather than internal, and R38.8 is the reason. It reads the
table from androidTest, and whether that is a friend source set of main under
AGP 9 had no in-tree answer -- nothing referenced a main internal from there.
Settled by compiling one: it is a friend, so internal would work today. Public
anyway, because that friendship is AGP wiring rather than something this project
states, and the KDoc now carries the measurement so nobody has to repeat it.
Smoke tests cover each leaf: it renders, and its tag resolves to exactly one
node. Counting rather than asserting existence is deliberate, since a duplicated
tag fails differently depending on which finder a later test happens to use. The
state-branch tags -- Convert, Cancel, Save file, Start over, the progress bars --
have no bite yet: reaching a branch needs the state seam R38.5 extracts, and the
state matrix is R38.6/R38.7 by design.
Five mutations, all red on the named test alone: FORMAT_CHIPS deleted,
FILE_CARD_NAME deleted, detailRow's tag no longer derived from its label,
FileRow's no longer derived from its name, and JOIN_MORE given JOIN's value --
the last caught only by the uniqueness check, which is what it is for.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ConversionViewModelProbeFailureTest.an OutOfMemoryError is not swallowed`
deliberately lets a real error escape `viewModelScope.launch`, which has no
exception handler by design -- the ViewModel's KDoc says an OOM raised in the
probe should reach the thread's handler and take the process down.
On the JVM something else takes it. kotlinx-coroutines-test installs a
process-wide collector for uncaught coroutine errors, keeps whatever it catches,
and hands the backlog to the next `runTest` that starts, which throws
UncaughtExceptionsBeforeTest. Every Compose test is a `runTest`: that is how
`createComposeRule` runs a composition. So the error lands on an unrelated test
in an unrelated file, and the message names neither the test that caused it nor
the error's origin.
Nothing has hit it yet only because the sole Compose test in the repo happens to
run before the ViewModel one. R38 adds six more Compose classes in exactly the
two packages that surround it, and the first two of them made the suite fail in
two different files on two consecutive runs of identical, green code -- the
throw is on a real Dispatchers.IO thread, delivered after the state assertion
that ends the test responsible, so which class catches it is a race.
Drain it where a Compose rule is built. A @Before cannot: the rule's `runTest`
wraps the statement that calls it, so it has already thrown. @BeforeClass cannot
either, because Robolectric runs it outside the sandbox classloader, where the
collector is a different object. Constructing the rule is early enough, since
JUnit builds a fresh test-class instance -- and every @get:Rule field on it --
before evaluating any rule.
This is containment, not the cure. The cure is a seam: give the probe hop an
injectable dispatcher the way the constructor already does for
cleanupDispatcher, so the error has somewhere to land. That is a production
change and deserves its own commit.
kotlinx-coroutines-test was already on the unit-test classpath through
compose-ui-test-junit4; it is declared now because a file imports it. Pinned,
like robolectric and the linters: org.jetbrains.kotlinx is not one of the groups
the prerelease guard covers, so a float here would be free to take a milestone.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI has never run the API level this app targets. The reason it did not was
never "API 37 is untestable" -- it was that two tests fail on the emulator
image, so one row would be permanently red or permanently allow-listed. This
splits that row instead of choosing between those two.
E2E API 37 gates. It runs 55 of the suite's 57 instrumented tests and must be
green. E2E API 37 Media3 hardware transcode runs the other two, reports, and
never blocks (continue-on-error). Both are driven off ONE marker,
@FailsOnEmulatorApi37: the gating job passes notAnnotation, the advisory job
passes annotation. Two lists would drift, and drift is silent in both
directions -- a test that ends up in neither job reads as green. Excluding by
class was not an option either: Media3EngineTest has four tests and two of
them pass here, so notClass would have thrown away real coverage.
The advisory job is named for what it runs, not for what we think is wrong.
Both its tests drive a full H.264 -> H.265 hardware transcode, which is what
distinguishes them from the two Media3EngineTest cases that pass -- those
never decode video. The goldfish-decoder theory sits in a comment inside the
job, where it can be corrected without renaming a check people have learned to
look for; docs/api-37-emulator-crash.md keeps measurement and inference apart.
The SystemUI disable moves into .github/scripts/e2e-run.sh behind
E2E_DISABLE_SYSTEM_UI, unset everywhere but the two API 37 jobs, so the other
four legs run byte-identical commands -- the same shape as
E2E_EXTRA_GRADLE_ARGS. It runs BEFORE the streamed logcat starts, deliberately:
`adb shell stop` would end that logcat and nothing restarts it, so a disable
placed after it would cost the leg its diagnostics for the part of the run that
matters. The body is probe v2 from api37-debug.yml -- the version measured 4/4
-- not the older one-round form: three rounds, waits for system_server to
actually be gone, verifies against `pm list packages -d`, and requires a 45 s
window with zero new aborts. The weaker probe reported success on a run that
then started SystemUI eight more times.
The caveat is written next to the row rather than left implicit: this leg runs
with SystemUI disabled and the framework restarted under it, a device
configuration no other leg and no Pixel run uses. Anything that touches system
UI must not trust it, and the Pixel check before each release is still the only
API 37 run with SystemUI intact.
docs/api-37-emulator-crash.md's "So should CI take API 37?" said no on three
reasons. Two were claims about CI that had never been measured; the section now
carries the eight runs that measured them, and the third reason is what the
split answers. docs/local-emulator.md and api37-debug.yml's header carried the
same "the matrix stops at 36" claim and are corrected with it.
CLAUDE.md is left alone deliberately -- its "CI's matrix therefore stops at API
36" clause is now false, and that correction is parked in the doc's existing
"Correction owed to CLAUDE.md" section, where two others are already waiting.
Making E2E API 37 an actually-required check is a repository-settings change
and must come after this is on main: adding a required context that does not
exist on the default branch blocks every PR.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three additions to docs/api-37-emulator-crash.md, all from a CI investigation
run through .github/workflows/api37-debug.yml.
A third measured bullet: API 36 against API 37, back to back, same two tests,
same renderer, same SystemUI-disable path. 37.0 fails both on
c2.goldfish.h264.decoder (32660148155); 36 passes both in 4.603 s with the
same decoder in its logcat (32660152961). That falsifies "the stripped
configuration is what breaks these tests" -- a reading the other measurements
never addressed, because they all compare against a device that still had
SystemUI. It carries its two uncontrolled variables rather than dropping them:
API 36's framework restart happened with zero aborts logged where API 37's had
two, so a restart under an active abort loop is still uncontrolled; and the
images differ on the encoder side, which is a second reason "broken h264
decoder" is the wrong shape of claim.
The decoder-mechanism bullet is unchanged and still labelled inference. This
adds a measurement next to it; it does not retract anything.
The intact-SystemUI counterfactual is unmeasurable on a GitHub runner, and now
says why. Seven dispatches, zero verdicts, with a mechanism rather than bad
luck: while the framework crash-loops the guest cannot reliably create per-user
private directories, so an app installed during the loop has no cache dir and
the fixture copy dies in @Before before any codec exists. googlesdksetup and
nexuslauncher hit the same thing. The result XML masks it behind an
UninitializedPropertyAccessException in tearDown, which reads as a defect in
this repository and is not one.
Abort cadence corrected. "Roughly every 20 s" was the watchdog's sampling
interval, not the cadence: measured gaps are 20-90 s, median 60-70 s, three to
five per run, with sys.boot_completed held at 1 throughout. The wrong figure
lived in api37-debug.yml's own comments, so that line is corrected too.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four dispatches of one configuration -- API 37.0, swiftshader_indirect,
SystemUI disabled -- came back three green and one not, and the odd one out
was not a different failure so much as the same run without the fix applied.
In 32646029143 `pm disable-user` reported `new state: disabled-user` and
SystemUI then started eight more times:
14:41:24 ActivityManager: Start proc 6412:com.android.systemui ... GradientColorWallpaper
14:45:05 ActivityManager: Start proc 17299:com.android.systemui ... GradientColorWallpaper
with ten more RegionSampling aborts and a surfaceflinger pid that never sat
still (489, 1570, 3524, 4396, 6038, 7987, 9732, 11520, 13208, 15048). The
framework is being SIGKILLed every twenty seconds while this runs, so a
package-state change can go down with the system_server that accepted it.
Two things were wrong, and the second is why the first went unnoticed:
- one disable attempt was treated as sufficient
- the wait after `adb shell stop` was not a wait. It asked `service check`
0.3 s later and got `found` from the system_server that was still on its
way out, so it never waited for anything. Both the good and the bad run
printed `services back after 5 s`, which is how a broken fix looked
identical to a working one.
Now: up to three rounds of disable -> take the framework down and confirm
system_server is actually gone -> bring it back -> verify the package is in
`pm list packages -d` -> require a 45 s window with zero new aborts. Nothing
is believed because a command said so.
Also adds measure_baseline, default true. The 45 s pre-measurement is what
makes the rate comparable with the local figures, but it is 45 s of
crash-looping before the disable has to land, which is a worse starting
point than a real leg would have.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first three dispatches came back with every watchdog sample reading
`boot=? surfaceflinger=none zygote64=none dma_aborts=0`, on runs where the
device demonstrably booted and the action's own adb was working two steps
away. The watchdog was not measuring anything.
The emulator action puts platform-tools on PATH with core.addPath, which
writes GITHUB_PATH and therefore only affects LATER steps. The watchdog is
started before the action -- that is the whole point of it -- so it inherits
the runner's own PATH, where a bare `adb` is not necessarily anything. Every
call failed into `2>/dev/null` and the sampler dutifully recorded the silence
as zero.
It now resolves adb by path, preferring ANDROID_HOME, re-resolving on every
iteration in case platform-tools arrives later, and echoing the path it
settled on. The launch step prints ANDROID_HOME and `command -v adb` for the
same reason: a repeat of this failure should be one line to spot, not three
runs of quiet zeros.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
status_check.yml stops its E2E matrix at 36 and says the android-37.0 image is
why. That is established locally under -gpu host and under ANGLE, and it is not
established for CI: runners use -gpu swiftshader_indirect, and the one local
measurement of that mode was void for a local reason -- Fedora denies execheap
to SwiftShader's JIT, so the emulator died before the guest mattered. What CI
does at API 37 has therefore never actually been measured.
This is that E2E job with the matrix replaced by workflow_dispatch inputs, so a
hypothesis costs a dispatch rather than a commit: renderer, API level, image
target, channel, SystemUI disable, boot timeout, whether the suite runs at all,
and free-form emulator and Gradle arguments. It triggers on nothing else and
gates nothing.
It calls .github/scripts/e2e-run.sh rather than forking it, and pins the same
disk-size, ram-size, action SHAs and KVM setup as the job it copies, so a run
here measures the renderer and not a different device.
The watchdog is load-bearing rather than decorative. The emulator action calls
killEmulator() from its own catch block, so a run whose emulator never boots is
torn down before any script: line executes and leaves nothing behind -- which is
the exact failure shape API 37 is suspected of. It starts before the action,
samples sys.boot_completed, the surfaceflinger and zygote pids and the
hasReadColorBufferDma abort count every 20 s, and keeps a rolling copy of the
crash buffer so the last read survives the teardown.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JMR-devandClaude Opus 5