Compare commits

..
Author SHA1 Message Date
Jason Ross 4d21996735 Merge branch 'main' into feat/expedited-conversion-work 2026-09-07 11:11:53 -05:00
Jason Ross 264b8027e4 Merge pull request #260 from JMR-dev/fix/gate-cache-in-worktrees
Resolve the gate's cache dir with --git-common-dir, and say when it cannot (#258)
2026-09-06 18:02:22 -05:00
JMR-devandClaude Opus 5 7d1d3191a9 Resolve the gate's cache dir with --git-common-dir, and say when it cannot (#258)
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>
2026-09-06 17:54:12 -05:00
JMR-devandClaude Opus 5 6a8cc01862 Expedite user-initiated work, and give both getForegroundInfo overrides a caller (#252)
`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>
2026-09-06 17:06:00 -05:00
Jason Ross 68b863fbdb Merge pull request #257 from JMR-dev/chore/gate-runs-shellcheck
Run shellcheck in the local gate, at CI's exact pin
2026-09-06 16:28:29 -05:00
JMR-devandClaude Opus 5 f4174e5b06 Run actionlint in the gate too, at CI's exact pin
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>
2026-09-06 16:19:49 -05:00
JMR-devandClaude Opus 5 dd01f9f27c Run shellcheck in the local gate, at CI's exact pin
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>
2026-09-06 16:16:23 -05:00
Jason Ross dd76229e90 Merge pull request #256 from JMR-dev/test/publish-delete-arm-real-provider
Delete the document a failed save could not write (#250)
2026-09-06 16:11:13 -05:00
JMR-devandClaude Opus 5 8105291f6a Make the gate name the levels it ran instead of claiming all of them
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>
2026-09-06 16:02:47 -05:00
JMR-devandClaude Opus 5 68bd24e54a Gate commits and pushes on a local sweep at every supported API level
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>
2026-09-06 15:52:21 -05:00
JMR-devandClaude Opus 5 19e35394e1 Bound the conversion against API 35's encode, not API 34's
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>
2026-09-06 15:20:51 -05:00
JMR-devandClaude Opus 5 cbbaf74285 Delete the document a failed save could not write (#250)
#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>
2026-09-06 14:58:40 -05:00
Jason Ross fac8e67db2 Merge pull request #255 from JMR-dev/docs/e8-instrumented-coverage
Record the first instrumented coverage measurement, and classify the 32 it found
2026-09-06 14:54:10 -05:00
JMR-devandClaude Opus 5 4de169c99b Record the first instrumented coverage measurement, and classify the 32 it found
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>
2026-09-06 14:22:24 -05:00
Jason Ross e27d7601b1 Merge pull request #251 from JMR-dev/docs/api37-carrier-count-drift
Re-derive the API 37 carrier counts, and correct what #226 left behind
2026-09-06 14:13:56 -05:00
JMR-devandClaude Opus 5 e81403c5f3 Measure the fourth claim rather than asserting it, and fix three slips
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>
2026-09-06 11:20:05 -05:00
JMR-devandClaude Opus 5 54167c052f Re-derive the API 37 carrier counts, and correct what #226 left behind
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>
2026-09-06 11:16:15 -05:00
Jason Ross 1f21557b8d Merge pull request #249 from JMR-dev/docs/e7-second-constraint
Record E7's second constraint, and that the premise held
2026-09-06 10:57:35 -05:00
JMR-devandClaude Opus 5 3b0c262030 Record E7's second constraint, and that the premise held (#226)
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>
2026-09-06 10:49:09 -05:00
Jason Ross 18aff51c98 Merge pull request #248 from JMR-dev/test/publish-to-a-real-saf-destination
Save to a document stock DocumentsUI created
2026-09-06 10:48:21 -05:00
JMR-devandClaude Opus 5 b23ff0f082 Wait for the app to come back before asking Compose about it
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>
2026-09-06 10:28:02 -05:00
JMR-devandClaude Opus 5 fa10d94192 Save to a document stock DocumentsUI created (#226)
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>
2026-09-06 10:12:37 -05:00
Jason Ross 69d5392227 Merge pull request #247 from JMR-dev/fix/api37-report-match-line
Stop the advisory match line claiming a failure count nobody measured
2026-09-06 09:11:24 -05:00
16 changed files with 1526 additions and 90 deletions
+14 -9
View File
@@ -272,13 +272,15 @@ jobs:
# has to begin after them, not between them. The name is stale and kept:
# read .github/scripts/e2e-run.sh's header, which carries the measurements.
#
# notAnnotation below keeps five tests off this row, and one of
# them is new. SafPickerRoundTripTest's PICKER test was measured on
# 2026-08-24 as passing here and was left on the leg; four gating logcats
# read on 2026-09-05 show it aborting system_server from the task-snapshot
# path on every single run, pass or fail, which is what had been failing
# unrelated PRs (#108). Both of that class's tests now carry the marker.
# docs/api-37-emulator-crash.md has the timings and the correction.
# notAnnotation below keeps seven tests off this row. SafPickerRoundTripTest's
# PICKER test was measured on 2026-08-24 as passing here and was left on the
# leg; four gating logcats read on 2026-09-05 show it aborting system_server
# from the task-snapshot path on every single run, pass or fail, which is what
# had been failing unrelated PRs (#108). All FOUR of that class's tests now
# carry the marker -- the two saves through the picker (#226, #250) joined on
# 2026-09-06 by inheritance rather than measurement, since they open the same picker.
# docs/api-37-emulator-crash.md has the timings and the correction, and
# FailsOnEmulatorApi37.kt has why the third one cannot be measured here.
#
# api-level must be a POINT release. A bare 37 is not an SDK package and
# fails during setup, which cost a run to discover. `37.0` is the choice
@@ -288,9 +290,12 @@ jobs:
# docs/api-37-emulator-crash.md measures 37.0 rev 6 and 37.1 rev 8 side
# by side, so pinning 37.0 is a decision, not a constraint.
#
# notAnnotation removes the three tests that do not pass on this image; they
# notAnnotation removes the seven tests that cannot be RUN on this image; they
# run in the advisory job below, off the same marker so they cannot end up
# in both or neither. docs/api-37-emulator-crash.md has the measurements.
# in both or neither. "Cannot be run" rather than "do not pass" is deliberate:
# four fail outright, one of those aborts the framework on its way down, and on
# the advisory leg the three picker tests behind it never report at all.
# docs/api-37-emulator-crash.md has the measurements.
- label: "37"
api-level: "37.0"
disable-system-ui: "1"
+67 -9
View File
@@ -76,12 +76,21 @@ days. Read it as the current answer, and see the git history if you need the old
`angle_indirect` and `swangle_indirect` all boot, while `auto`, `off`, `guest` and
`swiftshader_indirect` do not. `docs/local-emulator.md` has the evidence and the per-API renderer
table.
- **CI runs API 37, and it gates.** The matrix is 33/34/35/36/37. **Five** of the 69 instrumented
tests cannot be *run* on that image, for three unrelated reasons: three Media3 tests fail inside
the emulator's own `c2.goldfish.h264.decoder`, one SAF test takes the framework down when it
rotates the display, and its sibling — the SAF picker round trip — aborts `system_server` from
the task-snapshot path whether it passes or not. All five carry `@FailsOnEmulatorApi37` and run
in a separate `continue-on-error` job; the gating leg runs the other 64.
- **CI runs API 37, and it gates.** The matrix is 33/34/35/36/37. **Seven** of the 71 instrumented
tests cannot be *run* on that image, for three measured reasons and two inherited: three Media3
tests fail inside the emulator's own `c2.goldfish.h264.decoder`, one SAF test takes the framework
down when it rotates the display, and its sibling — the SAF picker round trip — aborts
`system_server` from the task-snapshot path whether it passes or not. The sixth, that class's
two saves through the picker (#226 and #250), carry the marker because they open the same picker
and a second DocumentsUI dialog on top of it — **not** because either has ever been observed here. It cannot be:
the rotation test runs first and takes the framework down, so all five advisory runs at the
previous baseline reported `expected: 6, received: 4, failed: 4`, and the four were the three
Media3 tests plus the rotation — runs 34041156680, 34041593697, 34042397320, 34043502322 and
34045105857. **No picker test has ever reported on the advisory leg**, which is a correction to
what the marker's own KDoc used to say. All seven carry `@FailsOnEmulatorApi37` and run in a
separate `continue-on-error` job; the gating leg runs the other 64 — **the same 64 for the third
time running**, which is exactly how this paragraph goes stale unnoticed: 69−5, 70−6 and 71−7
are all 64.
**These two numbers move with the suite and are derived, not remembered.** `grep -cE
'^\s*@Test' ` over `app/src/androidTest` is the first; the second is that minus the marker
@@ -114,7 +123,7 @@ days. Read it as the current answer, and see the git history if you need the old
describes everything in it. The name is kept deliberately — it is not a required context and
people have learned to look for it — so **read the marker, not the name**, for what it holds.
**It is red on every PR, by design**: do not read it as your change breaking something, and do
not read a green run as evidence those five tests pass.
not read a green run as evidence those seven tests pass.
`docs/api-37-emulator-crash.md` has the measurements.
**That instruction is also why nobody looks, so the job now reports its own shape** — expected,
@@ -134,7 +143,7 @@ days. Read it as the current answer, and see the git history if you need the old
is gradle never returning, so the log it left says nothing about it.
Still true, and the reason the advisory job is not simply deleted: **API 37 needs a manual check on
the Pixel 10 Pro XL before each release.** Those five tests are the one thing CI cannot answer
the Pixel 10 Pro XL before each release.** Those seven tests are the one thing CI cannot answer
for.
On a device or emulator, build only the ABI it can execute:
@@ -339,7 +348,7 @@ install for code that can never run — and on API 37 the full APK does not fit
when a fix is for something intermittent.
**Every number above is `testDebugUnitTest` only, and on 2026-09-05 the instrumented suite got its
first read for that reason** — `docs/e2e-read-findings.md`, entries **E1-E6**, tickets
first read for that reason** — `docs/e2e-read-findings.md`, entries **E1-E7**, tickets
**#223-#230**. Four waves had been steered by a figure that **cannot see `app/src/androidTest` at
all**, so nothing had ever asked what those 60 device tests pin, only that they were green.
@@ -378,6 +387,55 @@ install for code that can never run — and on API 37 the full APK does not fit
half. But the *input* bridge needs no documents provider at all, which is what kept #225 headless
and is how #238 surfaced.
**The 2026-09-06 re-check found that the read's own last PR had re-introduced the drift the read
was about**, and that is the entry worth carrying forward. #226 moved the suite 69 -> 70 and the
markers 5 -> 6 and changed neither the count in this file, the marker's KDoc, nor the 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 precisely why the
paragraph above says to derive these rather than remember them. Worse, two KDoc claims in the new
test described a draft rather than the code: it says MP3 was chosen so the setup could not depend
on the device's codecs, while the code converts at the default `MP4_H265`/`FAST` and therefore
routes on `canEncode(H265)` — the *negation* of the stated reason. **That is E1 and E3's failure
mode, committed by the wave that found it.** All of it is fixed; the standing item is **#250**,
because #226 proved D4's premise and never drove its delete arm.
- **Nothing is committed or pushed until the local gate is green, at every supported API level.**
Source work (`app/src/main`) needs the unit tests **and** the instrumented tests passing on every
level; test work (`app/src/test`, `app/src/androidTest`) needs the whole suite passing on every
level. `tools/git-hooks/local-gate.sh` enforces it as `pre-commit` and `pre-push`; wire it up once
with `git config core.hooksPath tools/git-hooks`.
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*: measured 2026-09-06, the image logs `3 new surfaceflinger
aborts in 45 s (want 0)` and then the APK install itself fails with `Can't find service:
package`, because the framework is gone before Gradle installs anything. `Starting 0 tests`. So
the hook runs API 37 on the **attached Pixel 10 Pro XL** when it is there, and says plainly that
the level is uncovered when it is not — CI's gating leg being what answers for it then. It never
claims five levels having run four.
**It runs shellcheck and actionlint too, at CI's exact pins** — shellcheck over
`git ls-files '*.sh'`, actionlint over the workflows, the same digests and the same file sets
that leg uses. actionlint is not an afterthought to shellcheck but the other half of the same
hole: much of this repo's bash lives in workflow `run:` blocks, which `'*.sh'` does not match at
all. That gap was found the hard way: the gate checked ktlint,
detekt and Android lint, so a new `.sh` file was precisely the case where it passed and CI still
went red, and the first file it could not check was itself. **The digest is read out of
`status_check.yml` rather than copied** — two copies drift, and the symptom of that drift is the
gate passing while CI fails, which is the one thing this check exists to prevent.
The sweep is cached under the hash of the **`app/src` subtree**, not the whole repo tree. Keying
it on the whole tree was the first cut and it was wrong: editing a comment in `CLAUDE.md` threw
away a sweep of byte-identical application code and re-ran forty minutes of emulators to prove
nothing, which is how a gate teaches people to resent it. Any change under `app/src` still
invalidates it, and the JVM gate runs unconditionally. **There is deliberately no skip
variable**, and `--no-verify` needs the repo owner's say-so each time rather than being reached
for when the gate is inconvenient.
Why it is worth tens of minutes a commit: the alternative was measured on 2026-09-06, when one PR
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 that reads as someone else's flake; as a sweep it is one signal.
- **Testable code is not done until it is tested.** If a piece is unit testable, it gets unit
tests before it counts as done. If it is e2e testable, it gets e2e tests. Both clauses apply —
a change that is both needs both.
@@ -9,14 +9,23 @@ package org.libremediaconverter
* That is the whole reason there is one annotation rather than a pair of test lists: two lists
* drift, and the drift is silent in both directions (a test that runs nowhere reads as green).
*
* **"Cannot be run" covers two things, and it said only the first until 2026-09-05.** Four of the
* five carriers simply fail: three Media3 tests die in the image's own `c2.goldfish.h264.decoder`,
* and the SAF rotation test takes the framework down with it. The fifth —
* `SafPickerRoundTripTest.pickingAFileThroughTheSystemPickerFillsInTheFileCard` — **passes about
* half the time and aborts `system_server` every time**, which is worse for a gating leg than an
* honest failure: it fails the leg from the teardown, with no failing test to point at (#108).
* The wording was widened rather than the test excused; that test's own KDoc has the four-run
* measurement.
* **"Cannot be run" covers three things now, and it covered only the first until 2026-09-05.**
* Four of the seven carriers simply fail: three Media3 tests die in the image's own
* `c2.goldfish.h264.decoder`, and the SAF rotation test takes the framework down with it. The
* fifth — `SafPickerRoundTripTest.pickingAFileThroughTheSystemPickerFillsInTheFileCard` —
* **passes about half the time and aborts `system_server` every time**, which is worse for a
* gating leg than an honest failure: it fails the leg from the teardown, with no failing test to
* point at (#108). The wording was widened rather than the test excused; that test's own KDoc has
* the four-run measurement.
*
* **The sixth and seventh are the new third thing: they are marked by inheritance, not by
* measurement.** `SafPickerRoundTripTest.aSaveWritesToTheDocumentTheSystemPickerCreated` (#226)
* and `.aFailedSaveDeletesTheDocumentItCouldNotWrite` (#250) each open the same picker and then a
* second DocumentsUI dialog on top of it, so they sit on the same task-snapshot path their sibling
* was marked for. Neither has ever been observed at API 37 either way — see the measurement under
* [FAILS_ON_EMULATOR_API37_BASELINE], which is why they cannot be. Marking them was the
* conservative choice, and **the trigger for revisiting it is the rotation test, not themselves**:
* while that one truncates the advisory run, nothing downstream of it can report.
*
* It says only what has been measured: **on the emulator, at API 37.** The same tests pass on a
* physical Pixel 10 Pro XL at API 37 and at API 33–36 on the same runner under the same renderer,
@@ -46,19 +55,21 @@ annotation class FailsOnEmulatorApi37
* keep printing with nothing to compare to, so it announces that it could not read the baseline
* rather than falling quiet. If you see that notice, this line is what it means.
*
* **One number, both checks, and that is what the marker means.** A test carrying it cannot be run
* on this image, so the count is simultaneously how many the advisory leg runs and how many fail.
* A *smaller* failure count is the interesting direction: it means one of them now passes, which
* is the trigger the KDoc above names for deleting the annotation.
* **One number, both checks, and that is what the marker was meant to mean.** A test carrying it
* cannot be run on this image, so the count is meant to be simultaneously how many the advisory
* leg runs and how many fail. A *smaller* failure count is the interesting direction: it means one
* of them now passes, which is the trigger the KDoc above names for deleting the annotation.
* **Since 2026-09-06 the second half no longer holds in practice** — the run truncates before
* three of the seven start, which the last paragraph below measures. `expected` still holds, and it is the
* field that catches a marker added without changing this number.
*
* **The picker test is the one to read that sentence carefully for.**
* `pickingAFileThroughTheSystemPickerFillsInTheFileCard` was marked on 2026-09-05 for aborting
* `system_server` rather than for failing (#108), and on the gating leg it passed two runs of
* four. It fails on the advisory leg because the rotation test runs before it and takes the
* framework down first — measured, `api37-debug.yml` run 34008889182, which reports
* `expected: 4, received: 4, failed: 4` with the four in the order Media3, Media3, rotation,
* picker. (Those dispatches predate the third Media3 marker landing on `main`, so their totals
* are four rather than five; the ordering they establish is what matters here.)
* **The picker tests are the ones to read that sentence carefully for, and the reason changed
* on 2026-09-06.** `pickingAFileThroughTheSystemPickerFillsInTheFileCard` was marked on
* 2026-09-05 for aborting `system_server` rather than for failing (#108), and on the gating leg
* it passed two runs of four. It was recorded here as *failing* on the advisory leg, behind the
* rotation test — measured, `api37-debug.yml` run 34008889182, `expected: 4, received: 4,
* failed: 4`, in the order Media3, Media3, rotation, picker. (Those dispatches predate the third
* Media3 marker, so their totals are four rather than six.)
*
* **But a second dispatch of the identical configuration reported 4/3/3**, having lost the last
* test to the abort rather than to anything about the test list, and that is why
@@ -68,6 +79,15 @@ annotation class FailsOnEmulatorApi37
* reporting fewer failures than this as one of them now passing; read a truncated one as the
* framework having died, which is this job's normal.
*
* **That is no longer what happens, and the difference is that neither picker test reports at
* all.** The rotation test truncates the run before them: **all five** advisory runs at the
* previous baseline of six — 34041156680, 34041593697, 34042397320, 34043502322 and 34045105857 —
* report `expected: 6, received: 4, failed: 4`, and the four are the three Media3 tests plus the
* rotation. #250 adds a third picker test behind the same wall, so expect `expected: 7,
* received: 4`. So the advisory leg currently answers for
* four of its six, and the comparison below is unaffected only because `failed` is not compared
* on a truncated run. Read it as **unmeasured**, not as passing or failing.
*
* So: adding or removing a [FailsOnEmulatorApi37] means changing this number, in this file, in
* the same diff. The report says so on the run itself if you forget — it prints the tree's own
* `grep` count beside this one.
@@ -78,4 +98,4 @@ annotation class FailsOnEmulatorApi37
* `INSTRUMENTATION_ABORTED`, so the count is a number taken from a partial run. The report
* records the truncation next to the counts for that reason.
*/
const val FAILS_ON_EMULATOR_API37_BASELINE = 5
const val FAILS_ON_EMULATOR_API37_BASELINE = 7
@@ -104,6 +104,17 @@ public final class FixtureDocumentsProvider extends DocumentsProvider {
private static final String ROOT_DOCUMENT_ID = "root";
private static final String FIXTURE_DOCUMENT_ID = "root/" + FIXTURE_DISPLAY_NAME;
/**
* Prefix for documents this provider CREATES, as opposed to the one it serves for reading.
*
* <p>Two namespaces rather than one so a destination can never be confused with the fixture.
* The fixture is read-only and must stay that way for the picker tests; a destination is
* writable and deletable, which is what {@code PublishToRealSafDestinationTest} needs.
*/
public static final String DESTINATION_PREFIX = "dest/";
/** Document ids {@link #deleteDocument} was called with, newest last. Cleared by {@link #reset}. */
/** Already in this source set, and already a real H.264 MP4 the engines can open. */
private static final String FIXTURE_ASSET = "sample_h264.mp4";
@@ -147,7 +158,7 @@ public final class FixtureDocumentsProvider extends DocumentsProvider {
.add(Root.COLUMN_TITLE, ROOT_TITLE)
.add(Root.COLUMN_SUMMARY, "Instrumentation fixture")
.add(Root.COLUMN_MIME_TYPES, FIXTURE_MIME_TYPE)
.add(Root.COLUMN_FLAGS, Root.FLAG_LOCAL_ONLY)
.add(Root.COLUMN_FLAGS, Root.FLAG_LOCAL_ONLY | Root.FLAG_SUPPORTS_CREATE)
.add(Root.COLUMN_ICON, android.R.drawable.ic_menu_gallery);
return cursor;
}
@@ -159,6 +170,8 @@ public final class FixtureDocumentsProvider extends DocumentsProvider {
addDirectoryRow(cursor);
} else if (FIXTURE_DOCUMENT_ID.equals(documentId)) {
addFixtureRow(cursor);
} else if (documentId != null && documentId.startsWith(DESTINATION_PREFIX)) {
addDestinationRow(cursor, documentId);
} else {
throw new FileNotFoundException("no such document: " + documentId);
}
@@ -178,10 +191,63 @@ public final class FixtureDocumentsProvider extends DocumentsProvider {
@Override
public ParcelFileDescriptor openDocument(String documentId, String mode, CancellationSignal signal)
throws FileNotFoundException {
if (!FIXTURE_DOCUMENT_ID.equals(documentId)) {
if (FIXTURE_DOCUMENT_ID.equals(documentId)) {
return ParcelFileDescriptor.open(fixtureFile(), ParcelFileDescriptor.MODE_READ_ONLY);
}
if (documentId == null || !documentId.startsWith(DESTINATION_PREFIX)) {
throw new FileNotFoundException("no such document: " + documentId);
}
return ParcelFileDescriptor.open(fixtureFile(), ParcelFileDescriptor.MODE_READ_ONLY);
int flags = "r".equals(mode)
? ParcelFileDescriptor.MODE_READ_ONLY
: ParcelFileDescriptor.MODE_READ_WRITE | ParcelFileDescriptor.MODE_TRUNCATE;
return ParcelFileDescriptor.open(destinationFile(documentId), flags);
}
/**
* Creates a real, empty file and reports the document id for it.
*
* <p><b>Empty is the whole point, and this provider does not get to decide it.</b> The premise
* under test in {@code PublishToRealSafDestinationTest} is what <i>DocumentsUI</i> hands back
* from {@code ACTION_CREATE_DOCUMENT}, and {@code OutputPublisher.destinationIsKnownEmpty}
* authorises its cleanup delete only on a positive zero. This creates the file and writes
* nothing to it, which is what the SAF contract documents; the test asserts what actually came
* back rather than trusting either side.
*/
@Override
public String createDocument(String parentDocumentId, String mimeType, String displayName)
throws FileNotFoundException {
if (!ROOT_DOCUMENT_ID.equals(parentDocumentId)) {
throw new FileNotFoundException("cannot create in: " + parentDocumentId);
}
String documentId = DESTINATION_PREFIX + displayName;
File file = destinationFile(documentId);
try {
if (!file.createNewFile() && !file.exists()) {
throw new FileNotFoundException("could not create: " + documentId);
}
} catch (IOException e) {
throw new FileNotFoundException("could not create " + documentId + ": " + e);
}
return documentId;
}
@Override
public void deleteDocument(String documentId) throws FileNotFoundException {
if (documentId == null || !documentId.startsWith(DESTINATION_PREFIX)) {
throw new FileNotFoundException("refusing to delete: " + documentId);
}
destinationFile(documentId).delete();
}
/** Removes created destinations. The process outlives one class. */
public static void reset(File filesDir) {
File dir = new File(filesDir, "destinations");
File[] children = dir.listFiles();
if (children != null) {
for (File child : children) {
child.delete();
}
}
}
private void addDirectoryRow(MatrixCursor cursor) {
@@ -189,10 +255,32 @@ public final class FixtureDocumentsProvider extends DocumentsProvider {
.add(Document.COLUMN_DOCUMENT_ID, ROOT_DOCUMENT_ID)
.add(Document.COLUMN_DISPLAY_NAME, ROOT_TITLE)
.add(Document.COLUMN_MIME_TYPE, Document.MIME_TYPE_DIR)
.add(Document.COLUMN_FLAGS, 0)
.add(Document.COLUMN_FLAGS, Document.FLAG_DIR_SUPPORTS_CREATE)
.add(Document.COLUMN_SIZE, null);
}
private void addDestinationRow(MatrixCursor cursor, String documentId) throws FileNotFoundException {
File file = destinationFile(documentId);
if (!file.exists()) {
throw new FileNotFoundException("no such document: " + documentId);
}
cursor.newRow()
.add(Document.COLUMN_DOCUMENT_ID, documentId)
.add(Document.COLUMN_DISPLAY_NAME, documentId.substring(DESTINATION_PREFIX.length()))
.add(Document.COLUMN_MIME_TYPE, FIXTURE_MIME_TYPE)
.add(Document.COLUMN_FLAGS, Document.FLAG_SUPPORTS_DELETE | Document.FLAG_SUPPORTS_WRITE)
.add(Document.COLUMN_SIZE, file.length())
.add(Document.COLUMN_LAST_MODIFIED, file.lastModified());
}
private File destinationFile(String documentId) throws FileNotFoundException {
File dir = new File(getContext().getFilesDir(), "destinations");
if (!dir.isDirectory() && !dir.mkdirs()) {
throw new FileNotFoundException("could not make the destinations directory");
}
return new File(dir, documentId.substring(DESTINATION_PREFIX.length()));
}
private void addFixtureRow(MatrixCursor cursor) throws FileNotFoundException {
File file = fixtureFile();
cursor.newRow()
@@ -1,12 +1,18 @@
package org.libremediaconverter.saf
import android.app.UiAutomation
import android.content.Context
import android.net.Uri
import android.provider.DocumentsContract
import android.provider.OpenableColumns
import androidx.compose.ui.test.ComposeTimeoutException
import androidx.compose.ui.test.assertIsEnabled
import androidx.compose.ui.test.assertTextEquals
import androidx.compose.ui.test.junit4.v2.createAndroidComposeRule
import androidx.compose.ui.test.onAllNodesWithTag
import androidx.compose.ui.test.onNodeWithTag
import androidx.compose.ui.test.performClick
import androidx.compose.ui.test.performScrollTo
import androidx.media3.common.util.UnstableApi
import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.test.platform.app.InstrumentationRegistry
@@ -19,15 +25,26 @@ import androidx.test.uiautomator.Configurator
import androidx.test.uiautomator.StaleObjectException
import androidx.test.uiautomator.UiDevice
import androidx.test.uiautomator.Until
import androidx.work.WorkManager
import org.junit.After
import org.junit.Assert.assertArrayEquals
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertNotEquals
import org.junit.Assert.assertNotNull
import org.junit.Assert.assertTrue
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.FailsOnEmulatorApi37
import org.libremediaconverter.MainActivity
import org.libremediaconverter.convert.ConversionDependencies
import org.libremediaconverter.convert.OutputPublisher
import org.libremediaconverter.ui.TestTags
import java.io.File
import java.io.OutputStream
import java.util.concurrent.atomic.AtomicInteger
import java.util.regex.Pattern
/**
* Choosing a file, through the real system picker, and still having it after a rotation.
@@ -241,12 +258,96 @@ import java.util.concurrent.atomic.AtomicInteger
* file".** That is what API 33 through 36 are for, and they answer it.
*/
@UnstableApi
/**
* Reads what SAF handed back, then publishes for real.
*
* The premise `OutputPublisher.destinationIsKnownEmpty` depends on has only ever been asserted
* 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.
* This records what stock DocumentsUI actually produced, at the moment `publish` sees it and before
* a byte is written, and then lets the real copy proceed. See #226.
*/
private class RecordingPublisher(private val app: Context) : OutputPublisher(app) {
override fun publish(staged: File, destination: Uri) {
seenDestination = destination
seenIsDocumentUri = DocumentsContract.isDocumentUri(app, destination)
seenSizeBefore = app.contentResolver
.query(destination, arrayOf(OpenableColumns.SIZE), null, null, null)
?.use { row ->
val column = row.getColumnIndex(OpenableColumns.SIZE)
if (column >= 0 && row.moveToFirst() && !row.isNull(column)) row.getLong(column) else null
}
// Read before the copy: the ViewModel deletes the staged file once publish returns.
savedBytes = staged.readBytes()
super.publish(staged, destination)
}
/**
* Refuses the write when [failOpen] is set, which is the forcing condition for #250.
*
* Returning null rather than throwing is deliberate: it is the arm `publish`'s
* `?: error("Could not open destination for writing")` exists for, and `openDestination`'s
* own KDoc says a provider that is present and declines is the half no fake can produce on
* demand. The size probe in `publish` has already run by the time this is reached, so
* `destinationWasEmpty` is true and `deletePartialOutput` is reached with the document
* genuinely empty — which is the whole point.
*/
override fun openDestination(destination: Uri): OutputStream? =
if (failOpen) null else super.openDestination(destination)
companion object {
var savedBytes: ByteArray = ByteArray(0)
var seenDestination: Uri? = null
var seenIsDocumentUri: Boolean? = null
var seenSizeBefore: Long? = null
/**
* Makes the next `publish` refuse to open its destination.
*
* A flag rather than a second publisher because `ConversionDependencies.publisher` is one
* seam and there is no orchestrator: every test in this process shares the instance the
* `init` block installed. [reset] clears it in teardown, so a test that sets it cannot
* leak a refusing publisher into the next class.
*/
var failOpen: Boolean = false
fun reset() {
savedBytes = ByteArray(0)
seenDestination = null
seenIsDocumentUri = null
seenSizeBefore = null
failOpen = false
}
}
}
@RunWith(AndroidJUnit4::class)
class SafPickerRoundTripTest {
/**
* Installs [RecordingPublisher] before the Activity exists.
*
* `ConversionViewModel` resolves its publisher through `ConversionDependencies` **at
* construction**, and the Compose rule launches `MainActivity` as part of the rule chain —
* which wraps `@Before`, so `@Before` is already too late. JUnit constructs the test instance
* before it evaluates the rules, so an initialiser is early enough, and it needs no
* `@BeforeClass` (this class's companion is private, and JUnit wants a public static there).
*
* Harmless for the other two tests: neither saves, so `publish` is never called and the
* subclass behaves exactly like `OutputPublisher`. `restoreOrientation` puts the seam back.
*/
init {
RecordingPublisher.reset()
ConversionDependencies.publisher = { RecordingPublisher(it) }
}
@get:Rule
val composeRule = createAndroidComposeRule<MainActivity>()
private val context: Context =
InstrumentationRegistry.getInstrumentation().targetContext
private val device: UiDevice =
UiDevice.getInstance(InstrumentationRegistry.getInstrumentation())
@@ -291,6 +392,10 @@ class SafPickerRoundTripTest {
*/
@After
fun restoreOrientation() {
// The suite runs without Android Test Orchestrator, so a swapped seam outlives the class.
ConversionDependencies.reset()
RecordingPublisher.reset()
clearFinishedWork()
ActivityLifecycleMonitorRegistry.getInstance().removeLifecycleCallback(recreationWatcher)
if (!rotated) return
device.setOrientationNatural()
@@ -406,6 +511,304 @@ class SafPickerRoundTripTest {
* are all warm and the only thing being waited on is one screen. That is what keeps the cost
* of a genuinely absent root bounded — see the class KDoc.
*/
/**
* The save side of SAF, end to end, against a document stock DocumentsUI created (#226).
*
* ## What this settles
*
* `publish` deletes a destination it could not write to — `docs/defect-audit.md` **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 is right, and 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 is false in production,
* D4's fix is inert and every existing test still passes.** [RecordingPublisher] reads what SAF
* actually handed over, at the moment `publish` sees it and before a byte is written.
*
* ## Why it has to go through the app, and through the picker
*
* Through the **picker** because a `DocumentsProvider` cannot be reached any other way —
* measured three ways and recorded as **E7** in `docs/e2e-read-findings.md`: an unprotected one
* is refused at install, instrumentation carries the app's uid so the test APK's own identity
* is no help, and shell identity is denied too, each denial naming `ACTION_OPEN_DOCUMENT`.
*
* Through the **app** because the same constraint sinks the obvious alternative. A host
* Activity in this source set that owns a `CreateDocument` launcher cannot be started:
* `ActivityScenario` refuses with *"Intent in process org.libremediaconverter resolved to
* different process org.libremediaconverter.test"*. Instrumentation runs in the target app's
* process, so the only Activity available to drive is the app's own — which is also the more
* faithful thing to drive.
*
* ## The conversion is setup, not subject
*
* Save is only offered on `Converted`, so the test converts first, at the screen's default
* `MP4_H265` / `FAST`. That is **not** codec-independent, and this KDoc claimed the opposite
* until 2026-09-06: an earlier draft used MP3 for exactly that reason, and the format had to
* move for a different constraint the picker imposes — [convertToTheDefaultFormat] has it.
* `MP4_H265` at `FAST` reaches `ConversionRouter`'s `canEncode(H265)` gate, so it runs on
* FFmpeg on the emulators (no hardware H265) and on Media3 on the Pixel.
*
* **That is tolerable here, and #223 is the reason it needs saying.** There, the routing
* decided whether the *subject* was reached, so a route to FFmpeg made the test pass while
* proving nothing. Here the conversion is setup: if it goes the other way and fails, this test
* fails loudly on the setup rather than quietly on the assertion. The subject is what `publish`
* was handed, which the engine that produced the file does not touch.
*
* ## Why it carries [FailsOnEmulatorApi37]
*
* By inheritance, not measurement. It opens the same picker as
* [pickingAFileThroughTheSystemPickerFillsInTheFileCard], which was marked for aborting
* `system_server` from the task-snapshot path (#108), and then a second DocumentsUI dialog on
* top of it. It has never been observed at API 37 either way: the rotation test truncates the
* advisory run first, so all four advisory runs at this baseline report
* `expected: 6, received: 4` without reaching either picker test. Marking it was the conservative choice and it is
* recorded as unmeasured in `FailsOnEmulatorApi37.kt` rather than dressed up as a measurement.
*/
@Test
@FailsOnEmulatorApi37
fun aSaveWritesToTheDocumentTheSystemPickerCreated() {
pickTheFixture()
convertToTheDefaultFormat()
saveThroughTheSystemPicker()
val destination = RecordingPublisher.seenDestination
assertNotNull("publish was never reached, so nothing was saved", destination)
assertTrue(
"SAF handed back something that is not a document URI, so publish's cleanup can " +
"never run and D4's fix is inert: $destination",
RecordingPublisher.seenIsDocumentUri == true,
)
assertEquals(
"SAF handed back a document that is not positively empty, so " +
"destinationIsKnownEmpty answers false and a failed save keeps its partial file",
0L,
RecordingPublisher.seenSizeBefore,
)
// And the bytes really arrived, which only the failure side was covered for on a device.
val staged = File(context.cacheDir, "conversions")
assertArrayEquals(
"the destination did not receive what was staged",
RecordingPublisher.savedBytes,
context.contentResolver.openInputStream(destination!!)!!.use { it.readBytes() },
)
assertTrue("staging should be empty after a successful save", staged.listFiles().isNullOrEmpty())
}
/**
* The other half of D4 (#250): a save that fails deletes the document it could not write.
*
* ## Why this is separate from the test above
*
* #226 proved the *premise* — SAF hands back a document reporting exactly zero bytes, so
* `destinationIsKnownEmpty` can answer true — and then drove the success path, where the
* `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.
*
* ## The forcing condition, and why it is a returned null
*
* [RecordingPublisher.failOpen] makes `openDestination` return null. `publish` turns that into
* `error("Could not open destination for writing")` **after** its size probe has already run,
* so the `catch` is reached with `destinationWasEmpty == true` on a document DocumentsUI
* created seconds earlier. Nothing is simulated: the URI, the grant, the provider and the
* delete are all real.
*
* 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 this is also the first time that
* arm has been taken against a live provider rather than a stub.
*
* ## The oracle, and why it is not a recorder inside the provider
*
* The obvious assertion — have the provider record what `deleteDocument` was called with, and
* read it back — **cannot work here, and finding that out is half of what this test cost.**
* `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 therefore a different object from the one a test can see, and the accessor #226
* left behind read empty on every run. That is E7's process wall from a third side, after
* `ACTION_OPEN_DOCUMENT` and `ActivityScenario`.
*
* So the oracle is the document, which does cross the boundary because the app holds a URI
* grant for it. **This is still the path rather than the artefact**, because the two
* assertions are read together: the size query above proves the document *existed and was
* empty* moments earlier, and a `content://` document that no longer answers a query is one
* something deleted. Nothing else in the app deletes SAF documents.
*
* The staged file is asserted to **survive**, which is the deliberate other half of that
* `catch`: a failed save may leave the staged copy as the only copy of an hour of transcoding,
* so `ConversionViewModel` keeps it and puts "Try saving again" on screen.
*/
@Test
@FailsOnEmulatorApi37
fun aFailedSaveDeletesTheDocumentItCouldNotWrite() {
pickTheFixture()
convertToTheDefaultFormat()
RecordingPublisher.failOpen = true
saveThroughTheSystemPicker(settlesOn = TestTags.RETRY_SAVE)
val destination = RecordingPublisher.seenDestination
assertNotNull("publish was never reached, so the delete arm was not exercised", destination)
assertEquals(
"the document was not positively empty, so publish would refuse to delete it",
0L,
RecordingPublisher.seenSizeBefore,
)
assertFalse(
"publish did not delete the document it could not write: $destination",
documentStillExists(destination!!),
)
// The staged copy is kept on purpose -- see ConversionViewModel.save's onFailure.
val staged = File(context.cacheDir, "conversions")
assertTrue(
"a failed save must not delete the staged file; it may be the only copy",
staged.listFiles()?.isNotEmpty() == true,
)
}
/**
* Leaves nothing for the next test's launch to reattach to.
*
* **In teardown rather than at the end of a test, and that placement is the point.**
* `aFailedSaveDeletesTheDocumentItCouldNotWrite` proves that a failed save *keeps* its staged
* file — deliberately, since it may be the only copy — so it ends with a finished job and a
* live staged file, which is exactly what the app reattaches to on the next launch. Its
* sibling then opened on `Converted` with no "Choose file" to tap: measured, as a 30 s timeout
* on `converter.chooseFile` in a test that had nothing wrong with it.
*
* The first fix tapped "Start over" at the end of the test body. That works until the test
* fails, and then it does not run at all — measured too, on the mutation run that proved this
* suite bites: one real failure became two, and the second looked like an unrelated flake.
* **One cause must produce one red test**, so the cleanup belongs where it runs either way.
*
* **`pruneWork` and not `cancelAllWork`, on design grounds and not on a measurement.** Only
* finished work records need to go — that is all the next launch reattaches to — and
* `cancelAllWork` additionally cancels live work, which is a wider blast radius than teardown
* in a shared process needs. `pruneWork` cannot touch a job that has not run yet.
*
* `cancelAllWork` was **suspected** of causing an API 35 red here and did not cause it; see
* [CONVERSION_TIMEOUT_MS], which did. A local API 35 run with `cancelAllWork` passed, and the
* logcat showed the conversion encoding rather than cancelled. The narrower call is kept
* because it is the right one, not because it fixed anything.
*/
private fun clearFinishedWork() {
WorkManager.getInstance(context).pruneWork()
File(context.cacheDir, "conversions").listFiles()?.forEach { it.delete() }
}
/** Whether [destination] still answers a metadata query. A deleted document does not. */
private fun documentStillExists(destination: Uri): Boolean = runCatching {
context.contentResolver
.query(destination, arrayOf(OpenableColumns.SIZE), null, null, null)
?.use { it.moveToFirst() } ?: false
}.getOrDefault(false)
/**
* Runs the conversion, leaving the screen on `Converted`.
*
* **The format is left at its default, and that is a constraint rather than laziness.**
* `ConverterScreen` registers `CreateDocument` with the *output's* MIME type, and
* [FixtureDocumentsProvider] advertises `Root.COLUMN_MIME_TYPES` of `video/mp4` — deliberately,
* so the picker's MIME filter has a mutation with a shape. DocumentsUI honours that on the save
* side too: choosing MP3 makes the destination type `audio/mpeg`, and the fixture root is then
* filtered out of the save dialog entirely. Measured, as *"the create-document dialog never
* showed LMC R38 fixtures"*. The default `MP4_H265` produces `video/mp4` and the root is
* offered.
*
* **The notification dialog is dismissed rather than pre-granted, and that is the honest
* version.** Convert never calls `convert()` directly — it launches `RequestPermission` for
* `POST_NOTIFICATIONS` and converts from the callback **whichever way the answer goes**. So the
* dialog only has to be got out of the way; denying it is a real user's path and the conversion
* still runs. Granting it programmatically was tried first and did not take —
* `GrantPermissionsActivity` appeared anyway, the click that followed went to it rather than to
* the app, and the screen sat in `Ready` with nothing enqueued.
*
* **Both taps scroll first.** On `Ready` the screen carries a file card, five pickers and then
* the button, so Convert is below the fold on a phone. `performClick` on an off-screen node
* dispatches at a position that hits nothing and throws nothing, and `assertIsEnabled` passes
* either way — the first version of this sat waiting for a `Converted` that could never come.
*/
private fun convertToTheDefaultFormat() {
composeRule.onNodeWithTag(TestTags.Converter.CONVERT)
.performScrollTo()
.assertIsEnabled()
.performClick()
dismissThePermissionDialog()
awaitNode(TestTags.SAVE_FILE, CONVERSION_TIMEOUT_MS)
}
/**
* Gets the `POST_NOTIFICATIONS` dialog out of the way, if this device shows one.
*
* Backing out of it is a denial, and a denial is fine here: the conversion starts either way,
* and what that costs the user is a progress notification confined to the Task Manager. Waiting
* only briefly, because on a device where the permission is already held no dialog appears at
* all and the conversion is already under way.
*/
private fun dismissThePermissionDialog() {
if (device.wait(Until.hasObject(By.pkg(PERMISSION_UI_PACKAGE)), PERMISSION_DIALOG_MS) != true) {
return
}
device.pressBack()
device.wait(Until.gone(By.pkg(PERMISSION_UI_PACKAGE)), PERMISSION_DIALOG_MS)
// And wait for the app to be in front again before anything asks Compose about it.
// Querying while another window still owns the screen raises "No compose hierarchies found
// in the app", which is what this test did on an API 35 leg: the back press had landed but
// the dialog had not finished going away.
//
// Asked of UiAutomator rather than through awaitAppFocus, which is the opposite of what the
// class KDoc 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.
device.wait(Until.hasObject(By.pkg(context.packageName)), FOCUS_TIMEOUT_MS)
}
/**
* Taps Save and drives the create-document dialog into the fixture root.
*
* Retried whole, for the reason [pickTheFixture] documents: a dialog that came up unreadable
* cannot be recovered from inside, and a fresh one is the only answer.
*/
private fun saveThroughTheSystemPicker(settlesOn: String = TestTags.Converter.CONVERT_ANOTHER) {
var missing: BySelector? = null
repeat(PICK_ATTEMPTS) { attempt ->
requireAReadableScreen()
composeRule.onNodeWithTag(TestTags.SAVE_FILE).performClick()
missing = walkTheSaveDialog(
if (attempt == 0) PICKER_TIMEOUT_MS else REOPENED_TIMEOUT_MS,
)
if (missing == null) {
// The node that says the save has *finished*, either way. Waiting on the success
// one when the save is meant to fail would time out on a test that is working.
awaitNode(settlesOn, SAVE_TIMEOUT_MS)
return
}
dismissThePicker()
}
throw AssertionError(
"the create-document dialog never showed $missing, in $PICK_ATTEMPTS separate " +
"dialogs (the last one left ${device.currentPackageName} in front)",
)
}
/** Into the fixture root, then Save. Returns the selector never found, or null. */
private fun walkTheSaveDialog(timeoutMs: Long): BySelector? {
val picker = By.pkg(DOCUMENTS_UI_PACKAGE)
val root = By.text(FixtureDocumentsProvider.ROOT_TITLE)
return when {
device.wait(Until.hasObject(picker), timeoutMs) != true -> picker
!tapPickerNode(root, timeoutMs, ifAbsent = ::openTheRootsDrawer) -> root
!tapPickerNode(SAVE_BUTTON, timeoutMs) -> SAVE_BUTTON
else -> null
}
}
private fun pickTheFixture() {
var missing: BySelector? = null
repeat(PICK_ATTEMPTS) { attempt ->
@@ -795,9 +1198,35 @@ class SafPickerRoundTripTest {
}
}
private fun awaitNode(tag: String) {
composeRule.waitUntil("a node tagged $tag exists", APP_TIMEOUT_MS) {
composeRule.onAllNodesWithTag(tag).fetchSemanticsNodes().isNotEmpty()
/**
* Waits for [tag], treating "the app has no composition right now" as *not yet* rather than
* as a failure.
*
* `fetchSemanticsNodes` **throws** `IllegalStateException: No compose hierarchies found in the
* app` when nothing is attached at that instant, and `waitUntil` propagates it on the first
* poll instead of waiting out the deadline. This class spends much of its time with another
* app in front — the picker, the create-document dialog, the permission dialog — so there is
* always a window where the app is coming back and has no composition yet. Before this, that
* window was a hard failure: measured on the API 34 leg of run 34057196628, where **both** SAF
* tests died that way while the same commit passed API 33, 35, 36 and 37, and the previous
* commit passed API 34 and failed 35. A failing leg that moves between runs is #190's
* emulator flake, and this is the one place in the class that turned it into a red test.
*
* **The cost is honest and bounded**: an app that is genuinely gone now fails at the deadline
* rather than immediately, so the last composition error is carried into the message to keep
* that case diagnosable.
*/
private fun awaitNode(tag: String, timeoutMs: Long = APP_TIMEOUT_MS) {
var lastError: Throwable? = null
try {
composeRule.waitUntil("a node tagged $tag exists", timeoutMs) {
runCatching { composeRule.onAllNodesWithTag(tag).fetchSemanticsNodes().isNotEmpty() }
.onFailure { lastError = it }
.getOrDefault(false)
}
} catch (timeout: ComposeTimeoutException) {
val note = lastError?.let { "; last composition error: ${it.message}" } ?: ""
throw AssertionError("waited ${timeoutMs}ms for a node tagged $tag$note", timeout)
}
}
@@ -811,6 +1240,39 @@ class SafPickerRoundTripTest {
const val PICKER_TIMEOUT_MS = 30_000L
const val APP_TIMEOUT_MS = 30_000L
/** The runtime-permission dialog's package, so it can be recognised and dismissed. */
const val PERMISSION_UI_PACKAGE = "com.google.android.permissioncontroller"
/** Short: either the dialog is up almost immediately, or the permission was already held. */
const val PERMISSION_DIALOG_MS = 5_000L
/**
* Bounds a hang, and **the first number here was measured on one API level and wrong on
* another.** It read 120 s, on the strength of the whole test taking 11.8 s on the API 34
* CI leg (run 34043502322). API 35 is a different machine: on run 34056545386 the fixture's
* `libx265 -crf 24 -preset veryfast` encode started at `20:05:26.897` and the next job in
* the suite did not appear until `20:07:41.693` — **134.8 s**, so the encode was still
* running when the 120 s bound expired and the test failed with the conversion healthy.
*
* The logcat is what settles it: `ConversionWorker` logs the route and `FFmpegEngine` the
* command, and there is no cancel between them. A timeout that fires on a working
* conversion is worse than no bound, because it reads as a product failure.
*
* 300 s is chosen against that 134.8 s, not against API 34's 11.8 s. **Do not re-tighten
* it from a fast leg's timing** — the encode is software on every emulator here, and the
* spread between images is larger than any margin a single measurement would suggest.
*/
const val CONVERSION_TIMEOUT_MS = 300_000L
/** The copy is a few kilobytes, but it crosses a provider. */
const val SAVE_TIMEOUT_MS = 30_000L
/**
* DocumentsUI's save button. Case-insensitive because the label is "SAVE" on some images
* and "Save" on others, and the difference is not what this test is about.
*/
val SAVE_BUTTON: BySelector = By.text(Pattern.compile("save", Pattern.CASE_INSENSITIVE))
/**
* The same wait once a picker has already come and gone, and shorter for a reason.
*
@@ -8,6 +8,7 @@ import androidx.work.CoroutineWorker
import androidx.work.Data
import androidx.work.ForegroundInfo
import androidx.work.OneTimeWorkRequestBuilder
import androidx.work.OutOfQuotaPolicy
import androidx.work.WorkerParameters
import androidx.work.hasKeyWithValueOfType
import androidx.work.workDataOf
@@ -27,6 +28,10 @@ import org.libremediaconverter.model.OutputFormat
* Progress is not reported. FFmpeg's statistics callback gives a timestamp against a
* single input's duration, which is meaningless once several files are being
* concatenated; showing a fabricated percentage would be worse than showing none.
*
* Enqueued as **expedited** work for the same reasons, and with the same caveats, as
* [ConversionWorker] — its class KDoc carries both, and a join is user-initiated in exactly the
* way a conversion is.
*/
@UnstableApi
class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker(context, params) {
@@ -68,13 +73,10 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker
// which is where a WorkManager restart after process death always begins -- used to
// throw straight past this catch, taking the retry, the error message and the delete
// with it. See ConversionWorker.doWork and FailureOutcome.
setForeground(
ForegroundInfo(
NOTIFICATION_ID,
notifications.build(id, "Joining ${uris.size} files", 0, indeterminate = true),
ConversionForegroundType.current(),
),
)
//
// Posted through getForegroundInfo() rather than built here a second time -- see that
// override, and its twin in ConversionWorker.
setForeground(getForegroundInfo())
val result = ConversionDependencies.concat(applicationContext).join(uris, staged, format)
Result.success(
@@ -129,11 +131,29 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker
return publisher.hasSpaceFor(bytes)
}
override suspend fun getForegroundInfo(): ForegroundInfo = ForegroundInfo(
NOTIFICATION_ID,
notifications.build(id, "Joining files", 0, indeterminate = true),
ConversionForegroundType.current(),
)
/**
* The notification a starting join posts, and now the only definition of it.
*
* WorkManager's hook for expedited work, which **will not call this on any device this app
* supports** — see [ConversionWorker.getForegroundInfo] for the measurement and for why
* `setExpedited` alone would have left these lines exactly as cold as they were. What makes
* them live is [doWork] posting this instead of building its own copy.
*
* It counts the inputs itself rather than being handed the number, so that it is still answerable
* before [doWork] has parsed anything — which is the contract WorkManager's own caller wants.
* The count is read from the same key, so the two cannot disagree. The `?: 0` arm is
* unreachable and named rather than covered: [doWork] refuses a job with no URI array several
* lines above this call, and nothing else calls it. It is the shape `docs/coverage-read-findings.md`
* calls F4 — a second line of defence that cannot be provoked.
*/
override suspend fun getForegroundInfo(): ForegroundInfo {
val inputCount = inputData.getStringArray(KEY_INPUT_URIS)?.size ?: 0
return ForegroundInfo(
NOTIFICATION_ID,
notifications.build(id, joiningTitle(inputCount), 0, indeterminate = true),
ConversionForegroundType.current(),
)
}
companion object {
/**
@@ -204,6 +224,16 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker
*/
fun outputNameFor(format: OutputFormat): String = "joined.${format.extension}"
/**
* What the progress notification says while a join runs.
*
* Named once, for the convention #158 established about strings the user can see. It was
* two strings until 2026-09-06 — `"Joining N files"` built inline in [doWork] and a
* countless `"Joining files"` in [getForegroundInfo] — for one notification that only ever
* had one job, and the copy nothing executed was free to drift from the one that did.
*/
fun joiningTitle(inputCount: Int): String = "Joining $inputCount files"
private const val NOTIFICATION_ID = 1002
private const val TAG = "ConcatWorker"
@@ -215,6 +245,10 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker
*/
fun request(inputs: List<Uri>, totalBytes: Long?, format: OutputFormat = DEFAULT_FORMAT) =
OneTimeWorkRequestBuilder<ConcatWorker>()
// Expedited, exactly as ConversionWorker.request is and for the same reasons; that
// one's comment and class KDoc carry them. Nothing here sets an initial delay or a
// constraint, which is what makes it legal for `build()` to accept.
.setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST)
.addTag(JobTags.inputCount(inputs.size))
.setInputData(
Data.Builder()
@@ -8,6 +8,7 @@ import androidx.work.CoroutineWorker
import androidx.work.Data
import androidx.work.ForegroundInfo
import androidx.work.OneTimeWorkRequestBuilder
import androidx.work.OutOfQuotaPolicy
import androidx.work.WorkerParameters
import androidx.work.hasKeyWithValueOfType
import androidx.work.workDataOf
@@ -40,8 +41,28 @@ import java.io.File
* observe. That durability is what makes the six-hour foreground-service timeout
* recoverable instead of fatal.
*
* Expedited work is deliberately *not* used. It maps to JobScheduler expedited jobs
* with a short quota, which is the wrong shape for a multi-minute transcode.
* Enqueued as **expedited** work, with `RUN_AS_NON_EXPEDITED_WORK_REQUEST`. This paragraph said
* the opposite until 2026-09-06 — "deliberately *not* used… the wrong shape for a multi-minute
* transcode" — and the quota it named does not reach a transcode the way it reads:
*
* - The quota belongs to the *JobScheduler* job, and `SystemJobInfoConverter:135` in
* work-runtime 2.11.2 sets `JobInfo.setExpedited(true)` only when `!isRetry && !isDelayed`.
* A retry is therefore scheduled exactly as every job is scheduled today.
* - A job the system stops mid-run does not get its answer from [FailureOutcome].
* `WorkerWrapper.interrupt` cancels the worker's coroutine with a `WorkerStoppedException`,
* which its `launch` resolves as `ResetWorkerStatus` — the worker's own `Result` is discarded
* and the work re-enqueued with backoff, whatever it returned. So a quota stop is a retry, and
* the `CancellationException` arm in [doWork] is what deletes the partial on the way through.
*
* What it buys is narrower than "conversions start sooner", and the narrowness is the honest part:
* `GreedyScheduler` starts unconstrained, undelayed work in-process the moment it is enqueued and
* carries no `expedited` branch at all, so a conversion begun from the open app runs exactly when
* it ran before — the common case does not move. The flag is for the job that has to go *through*
* JobScheduler because no process is left to start it: one still enqueued when the app died.
* `SystemJobScheduler.schedule` re-converts the spec every time it schedules, so such a job is
* expedited on the way back in, and a retried one is not. `RUN_AS_NON_EXPEDITED_WORK_REQUEST`
* rather than `DROP_WORK_REQUEST`: an invisible quota is no reason to throw a user's conversion
* away, and `SystemJobScheduler:198` degrades it to an ordinary job instead.
*
* That durability is not free, and the queue surviving is not the same as the job surviving.
* When WorkManager recovers a job after process death the app is by definition in the background,
@@ -60,7 +81,7 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo
override suspend fun doWork(): Result {
val inputUri = inputData.getString(KEY_INPUT_URI)?.let(Uri::parse)
?: return Result.failure(workDataOf(KEY_ERROR to "No input file."))
val displayName = inputData.getString(KEY_DISPLAY_NAME) ?: "input"
val displayName = displayName()
// Absent, not zero, when nobody could say -- see InputQuery. `getLong(key, 0L)` is what
// made those two the same number, and `hasSpaceFor(0)` is only "is there 128 MB free".
val declaredSize = inputData
@@ -100,7 +121,10 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo
// process death is. With it above the try that throw escaped doWork() entirely: no
// retry, no error in the output Data, and no staged.delete(). MediaProbe.probe below
// was outside for the same reason and had the same problem.
setForeground(foregroundInfo(displayName, percent = 0, indeterminate = true))
//
// Posted through getForegroundInfo() rather than built here a second time -- see that
// override for what the duplicate cost.
setForeground(getForegroundInfo())
// Through the seam rather than MediaProbe directly. The seam already existed for the
// ViewModel and the worker was the last caller bypassing it, which is why nothing on
@@ -339,8 +363,32 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo
return OutputSpec(container, video, audio)
}
/**
* What this job's input is called, or [InputQuery.FALLBACK_DISPLAY_NAME] when nothing named it.
*
* One read rather than the two copies of `?: "input"` that [doWork] and [getForegroundInfo]
* each carried, and against `InputQuery`'s constant rather than a third literal of the same
* string: it is the same fallback the picker uses, and it reaches the save dialog as
* `input_converted.mp4`.
*/
private fun displayName(): String = inputData.getString(KEY_DISPLAY_NAME) ?: InputQuery.FALLBACK_DISPLAY_NAME
/**
* The notification a starting conversion posts, and now the only definition of it.
*
* This is WorkManager's hook for expedited work, and **it will not be called on any device
* this app supports.** `WorkForeground.kt:38` in work-runtime 2.11.2 opens with
* `if (!spec.expedited || Build.VERSION.SDK_INT >= 31) return`, that function is the library's
* only caller of `getForegroundInfoAsync()`, and `minSdk` is 33. So #252's premise — that
* enqueueing expedited work would make these lines live — is false, and `setExpedited` alone
* would have left them exactly as cold as the first instrumented coverage read found them.
*
* What makes them live is [doWork] posting *this* instead of building its own copy. The two
* were identical — same title, `percent = 0`, `indeterminate = true` — so one was a duplicate
* that could drift, and the one nothing executed is the one that would have drifted silently.
*/
override suspend fun getForegroundInfo(): ForegroundInfo = foregroundInfo(
inputData.getString(KEY_DISPLAY_NAME) ?: "input",
displayName(),
percent = 0,
indeterminate = true,
)
@@ -416,6 +464,12 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo
quality: QualityTier = QualityTier.FAST,
enginePreference: EnginePreference = EnginePreference.AUTO,
) = OneTimeWorkRequestBuilder<ConversionWorker>()
// Expedited, so the jobs that do go through JobScheduler are treated as the
// user-initiated work they are -- see the class KDoc for what that is and is not worth.
// Safe to set here and only because of what this builder does not do: `build()` refuses
// an expedited request carrying an initial delay or any constraint but network and
// storage, and none of the three is set below.
.setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST)
.addTag(JobTags.displayName(displayName))
// Neither the tag nor the Data entry is written for a size nobody knows. A `Data` has
// no null, so the absence of the key *is* the unknown — and a tag reading
@@ -0,0 +1,102 @@
package org.libremediaconverter.work
import android.net.Uri
import androidx.media3.common.util.UnstableApi
import androidx.work.Constraints
import androidx.work.OutOfQuotaPolicy
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Test
import org.junit.runner.RunWith
import org.robolectric.RobolectricTestRunner
/**
* Both workers enqueue **expedited** work, and stay legal doing it.
*
* The two questions are separate and only one of them is about the flag.
*
* - **Is it set.** `expedited` is `false` by default, so `assertTrue` here is what a deleted
* `setExpedited(...)` reddens. That mutation was run.
* - **Is it legal.** `WorkRequest.Builder.build()` refuses an expedited request that carries an
* initial delay or any constraint but network and storage — `require(workSpec.initialDelay <= 0)
* { "Expedited jobs cannot be delayed" }` in work-runtime 2.11.2. Neither `request` sets either
* today, so both `build()` calls pass and the `IllegalArgumentException` is a *future* hazard
* rather than a current one. The delay and constraints assertions below are what name it: add a
* delay to either builder and this class fails on the throw, in the same second, instead of the
* app failing to enqueue a conversion on a device.
*
* **The policy assertion bites less than it reads, and that is worth writing down rather than
* leaving to be rediscovered.** `WorkSpec.outOfQuotaPolicy` *defaults* to
* `RUN_AS_NON_EXPEDITED_WORK_REQUEST`, so it is already this value on a request that was never
* expedited at all — deleting `setExpedited` does not redden it. What it does pin is the one
* alternative: `DROP_WORK_REQUEST` throws a user's conversion away because an invisible quota ran
* out, and that mutation *is* red here.
*
* The delay is not hypothetical either. Three tests deliberately build a delayed request to hold a
* job in `ENQUEUED` — `NotificationCancelActionTest`, `ReattachOnLaunchTest` and
* `CancelReachesWorkManagerTest` — and every one of them builds its own
* `OneTimeWorkRequestBuilder` rather than adding a delay to what `request` returns. That is why
* making these expedited broke none of them; the one that starts from `request` takes only
* `base.workSpec.input` from it.
*/
@UnstableApi
@RunWith(RobolectricTestRunner::class)
class ExpeditedRequestTest {
@Test
fun `a conversion is enqueued as expedited work`() {
val spec = ConversionWorker.request(INPUT, DISPLAY_NAME, INPUT_BYTES).workSpec
assertTrue("a conversion the user asked for has to be expedited work", spec.expedited)
assertEquals(
"a quota nobody can see is no reason to drop a conversion",
OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST,
spec.outOfQuotaPolicy,
)
}
@Test
fun `a join is enqueued as expedited work`() {
val spec = ConcatWorker.request(listOf(INPUT, SECOND_INPUT), TOTAL_BYTES).workSpec
assertTrue("a join the user asked for has to be expedited work", spec.expedited)
assertEquals(
"a quota nobody can see is no reason to drop a join",
OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST,
spec.outOfQuotaPolicy,
)
}
/**
* The two properties that keep `build()` from throwing, asserted on both requests at once
* because the rule is WorkManager's rather than either worker's.
*/
@Test
fun `neither expedited request carries what would make it illegal`() {
val requests = listOf(
ConversionWorker.request(INPUT, DISPLAY_NAME, INPUT_BYTES).workSpec,
ConcatWorker.request(listOf(INPUT, SECOND_INPUT), TOTAL_BYTES).workSpec,
)
requests.forEach { spec ->
assertEquals(
"expedited work cannot be delayed: ${spec.workerClassName}",
0L,
spec.initialDelay,
)
assertEquals(
"expedited work takes only network and storage constraints: ${spec.workerClassName}",
Constraints.NONE,
spec.constraints,
)
}
}
private companion object {
val INPUT: Uri = Uri.parse("file:///tmp/holiday.mp4")
val SECOND_INPUT: Uri = Uri.parse("file:///tmp/holiday2.mp4")
const val DISPLAY_NAME = "holiday.mp4"
const val INPUT_BYTES = 1_024L
const val TOTAL_BYTES = 2_048L
}
}
@@ -0,0 +1,203 @@
package org.libremediaconverter.work
import android.app.Application
import android.app.Notification
import android.net.Uri
import androidx.media3.common.util.UnstableApi
import androidx.work.Data
import androidx.work.testing.TestListenableWorkerBuilder
import androidx.work.workDataOf
import kotlinx.coroutines.runBlocking
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.convert.ConcatJoiner
import org.libremediaconverter.convert.ConversionDependencies
import org.libremediaconverter.convert.installTestWorkManager
import org.libremediaconverter.ffmpeg.ConcatEngine
import org.libremediaconverter.model.ConcatStrategy
import org.libremediaconverter.model.DeviceCodecs
import org.libremediaconverter.model.EnginePreference
import org.libremediaconverter.model.InputProbe
import org.libremediaconverter.model.OutputFormat
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment
import java.io.File
import java.util.UUID
/**
* The first thing either worker posts is what its own `getForegroundInfo()` builds.
*
* **Both overrides were dead code until 2026-09-06, and #252 is where that was found** — the first
* instrumented coverage read reported `ConversionWorker:342-346` and `ConcatWorker:132-136` among
* the 32 lines *neither* suite reaches. The ticket's premise was that enqueueing expedited work
* would make them live, since `getForegroundInfo()` is WorkManager's expedited-work hook.
*
* **That premise is false at this `minSdk`, which is the finding underneath the fix.**
* `WorkForeground.kt:38` in work-runtime 2.11.2 opens `workForeground` with
* `if (!spec.expedited || Build.VERSION.SDK_INT >= 31) return`, that function is the library's only
* caller of `getForegroundInfoAsync()`, and `minSdk` is 33. So `setExpedited` alone would have left
* both overrides exactly as cold as the read found them, and a test written to drive them through
* WorkManager would be testing a code path no device this app supports can take — E1's failure
* mode, where a test asserts and never reaches.
*
* What makes them live is a single-definition change instead. Each worker had **two** definitions
* of one notification: the override, and an identical `ForegroundInfo` built inline in `doWork`.
* `doWork` now posts the override's, so the copy nothing executed is gone and the one that remains
* runs on every job.
*
* These tests are what hold that wiring. Each asserts the notification's *contents* against
* constants rather than against `worker.getForegroundInfo()` — comparing the two would move
* together under every mutation and stay green — and the mutations that redden them are named on
* each test.
*/
@UnstableApi
@RunWith(RobolectricTestRunner::class)
class ForegroundNotificationTest {
private lateinit var app: Application
private lateinit var updater: RecordingForegroundUpdater
@Before
fun setUp() {
app = RuntimeEnvironment.getApplication()
updater = RecordingForegroundUpdater()
ConversionDependencies.publisher = { AlwaysRoomPublisher(app) }
ConversionDependencies.probe = { _, _ -> InputProbe() }
ConversionDependencies.deviceCodecs = { DeviceCodecs.PERMISSIVE }
ConversionDependencies.software = { WritingTranscoder }
ConversionDependencies.concat = { WritingJoiner }
// The notification carries a WorkManager cancel PendingIntent, so without this the worker
// fails building the notification rather than on anything these tests are about.
installTestWorkManager(app, Data.EMPTY)
}
@After
fun tearDown() {
ConversionDependencies.reset()
}
/**
* Mutation that must go red, and did: inside `ConversionWorker.getForegroundInfo`, replace
* `displayName()` with a literal, or `percent = 0` with anything else. Both are in the override's
* own body, so a red here is proof `doWork` executes it rather than a copy of it.
*/
@Test
fun `a conversion's first foreground post is the one getForegroundInfo builds`() {
runBlocking { conversionWorker().doWork() }
val first = updater.infos.first()
val extras = first.notification.extras
assertEquals(
"the notification has to name the file the user picked",
DISPLAY_NAME,
extras.getString(Notification.EXTRA_TITLE),
)
assertEquals("a conversion starts at zero", 0, extras.getInt(Notification.EXTRA_PROGRESS))
assertTrue(
"nothing is known about the length of the job yet, so the bar is indeterminate",
extras.getBoolean(Notification.EXTRA_PROGRESS_INDETERMINATE),
)
assertEquals(
"the foreground service type is the regime's, not zero",
ConversionForegroundType.current(),
first.foregroundServiceType,
)
}
/**
* The count is the point.
*
* `getForegroundInfo` said `"Joining files"` and `doWork` said `"Joining N files"` — one
* notification with two texts, and the one nothing ran was free to drift. Now there is one,
* and it reads the input array itself so it can still answer before `doWork` has parsed
* anything.
*
* Mutation that must go red, and did: replace the array read in `ConcatWorker.getForegroundInfo`
* with a constant `0`, which yields `"Joining 0 files"`. Asserting merely that the title starts
* with "Joining" would survive that, which is why the whole string is pinned.
*/
@Test
fun `a join's first foreground post counts the files it was given`() {
runBlocking { joinWorker().doWork() }
val first = updater.infos.first()
assertEquals(
"the notification has to say how many files are being joined",
ConcatWorker.joiningTitle(INPUTS.size),
first.notification.extras.getString(Notification.EXTRA_TITLE),
)
assertEquals(
"the foreground service type is the regime's, not zero",
ConversionForegroundType.current(),
first.foregroundServiceType,
)
}
/**
* And the title is really the file's name rather than any string at all.
*
* [ConcatWorker.joiningTitle] is asserted above through the constant the worker itself uses, so
* that assertion cannot catch the sentence being reworded — deliberately, since the wording is
* not what the test is about. This one can: two files, two names, one worker each.
*/
@Test
fun `two conversions of differently named files post differently named notifications`() {
runBlocking { conversionWorker(displayName = OTHER_NAME).doWork() }
assertEquals(
OTHER_NAME,
updater.infos.first().notification.extras.getString(Notification.EXTRA_TITLE),
)
}
private fun conversionWorker(displayName: String = DISPLAY_NAME) = TestListenableWorkerBuilder<ConversionWorker>(
context = app,
inputData = workDataOf(
ConversionWorker.KEY_INPUT_URI to INPUT.toString(),
ConversionWorker.KEY_DISPLAY_NAME to displayName,
ConversionWorker.KEY_SIZE_BYTES to INPUT_BYTES,
// FORCE_SOFTWARE is the one preference that decides without consulting the input,
// and a file:// URI keeps the worker out of FFmpegKit's native SAF bridge.
ConversionWorker.KEY_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name,
),
runAttemptCount = 0,
).setId(JOB_ID)
.setForegroundUpdater(updater)
.build()
private fun joinWorker() = TestListenableWorkerBuilder<ConcatWorker>(
context = app,
inputData = workDataOf(
ConcatWorker.KEY_INPUT_URIS to INPUTS.map(Uri::toString).toTypedArray(),
ConcatWorker.KEY_TOTAL_BYTES to TOTAL_BYTES,
ConcatWorker.KEY_FORMAT to OutputFormat.MP4_H264.name,
),
runAttemptCount = 0,
).setId(JOB_ID)
.setForegroundUpdater(updater)
.build()
private companion object {
val INPUT: Uri = Uri.parse("file:///tmp/holiday.mp4")
val INPUTS: List<Uri> = listOf(INPUT, Uri.parse("file:///tmp/holiday2.mp4"))
const val DISPLAY_NAME = "holiday.mp4"
const val OTHER_NAME = "birthday.mkv"
const val INPUT_BYTES = 1_024L
const val TOTAL_BYTES = 2_048L
val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000252")
}
}
/** A joiner that writes an output and reports a strategy; nothing here is about the engine. */
private object WritingJoiner : ConcatJoiner {
override suspend fun join(inputs: List<Uri>, output: File, format: OutputFormat): ConcatEngine.Result {
output.writeBytes(ByteArray(OUTPUT_BYTES))
return ConcatEngine.Result(ConcatStrategy.STREAM_COPY, output)
}
private const val OUTPUT_BYTES = 512
}
@@ -3,16 +3,12 @@ package org.libremediaconverter.work
import android.app.Application
import android.app.Notification
import android.app.NotificationManager
import android.content.Context
import android.net.Uri
import androidx.media3.common.util.UnstableApi
import androidx.work.Data
import androidx.work.ForegroundInfo
import androidx.work.WorkInfo
import androidx.work.testing.TestForegroundUpdater
import androidx.work.testing.TestListenableWorkerBuilder
import androidx.work.workDataOf
import com.google.common.util.concurrent.ListenableFuture
import kotlinx.coroutines.runBlocking
import org.junit.After
import org.junit.Assert.assertEquals
@@ -224,26 +220,6 @@ class ProgressNotificationTest {
}
}
/**
* Records every [ForegroundInfo] the worker publishes, and otherwise behaves as the test default.
*
* Delegating to [TestForegroundUpdater] rather than hand-rolling a `ListenableFuture<Void>`: the
* worker awaits what this returns, so a future that never completes would hang the initial
* `setForeground` rather than test anything.
*/
private class RecordingForegroundUpdater : TestForegroundUpdater() {
val infos = mutableListOf<ForegroundInfo>()
override fun setForegroundAsync(
context: Context,
id: UUID,
foregroundInfo: ForegroundInfo,
): ListenableFuture<Void> {
infos += foregroundInfo
return super.setForegroundAsync(context, id, foregroundInfo)
}
}
/** An engine that reports whatever [report] wants reported, then writes an output. */
private class ReportingTranscoder(private val report: ((Int) -> Unit) -> Unit) : SoftwareTranscoder {
override suspend fun run(
@@ -1,11 +1,14 @@
package org.libremediaconverter.work
import android.content.Context
import androidx.work.ForegroundInfo
import androidx.work.testing.TestForegroundUpdater
import com.google.common.util.concurrent.ListenableFuture
import org.libremediaconverter.convert.OutputPublisher
import org.libremediaconverter.convert.SoftwareTranscoder
import org.libremediaconverter.model.ConversionRequest
import java.io.File
import java.util.UUID
import java.util.concurrent.ExecutionException
import java.util.concurrent.Executor
import java.util.concurrent.TimeUnit
@@ -94,3 +97,27 @@ internal class FailedFuture(private val failure: Throwable) : ListenableFuture<V
override fun get(): Void = throw ExecutionException(failure)
override fun get(timeout: Long, unit: TimeUnit): Void = throw ExecutionException(failure)
}
/**
* Records every [ForegroundInfo] the worker publishes, and otherwise behaves as the test default.
*
* Delegating to [TestForegroundUpdater] rather than hand-rolling a `ListenableFuture<Void>`: the
* worker awaits what this returns, so a future that never completes would hang the initial
* `setForeground` rather than test anything.
*
* Shared scaffolding since #252 moved it here out of `ProgressNotificationTest`, which asks what a
* *running* worker publishes; `ForegroundNotificationTest` asks what its *first* post is, and both
* questions need the same recorder. `infos.first()` is that first post in either.
*/
internal class RecordingForegroundUpdater : TestForegroundUpdater() {
val infos = mutableListOf<ForegroundInfo>()
override fun setForegroundAsync(
context: Context,
id: UUID,
foregroundInfo: ForegroundInfo,
): ListenableFuture<Void> {
infos += foregroundInfo
return super.setForegroundAsync(context, id, foregroundInfo)
}
}
+22 -1
View File
@@ -403,6 +403,27 @@ They are still correct to keep: `ForegroundInfo` is required by the `CoroutineWo
named — a `getForegroundInfo` that starts branching — plus one more: the day anything calls
`setExpedited`.
**Updated 2026-09-06 (#252, and the sentence above is half wrong).** "WorkManager calls
`getForegroundInfoAsync()` only for expedited work" is true and *not sufficient*, and the missing
half is what made the reopening trigger wrong. `WorkForeground.kt:38` in work-runtime 2.11.2 opens
the library's only caller with
```kotlin
if (!spec.expedited || Build.VERSION.SDK_INT >= 31) return
```
and `minSdk` is 33. So calling `setExpedited` reopens nothing: on **every** device this app
supports, WorkManager does not consult `getForegroundInfo()` whether the work is expedited or not.
#252 was filed on the trigger as this entry stated it, and its acceptance criterion — "a request
now carries `setExpedited` and the existing worker tests drive them" — cannot be met that way.
What made the lines live instead was that each worker held **two** definitions of one notification:
the override, and an identical `ForegroundInfo` built inline in `doWork`. `doWork` now posts the
override's, so the duplicate is gone and what remains runs on every job. The general lesson is the
one E1 states from the other side: *check that the mechanism you are relying on actually fires on
the machine that runs it* — here the mechanism was a library early-return two source lines long,
and four waves of reading had taken the API summary's word for it.
---
## F10 — Three arms that are reachable, uncovered, and cannot be made to bite
@@ -452,7 +473,7 @@ the cheaper order.
| F6 | Four more unreachable arms; `ConversionRouter:214-217`'s KDoc is false | low | confirmed by inspection; each traced to its upstream guard | **no action**, except the one-line KDoc fix |
| F7 | `probeWithExtractor`'s catch is unreachable, as `probeForConcat`'s is | n/a | measured across four URI shapes (recorded in `CLAUDE.md`) | **no action** — device-only, now written down for both sites |
| F8 | Three more dead members and six unused defaults | low | confirmed by inspection; grep per member | delete or keep knowingly — **not** a test gap |
| F9 | Both `getForegroundInfo` overrides are dead: expedited work is never used | n/a | confirmed by inspection; `grep setExpedited` returns nothing | **no action** — sharpens #88's close |
| F9 | Both `getForegroundInfo` overrides are dead: expedited work is never used | n/a | confirmed by inspection; `grep setExpedited` returns nothing | **closed 2026-09-06 by #252** — and its stated reopening trigger was wrong; see the update on the entry |
| F10 | Three reachable arms where no mutation bites | n/a | confirmed by inspection; each mutation traced to its masking guard | **no action** — recorded to stop the next read re-picking them |
Order, if these are acted on: **F1 and F5 first, separately.** They are the two with a possible
+137 -2
View File
@@ -1,6 +1,6 @@
# E2E-read findings
**Status:** seven findings; E4 fixed, the rest standing, none urgent — **plus one confirmed vacuous test, which is a
**Status:** seven findings; E4 fixed, E7 extended and its ticket closed, the rest standing — **plus one confirmed vacuous test, which is a
ticket rather than an entry here** (see [Not covered here](#not-covered-here)). `E1`–`E6` came from
the 2026-09-05 read of the instrumented suite. Every entry here is a *test-suite* observation —
something a new test would not fix, because the test already exists and the problem is what it
@@ -274,6 +274,27 @@ ticket is about.
**So any test of `publish` against a real `DocumentsProvider` must drive DocumentsUI**, and pays
#190's flake tax. The work is one item at that cost, not two, and #226 was updated to say so.
**Updated 2026-09-06, doing it: there is a second constraint underneath, and it has 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:
```
java.lang.RuntimeException: Intent in process org.libremediaconverter resolved to different
process org.libremediaconverter.test
at android.app.Instrumentation.startActivitySync
```
Instrumentation runs in the target app's process, so a component declared in the instrumentation
APK is in the wrong one — the same fact that sinks approach 2 above, arriving from the other side.
**The app's own Save button is the only launcher available to drive**, which is also the more
faithful thing to drive. `SafPickerRoundTripTest.aSaveWritesToTheDocumentTheSystemPickerCreated` is
what came of it.
**And the premise turned out to be true**, which is the answer #226 was filed for: on API 34,
stock DocumentsUI hands back a document URI reporting a size of exactly zero. `deletePartialOutput`
can fire, and D4's fix is live rather than inert. A "no defect found" — and not one that could have
been reached by reading.
### What this does *not* block, which is the useful half
`FFmpegKitConfig.getSafParameterForRead` — the bridge on every real conversion and join — needs no
@@ -361,7 +382,7 @@ decision, not a detail — see **E6** for why no third option exists — and **#
| **#223** | `HardwareFallbackTest` never attempts the hardware path on any emulator leg | closed — it skips instead of passing vacuously |
| **#224** | Cancelling a *running* native session, in any of the three engines | closed — all three engines |
| **#225** | No `content://` input has reached a *successful* conversion — the ffkitsaf bridge | closed, and it found **#238** |
| **#226** | `OutputPublisher.publish` against a real `DocumentsProvider` | **open** — re-scoped by E7; one picker-driven item, not two |
| **#226** | `OutputPublisher.publish` against a real `DocumentsProvider` | closed — the *premise* holds; see E7. The delete **arm** is still unrun: **#250** |
| **#227** | The notification's Cancel action has never been fired | closed |
| **#228** | `encodesFlacLosslessAudio` and `encodesOpus` pass on any non-empty file | closed |
| **#229** | FFmpeg's progress percentage is computed everywhere and asserted nowhere | closed |
@@ -380,3 +401,117 @@ unasserted value, it was **a combination of two covered things that no test put
the acceptance criterion wave 4 established and which caught two vacuous tests in that wave before
they shipped. #223 is the one that shows why the criterion matters: it has two passing assertions and
still tests nothing.
## The 2026-09-06 re-check
Run after the last ticket landed, to ask whether the suite's self-description had drifted again. It
had, and **every drifted line came from #226 — the last PR of this 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 (`baseline: matches`). Every gating leg is
green on `main`.
- **The counts had gone stale in four places** — `CLAUDE.md` (three sites),
`FailsOnEmulatorApi37.kt`'s KDoc, and two comments in `status_check.yml` — all still saying five
carriers of 69. **The gating figure is what hid it**: 69 − 5 and 70 − 6 are both 64, so the one
number a reader would check against a run had not moved. CLAUDE.md's own instruction to derive
these rather than remember them is what caught it.
- **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 by `canEncode(H265)`. The *negation* of the stated
reason was true. This is **E1 and E3's failure mode landing in a test written by the read that
found it** — a passing test with a wrong explanation.
- **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 all four advisory runs at this baseline (`34041156680`, `34041593697`,
`34042397320`, `34043502322`) report `expected: 6, received: 4, failed: 4` — the three Media3
tests plus the rotation. The save test is therefore
marked by **inheritance, not measurement**, which is now what both KDocs say.
- **One substantive gap, filed as #250.** `FixtureDocumentsProvider.deletedDocumentIds()` has no
callers. #226 proved D4's *premise* — SAF hands back a document of exactly zero bytes — but drove
only the success path, so `deletePartialOutput` against a real `DocumentsProvider` is still
asserted nowhere. `openDestination` is `protected open` precisely to force the failure, so the
test is cheap; it costs another marked picker test and a baseline of 7.
**The reusable part is the second bullet.** A read that fixes documentation drift can introduce it in
the same wave, and the tests it writes are no more self-describing than the ones it audited. The
check that found it is the one this document already recommends: **read the KDoc against the code,
not against the ticket.**
## E8 — the instrumented suite's coverage, measured for the first time
**Severity: n/a · Measured 2026-09-06 on API 34 · the number had never existed**
Four coverage waves were steered by a figure that cannot see `app/src/androidTest`. Nothing had
ever produced the other half, because `enableAndroidTestCoverage` was unset, so a connected run
emitted no `.ec` at all and `jacocoTestReport`'s execution data names only `testDebugUnitTest`.
Measured by setting that flag temporarily, running `run-e2e.sh 34` (70/70, 0 failed, 3m21s — the
instrumentation destabilised nothing), and reporting the resulting `.ec` against the **same** class
directories and exclusions the committed task uses:
| suite | line | branch |
|---|---|---|
| JVM `testDebugUnitTest` | 2236/2374 — **94.2%** | 1171/1338 — **87.5%** |
| Instrumented, 70 tests | 1711/2374 — **72.1%** | 669/1354 — **49.4%** |
| **Union** | 2342/2374 — **98.7%** | 1212/1354 — **89.5%** |
The JVM row reproduced the committed figure exactly, which is the control: both exec sets match the
current class files, so the union is trustworthy.
**Two caveats before anyone quotes these.** Branch denominators differ by 16 — 1338 against 1354 —
entirely inside `MediaProbe`, an artefact of offline versus on-the-fly instrumentation; line
denominators are identical at 2374, so only the line figures compare exactly. And **72.1% is not a
grade for the instrumented suite.** Seventy end-to-end tests reach code broadly and choose arms
rarely; a branch figure of 49.4% is what that shape looks like. This whole document exists because
the gaps that mattered — #223's vacuous assertions, #238's two covered things nobody combined —
are invisible to any percentage.
### What the device suite is for, in numbers
It closes **106 lines** the JVM suite misses, and they are precisely the ones wave 4 wrote off:
| file | JVM missed | union missed |
|---|---|---|
| `FFmpegEngine.kt` | 32 | **0** |
| `Media3Engine.kt` | 24 | **0** |
| `ConcatEngine.kt` | 15 | **0** |
| `MediaProbe.kt` | 13 | **3** |
| `MainActivity.kt` | 10 | **1** |
| `AndroidDeviceCodecs.kt` | 8 | **0** |
| `Transcoders.kt` | 10 | 3 |
CLAUDE.md's wave-4 read called 81 lines "native or device edges" — `FFmpegEngine` 33,
`Media3Engine` 24, `ConcatEngine` 14, `MainActivity.onCreate` 10. The first four rows above total
**81**, and the union leaves **1**. That **confirms** the read's own hypothesis rather than
overturning it: it always said those zeroes were "the `testDebugUnitTest`-only measurement
boundary". Nobody had measured past the boundary. `AndroidDeviceCodecs` is the pointed one — #194
was filed to cut a seam because `probe()` could not be reached, and on a device it is fully covered.
### The 32 lines neither suite reaches, classified
Every one was read. **None of them is an e2e test gap**, which is the result:
| lines | where | classification |
|---|---|---|
| 9 | `Transcoders` ×3, `ConversionViewModel`, `ConverterScreen`, `JoinViewModel`, `JoinScreen`, `MainActivity`, `Reattachment` | **compiler-generated** — default-arg `$default` bridges, coroutine completion, the synthetic `NoWhenBranchMatchedException` arm of a `when` over `Destination` |
| 10 | `ConversionWorker:342-346`, `ConcatWorker:132-136` | `getForegroundInfo()` — WorkManager's **expedited-work** hook, and nothing here enqueues expedited work. The live path is `setForeground(foregroundInfo(...))`, which is covered. **#252 — closed 2026-09-06, and not the way this row expects.** Expedited work is now enqueued, but that is *not* what covers these lines: `WorkForeground.kt:38` returns before the hook whenever `SDK_INT >= 31`, and `minSdk` is 33. What covers them is `doWork` posting the override instead of a second copy of the same notification. See `coverage-read-findings.md` F9's update |
| 3 | `ConversionNotifications:60-62` | **F5** — `areEnabled()` has no callers. Already on record |
| 3 | `CopyPlanner:28`, `OutputFormat:222-223` | public members with no callers. **#253**, with F5 |
| 3 | `MediaProbe:210-212` | `probeWithFFprobe`'s `catch` — **F7's sibling, and now measured**. See below |
| 2 | `FFmpegCommandBuilder:167-168` | `COPY`/`NONE -> error(...)` — F4-shaped, deliberately exempt |
| 1 | `FFmpegCommandBuilder:188` | the `VORBIS` encode arm. No `OutputFormat` produces it, but `ContainerCapabilities` lists it for WEBM and OGG. **#254** |
| 1 | `ConversionWorker:231` | `?: error("Could not open the input file.")`. `UnopenableUriTest` fails the job *downstream* of it, so the elvis is unprovoked — F4-shaped, same as the two above |
**`MediaProbe:210-212` is the one that gained a measurement.** F7 ruled `probeWithExtractor`'s catch
unreachable because Robolectric's `MediaExtractor` never throws. That reasoning does not transfer:
`probeWithFFprobe` calls `readMediaInformation` in native ffmpeg-kit, which the JVM never loads.
But `MediaProbe.probe` calls **both** probes on one line, and
`RemuxTest.probeDistinguishesAudioFromImagesFromRubbish` drives it on a device with 4096 bytes of
garbage — so the ffprobe path *has* been given malformed input on real hardware and **did not
throw**. Same conclusion as F7, reached by a different mechanism, and now on record rather than
assumed.
**The reusable part**: a union report is what separates "no test calls this" from "only a device
calls it", and neither report alone can. Six of the eight rows above were indistinguishable from
real gaps in the JVM-only number.
+249
View File
@@ -0,0 +1,249 @@
#!/usr/bin/env bash
#
# The local gate: what has to be green before a commit is made or a branch is pushed.
#
# THE RULE THIS ENFORCES (2026-09-06). Source changes must have the unit tests AND the
# instrumented tests passing at every supported API level before they are committed or
# pushed; test changes must have the whole suite passing at every API level. CI is not the
# place to find out. Four legs of this repo's history were spent discovering on CI what a
# local sweep would have said in twenty minutes -- and worse, the failing leg MOVED between
# runs (API 35 red then green, API 34 green then red), which is exactly the signal that gets
# misread as "someone else's flake" when it is read one leg at a time.
#
# WHY BOTH HOOKS RUN THE SAME GATE. A pre-commit-only gate is bypassed by amending; a
# pre-push-only gate lets a broken commit exist locally and get rebased into something else.
# Running both is not redundant in practice because of the cache below.
#
# THE CACHE IS KEYED ON CONTENT, NOT ON TIME, AND ON THE RIGHT CONTENT. The sweep is recorded
# under the hash of the `app/src` SUBTREE it verified, not the whole repo tree. Keying it on the
# whole tree was the first cut and it was wrong in a way that would have trained people to hate
# this hook: editing a comment in CLAUDE.md, or in this script, invalidated a sweep of identical
# application code and re-ran forty minutes of emulators to prove nothing. What the sweep is
# evidence about is `app/src`; that is what it is filed under. Any change to a single byte under
# `app/src` still invalidates it. The JVM gate is cheap and runs unconditionally.
#
# WHAT COUNTS AS "EVERY SUPPORTED API LEVEL", AND WHY 37 IS NOT AN EMULATOR HERE. 33, 34, 35
# and 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: measured 2026-09-06, the image logs
# `3 new surfaceflinger aborts in 45 s (want 0)` and then the APK install itself fails with
# `Can't find service: package`, because the framework is already gone before Gradle gets to
# install anything. `Starting 0 tests`. That is the same gralloc abort docs/api-37-emulator-crash.md
# measures, hit earlier in the sequence than the suite.
#
# So API 37 is covered here by the physical Pixel 10 Pro XL when it is attached, and by CI's
# gating leg otherwise. The hook says loudly which of the two happened rather than quietly
# claiming five levels when it ran four.
#
# THERE IS DELIBERATELY NO SKIP VARIABLE. An `LMC_SKIP_E2E=1` would be `--no-verify` wearing
# a different hat, and `--no-verify` needs the repo owner's say-so each time. If this gate is
# wrong, fix the gate.
set -uo pipefail
REPO_ROOT="$(git rev-parse --show-toplevel)"
cd "$REPO_ROOT" || exit 1
MODE="$(basename "$0")"
ZERO="0000000000000000000000000000000000000000"
# `git rev-parse --git-common-dir`, not a literal ".git" (#258). 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 this script does, it failed while the gate still printed
# green and exited 0. Every push from a worktree then re-swept 33-36 for nothing, silently, which
# is the worst shape a cache can fail in: invisible and expensive.
#
# --git-common-dir rather than --git-dir so the cache is SHARED across worktrees. The key is the
# app/src tree hash, and identical content is identical content whichever worktree produced it.
CACHE_DIR="$(git rev-parse --git-common-dir)/lmc-verify"
GRADLE_GATE=(:app:assembleDebug :app:testDebugUnitTest :app:compileDebugAndroidTestKotlin
:app:ktlintCheck :app:detekt :app:lintDebug)
# Says so when it cannot record, rather than leaving a cache that silently never fills (#258).
record_sweep() {
[ -n "$tree" ] || return 0
if mkdir -p "$CACHE_DIR" 2>/dev/null && : > "$CACHE_DIR/$tree" 2>/dev/null; then
return 0
fi
printf '\n\033[1m[local-gate]\033[0m could not record the sweep under %s -- it will re-run next
time. Not fatal, but it means every commit and push pays for it again.\n' "$CACHE_DIR"
}
say() { printf '\n\033[1m[local-gate]\033[0m %s\n' "$*"; }
die() {
printf '\n\033[1;31m[local-gate] BLOCKED\033[0m %s\n' "$*"
printf ' The rule: source work needs unit + e2e green at every API level before commit/push;\n'
printf ' test work needs the whole suite green at every level. Fix it, or ask before using\n'
printf ' --no-verify -- that flag is not yours to reach for unprompted.\n\n'
exit 1
}
# --- what changed, and what tree is being verified ---------------------------------------
changed_files=""
tree=""
case "$MODE" in
pre-commit)
changed_files="$(git diff --cached --name-only --diff-filter=ACMR)"
tree="$(git rev-parse "$(git write-tree):app/src" 2>/dev/null || echo "")"
;;
pre-push)
# stdin is `<local ref> <local sha> <remote ref> <remote sha>`, one line per ref pushed.
while read -r _ local_sha _ remote_sha; do
[ "$local_sha" = "$ZERO" ] && continue # branch deletion carries no content
base="$remote_sha"
if [ "$remote_sha" = "$ZERO" ]; then
# A new branch: compare against main rather than against every commit ever made.
base="$(git merge-base origin/main "$local_sha" 2>/dev/null || echo "")"
fi
if [ -n "$base" ]; then
changed_files="$changed_files$(git diff --name-only --diff-filter=ACMR "$base" "$local_sha")"$'\n'
else
changed_files="$changed_files$(git show --pretty=format: --name-only "$local_sha")"$'\n'
fi
tree="$(git rev-parse "$local_sha:app/src" 2>/dev/null || echo "")"
done
;;
*)
say "unknown hook name '$MODE'; nothing to do"
exit 0
;;
esac
if [ -z "${changed_files//[[:space:]]/}" ]; then
say "no added/modified files; nothing to verify"
exit 0
fi
touches_source=0
touches_tests=0
while IFS= read -r f; do
case "$f" in
app/src/main/*) touches_source=1 ;;
app/src/test/*|app/src/androidTest/*) touches_tests=1 ;;
esac
done <<< "$changed_files"
# --- the cheap gate always runs -----------------------------------------------------------
# --- shellcheck, at CI's exact pin ---------------------------------------------------------
# WHY THIS IS HERE. The gate ran ktlint, detekt and Android lint but not shellcheck, so a new or
# edited `.sh` file was precisely the case where this hook passed and CI's Static analysis leg
# still went red. That is not hypothetical: this script is itself a new `.sh` file, and the first
# thing it could not check was itself. It was caught by hand twice before it was caught here.
#
# THE DIGEST IS READ OUT OF status_check.yml, NOT COPIED INTO THIS FILE. shellcheck 0.9.0 and
# 0.11.0 disagree about how to report a trap handler -- SC2317 on seven body lines versus SC2329
# once on the declaration, same script, same directive, one red and one green. That disagreement
# is why CI pins by digest, and a second copy of the digest here would drift from it silently.
# When it drifts, the symptom is this gate passing and CI failing: the exact thing this section
# exists to prevent. So there is one digest in the repo and this reads it.
#
# ALL TRACKED FILES, not just changed ones, because that is what CI does -- `git ls-files '*.sh'`.
# The point is to predict that leg, not to audit the diff.
shellcheck_pin="$(grep -oE 'koalaman/shellcheck@sha256:[0-9a-f]{64}' \
.github/workflows/status_check.yml | head -1)"
# :z is podman's SELinux relabel and is what this host needs; docker on CI does without it.
runtime=""
mount=":z"
for candidate in podman docker; do
if command -v "$candidate" >/dev/null 2>&1; then
runtime="$candidate"
[ "$candidate" = "docker" ] && mount=""
break
fi
done
if [ -z "$shellcheck_pin" ]; then
say "NOT COVERED: shellcheck. Could not read the pinned digest out of
.github/workflows/status_check.yml -- if that pin moved or was reformatted, fix this grep
rather than leaving the check silently absent."
elif [ -z "$runtime" ]; then
say "NOT COVERED: shellcheck. Neither podman nor docker is on PATH, and there is no shellcheck
system package on this host. CI's Static analysis leg is what answers for .sh files then."
else
say "shellcheck ($runtime, $shellcheck_pin)"
if ! git ls-files -z '*.sh' |
xargs -0 -r "$runtime" run --rm -v "$PWD:/mnt$mount" "docker.io/$shellcheck_pin"; then
die "shellcheck failed. CI runs the same digest over the same files, so this is a red
Static analysis leg waiting to happen."
fi
fi
# --- actionlint, the half shellcheck cannot see ---------------------------------------------
# A good deal of this repo's bash lives in workflow `run:` blocks, which `git ls-files '*.sh'`
# does not match at all -- so without this a workflow edit is the same hole the section above
# just closed: green here, red on Static analysis. Pinned by digest for the reason in that
# section, 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.
actionlint_pin="$(grep -oE 'rhysd/actionlint@sha256:[0-9a-f]{64}' \
.github/workflows/status_check.yml | head -1)"
if [ -z "$actionlint_pin" ]; then
say "NOT COVERED: actionlint. Could not read the pinned digest out of
.github/workflows/status_check.yml -- fix this grep rather than leaving the check absent."
elif [ -z "$runtime" ]; then
say "NOT COVERED: actionlint. Neither podman nor docker is on PATH; CI's Static analysis leg
is what answers for the workflows then."
else
say "actionlint ($runtime, $actionlint_pin)"
if ! "$runtime" run --rm -v "$PWD:/repo$mount" -w /repo "docker.io/$actionlint_pin" -color; then
die "actionlint failed. CI runs the same digest over the same workflows."
fi
fi
say "$MODE: running the JVM gate"
if ! ./gradlew "${GRADLE_GATE[@]}" --continue; then
die "the JVM gate failed (assemble, unit tests, androidTest compile, ktlint, detekt, lint)."
fi
# --- the sweep, when code is involved ------------------------------------------------------
if [ "$touches_source" -eq 0 ] && [ "$touches_tests" -eq 0 ]; then
say "no app/src changes; the instrumented sweep is not required for this one"
record_sweep
exit 0
fi
if [ -n "$tree" ] && [ -f "$CACHE_DIR/$tree" ]; then
say "app/src ($tree) already swept and green; nothing under app/src has changed since"
exit 0
fi
say "app/src changed -- sweeping API 33, 34, 35, 36 (this takes tens of minutes, by design)"
if ! tools/local-emulator/run-e2e.sh 33 34 35 36; then
die "the instrumented suite is not green on 33-36."
fi
# API 37: the physical device if it is here, and an honest statement if it is not. run-e2e.sh is
# emulator-only (and overwrites E2E_EXTRA_GRADLE_ARGS with --rerun, so extra args cannot be passed
# through it), so this drives Gradle directly with the serial pinned -- the phone must never be
# picked up by accident, which is the hazard run-e2e.sh's header calls out.
export ANDROID_HOME="${ANDROID_HOME:-$HOME/Android/Sdk}"
export PATH="$ANDROID_HOME/platform-tools:$PATH"
device=""
while read -r serial state; do
[ "$state" = "device" ] || continue
case "$serial" in emulator-*) continue ;; esac
[ "$(adb -s "$serial" shell getprop ro.build.version.sdk 2>/dev/null | tr -d '\r')" = "37" ] || continue
device="$serial"
break
done < <(adb devices 2>/dev/null | tail -n +2)
levels="33, 34, 35, 36"
if [ -n "$device" ]; then
say "API 37 on the attached device $device"
if ! ANDROID_SERIAL="$device" ./gradlew :app:connectedDebugAndroidTest -PabiFilters=arm64-v8a; then
die "the instrumented suite is not green on API 37 (device $device)."
fi
levels="$levels, 37"
else
say "NOT COVERED LOCALLY: API 37. No API 37 device is attached, and the API 37 emulator cannot
install the APK on this host (see this script's header). CI's gating leg is what answers for it;
attach the Pixel 10 Pro XL to have this hook cover it too."
fi
record_sweep
# Name the levels rather than claiming "every supported level". The first cut said the latter on
# both paths, including the one that had just printed NOT COVERED two lines above -- a false claim
# printed by the tool whose whole job is to stop false claims reaching CI.
say "green on API $levels; $MODE allowed"
exit 0
+1
View File
@@ -0,0 +1 @@
local-gate.sh
+1
View File
@@ -0,0 +1 @@
local-gate.sh