The commit below describes "a back press aimed at a picker that had closed five seconds
earlier". The timing is right and the agency is wrong, and the agency is the interesting
half. Re-read out of the same logcat:
20:59:35.686 UiDevice: Retrieving node ... [RES='android:id/button1'].
20:59:35.689 UiObject2: Clicking on (927, 2274).
20:59:36.033 MainActivity RESUMED
20:59:36.350 VRI[PickActivity]: visibilityChanged ... newVisibility=false
`aerr_wait` and `aerr_close` both missed on that iteration and `dismissASystemErrorDialog`
fell through to `android:id/button1` -- the framework's generic AlertDialog positive button,
which is on every AlertDialog on the device. It hit one inside DocumentsUI, and that is what
closed the picker. The picker did not close on its own; this class closed it.
So the destroyed Activity and #271 are one incident rather than two findings that happened
to share a trace, and the loop's shape is three iterations rather than two: iteration 2
closes the picker through `button1` and then presses back into an app that is already in
front, iteration 3 dismisses the launcher's ANR dialog and presses again, and that press
finishes MainActivity.
It also sharpens what the fix does. With the re-read, iteration 2 returns -- the app is
focused within a second of the `button1` click -- so neither of the two presses that
followed it happens at all. The previous message implied the fix caught only the last one.
`requireAReadableScreen` now drops a Boolean return value, which this codebase treats as a
smell. It is correct there -- the `device.wait` on the next line is the re-probe -- and the
call site says so rather than leaving a reader to work out whether it was an oversight.
No behaviour change beyond the comment: the fix itself is unchanged and the sweep is re-run
because `app/src` is touched.
Refs #102, #271
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`dismissThePicker` guarded its back presses on `Activity.hasWindowFocus`, which a system
app-error dialog makes false as well -- it is a fullscreen `system_server` window, which is
why `dismissASystemErrorDialog` exists at all. So the guard could not tell "the picker is
still up" from "a dialog is on top of an app that is already in front", and the loop
dismissed the dialog and then pressed back on the reading it had taken before doing so.
Measured on the API 35 gating leg of run 34161043035 attempt 1, whose head is #269's own
commit: the save picker returns at 20:59:36.033, the launcher's ANR dialog is dismissed at
20:59:41.169, a back press goes out at 20:59:41.713, the launcher is moved to the front 41 ms
later, and MainActivity is DESTROYED at 20:59:42.278. Everything after that in the test throws
`NullPointerException: Cannot run onActivity since Activity has been destroyed already`.
The fix re-reads the focus after a dialog is actually dismissed, and only then -- so it
removes a back press sent on a stale reading rather than retrying one. A picker genuinely in
front still leaves the app unfocused and still gets the press, so nothing about what this
class can catch changes; and on the ordinary path, with no dialog, nothing is re-read and
nothing is waited on. `requireAReadableScreen` twenty lines away has always re-probed after
dismissing a dialog; this is the same rule in the one place that did not follow it.
It cannot be demonstrated by re-running and the KDoc says so: the launcher ANR is ambient on
these runners and is not reproducible on demand, so a green sweep is not evidence for this.
The trace is.
`docs/ci-failure-modes.md` is the rest of it -- a census of every gating E2E leg-attempt in
the repo's history, 129 failures in 1489, classified by mode with a disposition each. It
closes out #102, whose own mode turns out to be the emulator's Codec2 HAL segfaulting: a null
dereference in `getClientUsage` inside `libcodec2_goldfish_common.so` kills
`c2.goldfish.h264.decoder`, and Media3's 25 s export watchdog then aborts the export. Six
occurrences, six carrying that crash in the same job's log, 0.5% of API 33-36 leg-attempts,
and the same vendor HAL `@FailsOnEmulatorApi37` already names.
Three counting rules are in the document and in CLAUDE.md because each was learned by getting
it wrong: count per leg-attempt rather than per run, since a re-run to green replaces the
conclusion; give every mode its own denominator, since the API 37 row filters seven tests out
and some tests are younger than the window; and capture the log before retrying, since
`gh run view --job <id> --log` resolves by run and serves the latest attempt.
Refs #102
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The determinism argument said no coroutine had a `_state` write left in
flight, because `reattach` "has either returned on its `_state.value !is Idle`
guard or found nothing". There is a third outcome: `pruneWork()` is async, so
`reattach` can find an unpruned job, pass that guard, and start an `observe()`
that is a live coroutine with writes ahead of it.
The conclusion survives, by a mechanism the paragraph did not mention.
`reattach` reads `ownership.current` before its query and hands that token to
`observe`, while `onInputPicked` calls `ownership.claim()` synchronously on the
pick -- so once a detail row exists that observation is superseded and every
emission returns at `stillHeldBy` before it writes. The claim is therefore
"every write in flight is landed or superseded", not "no other coroutine
started".
This is the KDoc a future reader opens to learn why the test cannot flake, and
CLAUDE.md records the same failure mode twice already -- E1/E3, and #226's KDoc
that described a draft rather than the code. A correct test with an incomplete
explanation is its own defect.
Comment only; no test logic changed. Committed with --no-verify on the repo
owner's explicit say-so: the gate's cache is keyed on the `app/src` tree hash,
so a comment costs a full 33-36 sweep, and CI is already green on the parent
commit. ktlintCheck, detekt and compileDebugAndroidTestKotlin were run by hand
first and pass -- those are what a comment edit can actually break.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Its two picker tests failed on roughly half of gating runs, by two measured
mechanisms. Both are removed here rather than re-tuned; the fix is in the test.
**A -- the Convert tap was lost in the post-probe relayout.**
`ConversionViewModel.onInputPicked` writes `_state` twice: name and size first,
then the probe. The second write grows the file card and moves the Convert
button. Compose computes the tap's coordinate from the semantics node and
dispatches afterwards, so a relayout in that gap hit-tests a stationary
coordinate against the new layout and the touch lands on whatever moved into
the button's place -- silently. Measured as the gap between the pick's FFprobe
closing and the tap: 319 ms and 421 ms passed; 46 ms, 98 ms and 124 ms did not.
`convertToTheDefaultFormat` now waits for the `Container` detail row before
tapping. That row is composed only under `input.probe != null`, so its presence
means both of `onInputPicked`'s writes have landed and been laid out -- and
nothing else in the ViewModel has a `_state` write in flight at that moment
(`reattach` returned on its non-Idle guard, `observe` starts inside `convert()`).
The card cannot change height again before the tap. That is a different claim
from waiting longer.
**B -- the app was not the focused window when Compose was queried.**
One failure had a 416 ms gap, so it was not A: the tap landed,
`GrantPermissionsActivity` started, back was pressed, and nothing was ever
enqueued. A back press goes to whichever window holds *input* focus, while
`Until.hasObject` answers about the accessibility tree -- which can carry the
dialog's nodes first -- so a back that arrives one window early lands on
`MainActivity` and finishes it.
`POST_NOTIFICATIONS` is now held before the tap instead of the dialog being
dismissed after it. `RequestPermission.getSynchronousResult` returns without
starting anything when the permission is already granted, so there is no
foreign window, no back press, and nothing the test injects can finish the
Activity. `@Before` asserts the grant rather than assuming it.
The class KDoc claimed granting "was tried first and did not take". Re-measured
at API 34, six consecutive runs: zero `REQUEST_PERMISSIONS` starts, zero
`GrantPermissionsActivity`, and exactly two `Scheduling work ID` lines per run
-- one per converting test, so neither tap was lost.
Also adds a fail-fast that says the Convert tap started nothing, instead of
spending the 300 s conversion budget and then naming `action.saveFile`. It is a
diagnostic, explicitly not the synchronisation.
Not fixed in production. The double write is deliberate, documented progressive
disclosure -- blocking the screen on an FFprobe process spawn reads as the app
ignoring the tap -- and a layout fix (pinning the button, reserving the card's
height) would make A less likely for one widget where waiting on the probe makes
it impossible for every tap. The ticket's argument that each added `OutputFormat`
widens A does not hold either: the format `FlowRow`'s height is fixed for a given
entry list and does not change when the probe lands. What displaces Convert is
the card growing, independent of chip count.
Mutation, run not predicted: deleting `publish`'s `if (destinationWasEmpty)
deletePartialOutput(...)` arm reddens `aFailedSaveDeletesTheDocumentItCouldNotWrite`
with "publish did not delete the document it could not write", and reddens
nothing else -- its sibling stays green, since the success path never enters
that catch.
Counts re-derived and unchanged: 72 androidTest tests, 7 markers, 65 gating,
FAILS_ON_EMULATOR_API37_BASELINE = 7. Both tests keep @FailsOnEmulatorApi37.
`NotificationCancelActionTest`'s KDoc said the suite grants no runtime
permissions; that is no longer true and it now says so.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two claims in E9 went further than the evidence, in an entry whose whole subject
is a figure that was quoted past its own.
"All 16 sat on that one line" was consistent with what was measured, not
established by it: `MediaProbe:321` accounts for a 16-branch difference and the
denominators now agree at 1338, but the other lines were never enumerated in both
reports, so a line that lost coverage elsewhere would have been invisible to the
check that was run. Says that now.
And `matroskaOrWebm` is not "the only string-literal `when` in that file" --
`shortName` at :429 is a second one. The parenthetical was never checked; it is
gone rather than repaired, since the mechanism was not what the measurement
established anyway.
The previous commit message carries the stronger wording. It is left alone rather
than rewritten, because that would need a force push.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
E8 classified the 32 lines neither suite executes and stopped there. It never
asked which *arms* neither suite takes on lines both suites run, and that tier
is where what is left actually lives.
Rebuilt the union rather than reusing E8's artifact: a fresh jacocoTestReport
merged with E8's own API 34 .ec against one set of current class files. Union is
99.0% line, 90.1% branch. Controls confirm the device half applied -- 32 lines in
FFmpegEngine, 24 in Media3Engine, 15 in ConcatEngine, 9 in MainActivity that the
JVM suite never reaches.
Three things came out of it that are worth more than the count:
- **E8's denominator artefact is retired.** It warned the union's branch
denominator ran 16 ahead "entirely inside MediaProbe" and told readers not to
quote a MediaProbe branch figure raw. Rebuilt, both denominators are 1338 and
MediaProbe:321 reads mb=0 cb=4. All 16 sat on that one line, so it was a
property of how the report was built and never of the code.
- **CLAUDE.md's "18" is not stale.** It reproduces exactly under `mi == 0 && mb > 0`
(19 branches on 18 lines) and not under `ci > 0`, which admits partially-executed
signature lines and gives 139. Different metrics, not drift -- stated in E9 so
the next read does not "correct" a figure that is right.
- **Tier 1 is 22, down from 32.** #252 closed the ten getForegroundInfo lines and
added none. Two classes carry a one-line blind spot because #252 changed their
bytecode and JaCoCo rightly rejected E8's .ec for them; the bound is measured,
not assumed, and the line is not a gap.
Of the 22 branch sites, 12 are decided here -- compiler codegen, an F4/F6/F10
exemption already on record, or a thread race -- and recorded in E9 rather than as
new F-entries, since coverage-read-findings.md is being rewritten by #261. The
other 10 are filed as #262-#266, each naming the mutation that must go red or
saying that the read is the ticket.
FFmpegCommandBuilder:183's missed arm is VORBIS, which #261 is closing as it
lands, so the set is 21 the day it merges. E9 says to re-derive rather than edit
that sentence.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`FFmpegCommandBuilder` has emitted `-c:a libvorbis` since the day it was
written, and libvorbis was not in the AAR this app ships: the configure line
omitted `--enable-libvorbis`, and `strings` on both ABIs' `libavcodec.so`
named every other external encoder and not that one. The arm was unreachable
from both ends, so nobody ever hit it -- but the first user to pick Ogg
Vorbis would have got `Unknown encoder 'libvorbis'`. That is #238's shape
again: two individually-correct facts, a builder arm and a configure line,
that no test put together, and that no coverage number can see.
So the binary is rebuilt rather than the arm rewritten. FFmpeg's in-tree
`vorbis` encoder was already in there and was tried first; it is
experimental, stereo-only, and its quality knob spans 2x its floor against
libvorbis's 6x. Shipping it would have meant `-strict experimental`, a
forced `-ac 2` that silently upmixes every mono source, and a slider with
nowhere to go. What ships instead is the arm as originally written,
`-c:a libvorbis -q:a 5`, with `OGG_VORBIS` added to the presets, `VORBIS`
added to `ENCODABLE_AUDIO`, and Ogg's per-codec extension fixed so a Vorbis
file is not named `.opus`.
The flag is `--enable-libvorbis`, read out of ffmpeg-kit's
`get_library_name()` rather than guessed: the `--enable-lame` /
`--enable-opus` rule predicts `--enable-vorbis`, and that is not it. An
unrecognised `--enable-*` is ignored silently, so the artifact was checked
before `bin/README.md` was touched -- `libvorbis` present in both ABIs, the
configure line otherwise identical, FFmpeg still n8.1.2, 10 shared libraries
per ABI, every LOAD still `0x4000`.
Both mutations were run on API 34 rather than predicted. Pointing the arm at
`libopus` reddens the e2e test with `expected:<[audio/vorbis]> but
was:<[audio/opus]>` while its `OggS` assertion still passes, which is why
the track MIME is asserted and the container magic is not enough. Adding
`-ac 2` back reddens it with `expected:<[1]> but was:<[2]>`: this class's
own fixture is mono, so mono staying mono is an assertion rather than a
claim.
The unit test's load-bearing assertion inverts with this change and is
rewritten to say so. It used to assert that `libvorbis` was *absent*; it now
asserts the encoder name plus the two flags that must not be there. Nothing
on the JVM can tell a real encoder name from a fictional one -- which is
exactly how this survived four coverage waves -- so the e2e test is the only
thing that proves the positive.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CACHE_DIR was the literal ".git/lmc-verify". In a linked worktree `.git` is a
FILE containing `gitdir: ...`, so `mkdir -p .git/lmc-verify` fails with "Not a
directory" -- and because the write is the last thing the script does, it failed
while the gate still printed green and exited 0. Every commit and push from a
worktree then re-swept API 33-36 for nothing, silently. That is the worst shape a
cache can fail in: invisible and expensive, and it was found by an agent paying
for it four times over rather than by the tool saying anything.
Measured both ways: in a worktree the old expression gives
`mkdir: cannot create directory '.git': Not a directory`, exit 1; `git rev-parse
--git-common-dir` gives the real path and exit 0.
--git-common-dir rather than --git-dir so the cache is SHARED between worktrees.
The key is the app/src tree hash, and identical content is identical content
whichever worktree produced it -- a sweep run in one is evidence for all of them.
The write also stops being silent. record_sweep() prints when it cannot record,
because a cache that never fills looks exactly like one that is working.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ConversionWorker.request` and `ConcatWorker.request` now carry
`setExpedited(RUN_AS_NON_EXPEDITED_WORK_REQUEST)`. Conversions and joins are
started by a tap; the jobs that have to go back through JobScheduler because no
process is left to start them should not queue behind a background chore. The
class KDoc that said expedited was "deliberately not used" is replaced with what
was actually read out of work-runtime 2.11.2: retries are never expedited
(`SystemJobInfoConverter:135`), and a job the system stops mid-run is resolved as
`ResetWorkerStatus` and re-enqueued rather than answered by `FailureOutcome`.
#252's own premise does not survive measurement, and that is the second half of
this change. `getForegroundInfo()` is WorkManager's expedited-work hook, but
`WorkForeground.kt:38` opens the library's only caller with
`if (!spec.expedited || Build.VERSION.SDK_INT >= 31) return`, and minSdk is 33 --
so `setExpedited` alone leaves both overrides exactly as cold as the first
instrumented coverage read found them. Measured on API 34 rather than argued:
with the flag set and `doWork` still building its own notification, both methods
report `missed 1 / covered 0` and all ten lines `ci=0`, and the whole
instrumented suite is green anyway at 71/71.
What makes them live is that each worker held two definitions of one
notification. `doWork` now posts the override's instead of an identical copy, so
`ConcatWorker`'s countless "Joining files" -- which nothing executed and which
was therefore free to drift from the "Joining N files" that ran -- is gone.
After: both `getForegroundInfo` report `LINE 0 missed / 5 covered`.
Five mutations were run and all five went red: dropping `setExpedited` from
either request, hard-coding the conversion title, moving its `percent` off zero,
and dropping the join's input count.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The other half of the hole the previous commit closed. `git ls-files '*.sh'` does
not match workflow `run:` blocks, and a good deal of this repo's bash lives
there -- so a workflow edit was still the case where the gate passed and CI's
Static analysis leg went red.
Pinned by digest, read out of status_check.yml rather than copied, for the reason
the shellcheck section gives and for actionlint's own: its documented install is
`curl | bash` off a moving branch, which does not belong in a repo that pins every
action by SHA.
The container runtime detection and the SELinux `:z` mount option are hoisted out
of the shellcheck branch so both checks share one answer rather than deciding it
twice and drifting.
Verified that it bites rather than assumed: status_check.yml was given a
`needs: [a-job-that-does-not-exist]`, and the real pre-commit hook blocked with
actionlint's own message -- `job "static-analysis" needs job
"a-job-that-does-not-exist" which does not exist in this workflow [job-needs]`.
Workflow restored; nothing but the hook is in this diff.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The gate checked ktlint, detekt and Android lint but not shellcheck, so a new or
edited .sh file was precisely the case where the hook passed and CI's Static
analysis leg still went red. The first file it could not check was itself, and it
was caught by hand twice before it was caught here.
THE DIGEST IS READ OUT OF status_check.yml RATHER THAN COPIED. shellcheck 0.9.0
and 0.11.0 disagree about how to report a trap handler -- SC2317 on seven body
lines against SC2329 once on the declaration, same script, same directive, one
red and one green. That is why CI pins by digest, and it is also why a second
copy of the digest in this file would be worse than none: when it drifts, the
symptom is the gate passing and CI failing, which is the exact failure this
section prevents.
Runs over `git ls-files '*.sh'` -- all tracked files, not the diff -- because
that is what CI does, and the job here is to predict that leg rather than audit
the change. podman is preferred over docker for the mount's SELinux relabel;
neither present, or the digest unreadable, reports the check as NOT COVERED
rather than skipping it quietly.
Verified that it bites rather than assumed: a probe script whose only fault was
an unquoted `ls $foo` was staged, and the real pre-commit hook blocked on SC2086
before it reached the JVM gate. Probe removed; all tracked .sh are clean under
the pinned digest.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The closing line was `green at every supported API level`, printed on both
paths -- including the one that had just said `NOT COVERED LOCALLY: API 37` two
lines above. A false claim, printed by the tool whose entire purpose is to stop
false claims reaching CI, on its first run.
It now names them: `green on API 33, 34, 35, 36` when the Pixel is absent, and
`green on API 33, 34, 35, 36, 37` when it is attached and passed.
Nothing else changes. The app/src subtree is untouched, so this exercises the
cache scoping from the previous commit: the sweep is skipped as already green and
only the JVM gate runs -- which is the whole reason that key was moved off the
repo tree.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
New rule, and a hook rather than a habit. Source work needs the unit tests and
the instrumented tests green at every supported API level before it is committed
or pushed; test work needs the whole suite green at every level.
tools/git-hooks/local-gate.sh is wired in as pre-commit and pre-push (symlinks,
so shellcheck sees one file), enabled with
`git config core.hooksPath tools/git-hooks`.
WHAT "EVERY LEVEL" CAN MEAN HERE, measured rather than assumed. 33-36 run the
whole suite on emulators. API 37 CANNOT be run on an emulator on this host at
all -- not "is red", cannot run: the image logs `3 new surfaceflinger aborts in
45 s (want 0)` and the APK install then fails with `Can't find service: package`,
because the framework is gone before Gradle installs anything. Starting 0 tests.
So 37 runs on the attached Pixel 10 Pro XL when it is there, and the hook says
plainly that the level is uncovered when it is not, rather than claiming five
levels having run four.
The first cut passed a notAnnotation filter through E2E_EXTRA_GRADLE_ARGS, which
run-e2e.sh:587 overwrites with --rerun -- so that argument was discarded and
would have been discarded silently.
The sweep is cached under the app/src SUBTREE hash, not the whole repo tree. The
first cut used the whole tree and that was wrong in a way that would teach people
to resent this hook: editing a comment in CLAUDE.md discarded a sweep of
byte-identical application code and re-ran forty minutes of emulators to prove
nothing. Any change under app/src still invalidates it; the JVM gate always runs.
There is deliberately no skip variable -- that would be --no-verify wearing a
different hat.
Why it is worth the time: #256 spent several gating legs learning one leg at a
time what a sweep answers in one pass, and the failing leg MOVED between runs
(API 35 red then green, API 34 green then red). One leg at a time reads as
someone else's flake; as a sweep it is one signal.
Also here, and the reason the rule arrived now: awaitNode treated "the app has no
composition right now" as a failure rather than as not-yet. fetchSemanticsNodes
throws IllegalStateException when nothing is attached and waitUntil propagates it
on the first poll instead of waiting out the deadline. This class spends much of
its time behind the picker, the save dialog and the permission dialog, so there
is always a window where the app is coming back with no composition -- and on run
34057706195's API 34 leg both SAF tests died in it. Now it is not-yet, with the
last composition error carried into the timeout message so a genuinely dead app
stays diagnosable.
Verified: this commit's own hook swept API 33, 34, 35 and 36 at 71/71 failed=0,
API 34 included.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The API 35 leg of #256 went red on aSaveWritesToTheDocumentTheSystemPickerCreated
with a 120 s ComposeTimeoutException on action.saveFile. It was not a cancelled
job and not the new teardown: run 34056545386's logcat has
20:05:26.897 FFmpegEngine: ffmpeg ... -c:v libx265 -crf 24 -preset veryfast
20:07:41.693 ConversionWorker: Routing worker_sample.mp4 ...
134.8 s between the encode starting and the next job in the suite, with no cancel
between them. The conversion was healthy and still running when the bound fired.
CONVERSION_TIMEOUT_MS was 120_000, and its KDoc justified that with "the whole
test takes 11.8 s on the API 34 CI leg" -- a real measurement generalised to an
API level it was never taken on. Adding a second picker test made this class
encode twice, so the second one runs on a more contended emulator and crossed a
line that was already marginal. Now 300_000, justified against the 134.8 s, with
a note not to re-tighten it from a fast leg's timing.
cancelAllWork was SUSPECTED of causing this and did not. A local API 35 run with
it passed, which is what sent me to the logcat. pruneWork is kept because it is
the narrower call -- only finished records need to go, and cancelling live work
is a wider blast radius than teardown in a shared process needs -- and its KDoc
now says it fixed nothing rather than claiming a cause it does not have.
That correction is the point: the first version of that KDoc asserted a cause
from one red CI leg and one green local run on a different machine. A test
carrying a confident wrong explanation is the failure mode this whole read has
been about.
Verified: API 35 at 71/71 failed=0 with the raised bound, and the full gate green.
Production is untouched -- git diff origin/main -- app/src/main is empty.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#226 proved D4's premise -- SAF hands back a document reporting exactly zero
bytes, so destinationIsKnownEmpty can answer true -- and then drove the success
path, where publish's catch is never entered. So deletePartialOutput had still
never run against a real DocumentsProvider; its only assertions were
OutputPublisherPublishTest's, against FakeSafProvider under Robolectric. That is
the same "asserted only against a fake built to match it" shape #226 was filed to
break, one layer down.
RecordingPublisher.failOpen makes openDestination return null, which publish
turns into error("Could not open destination for writing") AFTER its size probe
has run -- so the catch is reached with destinationWasEmpty true on a document
DocumentsUI created seconds earlier. Null rather than a throw because
openDestination's KDoc says a provider that is present and declines is the half
no fake can produce on demand, so that arm is also taken for the first time.
Mutation, measured: delete the deletePartialOutput call and this test fails with
"publish did not delete the document it could not write". Nothing anywhere went
red for that line before.
TWO DEAD ACCESSORS #226 LEFT, and the reason is the same one:
FixtureDocumentsProvider is declared by the test APK and runs in
org.libremediaconverter.test; instrumentation runs in the app's process. A static
in the provider is a different object from the one a test can see, so
deletedDocumentIds() would have read empty forever, and reset(File) deletes under
a filesDir that is not the provider's. Both are removed rather than worked
around. That is E7's process wall from a third side, after ACTION_OPEN_DOCUMENT
and ActivityScenario.
The oracle is the document instead, which crosses the boundary because the app
holds a URI grant for it. Still the path rather than the artefact: the size query
proves the document existed and was empty moments earlier, and one that no longer
answers a query is one something deleted.
CLEANUP IS IN TEARDOWN, and the mutation run is why. A failed save keeps its
staged file deliberately, so this test ends with a finished job for the next
launch to reattach to; its sibling then opened on Converted with no "Choose file"
to tap. The first fix tapped Start over at the end of the test body, which does
not run when the test fails -- so the mutation run turned one real failure into
two, the second looking like an unrelated flake. One cause must produce one red
test.
Baseline 6 -> 7, with the derived counts in CLAUDE.md, the marker KDoc and
status_check.yml moved in the same diff. 71 - 7 is 64, the same gating figure for
the third consecutive time, which is how that paragraph goes stale unnoticed.
Verified: three API 34 runs at 71/71 failed=0, the mutation red on the right
assertion, and the full gate plus pinned actionlint green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
E8. The instrumented suite had never been measured: enableAndroidTestCoverage was
unset, so a connected run emitted no .ec at all, and jacocoTestReport reads only
testDebugUnitTest. Measured on API 34 by setting the flag temporarily.
JVM 2236/2374 line 94.2% 1171/1338 branch 87.5%
E2E 1711/2374 line 72.1% 669/1354 branch 49.4%
UNION 2342/2374 line 98.7% 1212/1354 branch 89.5%
The JVM row reproduced the committed figure exactly, which is the control that
says both exec sets match the current class files.
The device suite closes 106 lines the JVM suite misses, and the first four are
the 81 wave 4 wrote off as native or device edges -- FFmpegEngine 32,
Media3Engine 24, ConcatEngine 15, MainActivity 10. The union leaves one. That
confirms the read's own hypothesis rather than overturning it; nobody had
measured past the boundary it named.
All 32 lines reached by neither suite were read, and none is an e2e test gap:
nine are compiler-generated, ten are getForegroundInfo() for expedited work this
app never enqueues (#252), three are F5, three are uncalled members (#253), one
is the Vorbis encode arm (#254), and four are F4-shaped error guards.
MediaProbe:210-212 gained a measurement rather than an assumption. F7 ruled
probeWithExtractor's catch unreachable because Robolectric's MediaExtractor never
throws; probeWithFFprobe calls native ffmpeg-kit, so that reasoning does not
transfer. But probe() calls both, and RemuxTest drives it with garbage bytes on a
device -- so the ffprobe path has had malformed input on real hardware and did
not throw. Same conclusion as F7, different mechanism, now on record.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review of the previous commit found three things of exactly the kind it corrects.
"Both of the advisory runs that exist" asserted exhaustiveness that had not been
checked -- two jobs were read, and the #248 branch had three status_check runs.
All four advisory runs at baseline 6 are now read: 34041156680, 34041593697,
34042397320 and 34043502322 each report expected: 6, received: 4, failed: 4,
and the only SAF test reporting in any of them is the rotation one. The claim
was right; the wording claimed more than the evidence.
CONVERSION_TIMEOUT_MS's KDoc said the bound is "two orders of magnitude" clear
of the real cost. 120 s against a measured 11.8 s is one.
status_check.yml dated the save test's marker to 2026-09-05. fa10d94 is dated
2026-09-06.
Also reworded that comment's summary: "two abort the framework and two never
report" double-counts the picker test, so a reader summing gets seven.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The 2026-09-06 re-check of the instrumented suite. Every drifted line it found
came from #226, the last PR of the e2e read's own wave.
The suite is 70 tests in 14 classes, 6 carrying @FailsOnEmulatorApi37, gating
leg 64. The committed baseline says 6 and the advisory job agrees. Four places
still said five carriers of 69:
- CLAUDE.md, three sites
- FailsOnEmulatorApi37.kt's KDoc
- two comments in status_check.yml
The gating figure is what hid it. 69 - 5 and 70 - 6 are both 64, so the one
number a reader checks against a run had not moved -- which is exactly why
CLAUDE.md says to derive these rather than remember them.
Two KDoc claims in SafPickerRoundTripTest described a draft rather than the
code. The save test says MP3 was chosen so the setup could not depend on device
codecs; the code converts at the default MP4_H265/FAST, which routes on
canEncode(H265). The negation of the stated reason was true. That is E1 and E3's
failure mode committed by the wave that found it, so it is written down as such
rather than quietly corrected.
Neither picker test has ever reported on the advisory leg. The marker's KDoc
said the picker test fails there behind the rotation test; with six carriers the
rotation test truncates the run first, and both advisory runs since #226 --
34042397320 and 34043502322 -- report expected: 6, received: 4, the four being
the three Media3 tests plus the rotation. The save test is therefore marked by
inheritance, not measurement, and both KDocs now say so.
FixtureDocumentsProvider.deletedDocumentIds() has no callers: #226 proved D4's
premise and drove only the success path, so deletePartialOutput against a real
DocumentsProvider is still asserted nowhere. Filed as #250 with the forcing
condition and the mutation; the accessor is kept with a KDoc naming that ticket
rather than removed and re-added.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Doing #226 turned up a second obstacle underneath E7's, with the same
cause. The obvious way to avoid driving the app was a host Activity in
androidTest owning its own CreateDocument launcher; it cannot be started at
all, because instrumentation runs in the target app's process and the
component is in the instrumentation one. That is the same fact as E7's
second bullet arriving from the other side, and it leaves the app's own
Save button as the only launcher available to drive.
And the answer #226 was filed for: on API 34, stock DocumentsUI hands back
a document URI reporting a size of exactly zero, so destinationIsKnownEmpty
can return true and D4's fix is live rather than inert.
A "no defect found", and not one that could have been reached by reading --
which is the argument for having done it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The save test failed an API 35 leg with "No compose hierarchies found in
the app". Dismissing the POST_NOTIFICATIONS dialog presses back and waits
for the permission UI to be gone, but going away and the app being in front
again are not the same moment, and the next Compose query landed in the gap.
Asked of UiAutomator rather than through awaitAppFocus, which is the
opposite of what this class argues for elsewhere and is right here:
awaitAppFocus goes through composeRule.waitUntil, so it would raise the very
error it is being used to avoid.
Two more local API 34 runs at 70/0/0/3.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
publish deletes a destination it could not write to -- D4's fix, so a
failed save does not leave a truncated file at the name the user chose --
but only when that destination was positively zero bytes first.
destinationIsKnownEmpty is careful that "I could not tell" never authorises
a delete, which makes the precondition load-bearing.
Until now that precondition was asserted only against a fake built to match
it: OutputPublisherPublishTest writes ByteArray(0) into FakeSafProvider
before each case, under a comment stating this is how CreateDocument
behaves. If it were false in production, D4's fix would be inert and every
existing test would still pass.
It is not false. Measured on an API 34 emulator against the real dialog:
the document SAF hands back is a document URI and reports a size of exactly
zero before anything writes to it. RecordingPublisher reads both at the
moment publish sees them, through the ConversionDependencies seam, then
lets the real copy proceed so the bytes are checked too.
This has to go through the picker, and through the app, and both are
platform constraints rather than choices. E7 in docs/e2e-read-findings.md
records the first: a DocumentsProvider is reachable only through a
picker-issued grant. The second was measured here -- a host Activity in
this source set owning its own CreateDocument launcher cannot be started at
all, because instrumentation runs in the target app's process and
ActivityScenario refuses with "Intent in process org.libremediaconverter
resolved to different process org.libremediaconverter.test". So #226 has no
cheap half, which is what its comment now says.
Three things the flow needed, each measured rather than guessed:
Both taps scroll first. On Ready the screen carries a file card, five
pickers and then the button, so Convert is below the fold; performClick on
an off-screen node dispatches where nothing is and throws nothing, while
assertIsEnabled passes either way. The first version sat waiting for a
Converted that could never come.
The format stays at its default. FixtureDocumentsProvider advertises
video/mp4 so the picker's MIME filter has a mutation with a shape, and
DocumentsUI honours that on the save side too: choosing MP3 makes the
destination audio/mpeg and the fixture root is filtered out of the save
dialog entirely.
The notification dialog is dismissed rather than pre-granted. Convert
converts from the permission callback whichever way the answer goes, so
denying is a real user's path and enough. Granting programmatically did not
take -- GrantPermissionsActivity appeared anyway and swallowed the tap.
The provider gains create, write and delete support, which it needs to be a
save target at all. It carries @FailsOnEmulatorApi37 because anything that
puts DocumentsUI on screen aborts system_server on that image, as #245
established for the other two; baseline 5 -> 6.
Verified on a local API 34 emulator: three full-suite runs at 70/0/0/3, and
a publish that writes no bytes fails it with "array lengths differed,
expected.length=58677 actual.length=0".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This PR's own advisory leg caught it. With five markers and a truncated run it
printed
failed: 4
...
baseline: matches (5 expected, 5 failed)
three lines apart. The match line has always printed the baseline twice, which
was true while `failed` had to equal it to get there -- and the previous commit
removed that requirement for truncated runs without noticing this line depended
on it.
So the truncated spelling says what happened: `matches (5 expected; 4 of 5
failed, on a run the abort truncated -- not compared)`. Pinned by a third case
beside the two from that commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#221 landed while this was in review, adding one instrumented test: 68 -> 69, and
64 on the gating leg. Its own commit was "Move CLAUDE.md's instrumented counts
with the test that changes them", and it still arrived stale -- it says three
markers and 61 tests, both true of the main it was branched from and neither true
of the main it merged into.
That is the third time these two numbers have gone stale in a day, so the
paragraph now says where they come from: a grep for @Test over app/src/androidTest
minus the marker count, cross-checkable against any run's shape, since a leg below
37 reports the first as `expected` and the API 37 gating leg reports the second.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
main landed 25 commits while this branch was open, including a third
@FailsOnEmulatorApi37 on Media3EngineTest.cancellingARunningExportStopsIt and a
batch of new instrumented tests. Every number this branch touches moved with them.
Re-derived rather than adjusted, and cross-checked against run 34020234606: the
API 34 leg (no filter) reports 68 tests and the API 37 gating leg 64, which is
68 minus main's four markers. With the picker test marked that is five markers,
baseline 5, and 63 on the gating leg.
The conflict in FailsOnEmulatorApi37.kt is resolved main's way: it had replaced
the hardcoded "grows by two" with a reference to the constant, which is the same
drift this file exists to prevent and a better fix than the number I put there.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two results from #223-#230 that belong with the read rather than only in
their own tickets.
E7 re-scoped its own ticket. #226 split into a cheap headless half and an
expensive picker-driven one, on the premise that a real DocumentsProvider
can be reached without DocumentsUI. It cannot: an unprotected one is
refused at install, instrumentation runs in the app's uid so the test APK's
own identity is no help, and adopting shell identity is denied too -- each
denial naming ACTION_OPEN_DOCUMENT as the only way in. Measured three ways.
So #226 is one item at the picker's cost, not two.
The useful half of that distinction is that the input bridge needs no
documents provider at all. getSafParameterForRead opens a descriptor
through the resolver, so any readable content:// URI exercises it, which is
what kept #225 headless.
And that is how the read's one production defect surfaced. #238: joining
files picked through the system picker failed outright on the stream-copy
path, because the concat demuxer whitelists protocols separately from
-safe 0 and ffkitsaf was not on the list. Only STREAM_COPY feeds the
demuxer a list file, and every existing join test passed Uri.fromFile, so
the one broken combination was the only one a user could reach.
Worth stating plainly next to the coverage entry: it was not a missed line
and not an unasserted value, but two covered things no test put together --
the gap shape a coverage number is worst at, and the reason the read
happened.
E4 is marked fixed; #243 made that KDoc name the constant rather than
restate it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two FFmpeg engines were done in ad2a75d and d293646. This is
Media3Engine.transcode's invokeOnCancellation, which posts
transformer.cancel() onto the engine's own HandlerThread because cancel()
has the same single-thread requirement as start().
The assertion is the output file here, where it could not be for FFmpeg.
That side deletes the partial on cancellation, and on POSIX ffmpeg keeps
writing to the unlinked inode, so the path stays gone whether or not the
cancel landed -- it asserts the session's return code instead. Media3Engine
deletes nothing, the partial being ConversionWorker's to clean up, so the
file is the evidence.
A cancelled export reports itself two ways and both mean interrupted: no
video track, or MediaExtractor refusing the file outright with "Failed to
instantiate extractor" because there is no moov atom. The first version
treated only the null as success and the exception failed the test, which
is how that was measured. Only a playable file counts as a miss.
The wait before reading is several times the export's own length, so a
cancel that did not land has certainly finished by then: the failure
direction is "the file became playable", never "we did not wait long
enough". The attempt is retried for the reason the other two engines
measured -- a 3 s 320x240 export outruns a naive cancel on a loaded runner
-- and an export that never wrote a file at all is recorded as
inconclusive rather than allowed to pass as a cancellation.
It carries @FailsOnEmulatorApi37, so FAILS_ON_EMULATOR_API37_BASELINE moves
3 -> 4 in this diff. That file also said removing the marker would grow the
gating leg "by two", which has been wrong since the third marker landed; it
now names the constant instead of restating it.
Verified on a local API 34 emulator: 68 tests, 0 failures, 3 skipped; and
with transformer.cancel() removed all five attempts produce a playable
video/hevc and the test fails, naming each one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reattachment.rank gives RUNNING the highest rank of all -- "live work
outranks a finished result because a running job is holding a foreground
service" -- and no test on either source set had ever produced one.
ReattachOnLaunchTest covers a job that finished, one whose staged file is
gone, an ambiguous pair, one still queued, and one the user cancelled.
ReattachmentTest exercises the ranking as a pure function over fabricated
snapshots. What was missing is a ViewModel meeting a real running job,
which is also the likeliest reattachment there is: the user starts a
conversion, leaves, and comes back while it is still going.
The engine is a fake, deliberately. The job has to still be running when
the ViewModel is built, and every real conversion in this suite finishes in
about a second -- racing that is what made the cancellation tests flaky
enough to need retries. A SoftwareTranscoder that blocks until released
removes the race outright. Nothing about reattachment depends on which
engine is transcoding: the tag query, Reattachment.choose over live
WorkManager state, and observe's mapping to Converting all run identically
whatever is doing the work.
This is what #230 can actually deliver, and the ticket asked for the answer
either way. Process death itself stays device-manual. D3/D13 already record
that am kill refuses a process holding a foreground service, and there is a
more basic obstacle underneath it: instrumentation runs in the app's own
process, so any route that really killed it would take the test runner with
it and leave nothing to assert with. Observing a relaunch needs two
instrumentation runs, which the runner does not provide. So the closest
observable analogue is a fresh ViewModel, with no memory of the work,
meeting a job that is genuinely mid-flight.
The teardown now resets ConversionDependencies. The suite runs without
Android Test Orchestrator, so a BlockingTranscoder left in place would hang
the next class that converts anything.
Verified on a local API 34 emulator: 67 tests, 0 failures, 3 skipped; and
making RUNNING unreattachable in Reattachment.rank fails this test and
nothing else -- which is also the evidence that the JVM ranking test was
not already covering it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Jason Ross
JMR-devandClaude Opus 5