Compare commits

..
Author SHA1 Message Date
Jason Ross 5461fa9cfa Merge pull request #189 from JMR-dev/docs/coverage-wave3
Re-measure after wave 3, and write down the two kinds of gap it had to separate
2026-09-01 23:44:33 -05:00
JMR-devandClaude Opus 5 da344b0fe4 Re-measure after wave 3, and write down the two kinds of gap it had to separate
92.8% line (2183/2352), 81.3% branch (1091/1342), 584 JVM tests in 87 classes, measured
2026-09-02 on the tree this branch creates rather than quoted from a PR body.

The shape of the wave is worth more than the number, and it is different from the two
before it. Waves 1 and 2 were finding uncovered code; by wave 3 there was little of that
left, so the gaps had to be sorted before any test was written. Coverage gaps -- filtered
to sites where JaCoCo reports mi > 0, which is what separates a real gap from a partial
branch on a compound condition, and which cut the candidate list roughly in half. And
assertion gaps, where JaCoCo is green and nothing checks the answer: MainActivity's rail
and bottom bar were both executed and transposing them passed the entire suite, as did
swapping the two progress-notification strings and swapping Content's two destinations.
No coverage number would have found any of the three.

Naming the required mutation per ticket earned its keep three times, each recorded with
what the weak assertion actually was. Also recorded: a green mutation is only evidence
when the mutation is a real change -- one classify reordering was semantically equivalent
for every reachable input, and a bad mutation and a weak test look identical in the output.

Two entries came back as not gaps, which is a result rather than a shortfall:
ContainerCapabilities:282's exclude filter cannot drop anything, and probeForConcat's
catch arm is unreachable on this runtime -- Robolectric's MediaExtractor never throws from
setDataSource, measured across four input shapes.

Both denominators moved, in opposite directions and for different reasons, so they are
stated rather than folded into the percentage: 1340 -> 1342 branches from MediaProbe.merge,
2348 -> 2352 lines from the ConcatJoiner interface. Neither is new untested code.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-01 22:09:35 -05:00
49 changed files with 282 additions and 4922 deletions
-40
View File
@@ -184,46 +184,6 @@ out="$(run_report "$root")"
assert_contains "same-line annotation removed: counts 2, so it was worth 1" "$out" \
" baseline DEVIATION: the tree carries 2 tests marked \`@FailsOnEmulatorApi37\` but the baseline says 3 — update FAILS_ON_EMULATOR_API37_BASELINE"
# ---------------------------------------------------------------------------
# 4. A run the abort truncated, with fewer failures than the baseline: NOT a deviation.
#
# `expected` comes from `Starting N tests`, printed before anything can abort, so it still
# answers "is the marked set the size the baseline says". `failed` is a tally of what actually
# ran, and on a truncated run the tests after the abort never start. Measured on 2026-09-05, two
# api37-debug dispatches of the same four marked tests: 4/4/4 and then 4/3/3. Announcing the
# second as "one now passes" is the wrong reading, and #120 is the standing lesson about a notice
# that is wrong often enough to be skimmed past.
# ---------------------------------------------------------------------------
root="$(make_root "$FIXTURE_DIR" 3)"
cat > "$root/gradle.log" <<'TRUNCATED'
> Task :app:connectedDebugAndroidTest
Starting 3 tests on test(AVD) - 16
There was 2 failure(s).
Test run failed to complete. Expected 3 tests, received 2. onError: commandError=false message=INSTRUMENTATION_ABORTED: System has crashed.
TRUNCATED
out="$(run_report "$root")"
assert_contains "truncated run: the truncation is reported" "$out" ' completed cleanly: no'
assert_absent "truncated run: the short failure count is not a deviation" "$out" 'tests failed, the baseline is'
# And the match line has to say what actually happened rather than repeat the baseline: PR #245's
# advisory leg printed `failed: 4` three lines above `matches (5 expected, 5 failed)`.
assert_contains "truncated run: the match line does not claim the baseline's failure count" "$out" \
' baseline: matches (3 expected; 2 of 3 failed, on a run the abort truncated — not compared)'
# ---------------------------------------------------------------------------
# 5. The same short failure count on a run that finished IS a deviation.
#
# The pair is the point: case 4 must not have bought its quiet by disabling the check outright.
# ---------------------------------------------------------------------------
root="$(make_root "$FIXTURE_DIR" 3)"
cat > "$root/gradle.log" <<'CLEAN'
> Task :app:connectedDebugAndroidTest
Starting 3 tests on test(AVD) - 16
There was 2 failure(s).
CLEAN
out="$(run_report "$root")"
assert_contains "clean run, short by one: the deviation fires" "$out" \
'2 tests failed, the baseline is 3'
echo
if [ "$failures" -eq 0 ]; then
echo "e2e-report-shape-test.sh: all checks passed"
+2 -25
View File
@@ -252,22 +252,8 @@ if [ -n "$baseline" ]; then
if [ "$expected" != "unknown" ] && [ "$expected" != "$baseline" ]; then
deviations+=("the runner started $expected tests, the baseline is $baseline")
fi
# `expected` is compared on every run and `failed` only on a run that finished, and the
# difference is the truncation this file already records rather than compares. `expected`
# comes from `Starting N tests`, which is printed before anything can abort, so it answers
# "is the marked set the size the baseline says" whatever happens afterwards. `failed` is a
# tally of what actually ran: on a truncated run the tests after the abort never start, so
# comparing it to the baseline announces a deviation about the framework dying rather than
# about the test list. Measured on 2026-09-05, two api37-debug dispatches of the same four
# marked tests: 4/4/4 and then 4/3/3, the second having lost the last test to the abort.
# Announcing that as "one now passes" is exactly the wrong reading, and #120 is the standing
# lesson about a notice that is wrong often enough to be skimmed past.
if [ "$failed" != "unknown" ] && [ "$failed" != "$baseline" ]; then
if [ "$completed" = "**no**" ]; then
echo "::debug::$failed of $baseline marked tests failed, on a run the abort truncated — not compared"
else
deviations+=("$failed tests failed, the baseline is $baseline — every test carrying the marker is expected to fail on this image, so fewer means one now passes and more means a new one joined")
fi
deviations+=("$failed tests failed, the baseline is $baseline — every test carrying the marker is expected to fail on this image, so fewer means one now passes and more means a new one joined")
fi
fi
if [ -n "$marked" ] && [ "$marked" != "$baseline" ]; then
@@ -297,16 +283,7 @@ if [ -n "$failed_names" ]; then
fi
if [ "$advisory" = "yes" ]; then
if [ "${#deviations[@]}" -eq 0 ]; then
# Two spellings, because one of them would be a lie half the time. `$baseline expected,
# $baseline failed` is only true of a run that finished; on a truncated one `failed` is a
# tally of the tests that got to run before the framework died, and printing the baseline in
# its place claims a number nobody measured. Seen on PR #245's advisory leg, which reported
# `failed: 4` three lines above `matches (5 expected, 5 failed)`.
if [ "$failed" != "unknown" ] && [ "$failed" != "$baseline" ]; then
echo " baseline: matches ($baseline expected; $failed of $baseline failed, on a run the abort truncated — not compared)"
else
echo " baseline: matches ($baseline expected, $baseline failed)"
fi
echo " baseline: matches ($baseline expected, $baseline failed)"
else
printf ' baseline DEVIATION: %s\n' "${deviations[@]}"
fi
+50 -66
View File
@@ -52,72 +52,40 @@ WEDGE_TIMEOUT=1200
# the same shape as E2E_EXTRA_GRADLE_ARGS below. The other four E2E legs run byte-identical
# commands with it unset.
#
# WHY IT RUNS HERE, BEFORE THE LOGCAT STREAM: it is a 45-second wait, and the stream below is
# meant to cover the suite rather than the wait. Everything this function counts comes from
# `adb logcat -d -b crash`, a fresh read each time and independent of the stream. (The original
# reason was stronger and no longer applies: `adb shell stop` would have ended the streamed
# `adb logcat` and nothing restarts it. There is no `stop` here any more -- see below.)
# WHY IT RUNS HERE, BEFORE THE LOGCAT STREAM: `adb shell stop` ends the `adb logcat` started
# below, and nothing restarts it, so a disable performed after that point would cost this leg
# its whole diagnostic story for the part of the run that matters. Everything this function
# counts comes from `adb logcat -d -b crash`, which is a fresh read each time and independent
# of the stream.
#
# WHAT IT IS FOR -- AND THE NAME IS NOW WRONG, WHICH IS WHY THIS PARAGRAPH IS LONG.
# The android-37.x images abort surfaceflinger from RegionSamplingThread inside their own gralloc
# mapper (docs/api-37-emulator-crash.md). surfaceflinger is a critical service, so init SIGKILLs
# zygote with it and the framework restarts under the run -- Gradle then reports
# WHAT IT IS FOR: the android-37.x images abort surfaceflinger from RegionSamplingThread inside
# their own gralloc mapper (docs/api-37-emulator-crash.md). surfaceflinger is a critical service,
# so init SIGKILLs zygote with it and the framework restarts under the run -- Gradle then reports
# `cmd: Can't find service: package` and `Starting 0 tests`. RegionSamplingThread exists only
# because SystemUI registers a nav-bar luma-sampling listener, so this was written to remove the
# package and with it the whole chain. Measured cadence of those kills on `-gpu host`: 20-90 s
# apart, median 60-70 s, three to five in a four-minute window.
#
# **THE DISABLE HALF OF THAT HAS NEVER WORKED, AND THE QUIET WINDOW IS WHAT THE LEG ACTUALLY
# GETS.** Measured 2026-09-05, two ways that agree:
#
# - On CI, in the gating leg of run 34006456986: `pm disable-user` is accepted at 02:28:37.9 and
# `com.android.systemui` really is in `pm list packages -d` at 02:29:33 -- and SystemUI is
# started anyway at 02:28:39.5 and again at 02:28:52.3, the second of which (pid 4275) is
# alive for the whole instrumentation run, logging `WindowManagerShell ...
# app=com.android.systemui` minutes after this function prints its final line.
# - Locally on android-37.0, with the package verified disabled before AND after a deliberate
# `stop; start`: `com.android.systemui` comes up 3 s after `system_server` regardless.
#
# So `pm disable-user --user 0 com.android.systemui` does not stop SystemUI starting on this
# image, whatever else happens. The name `E2E_DISABLE_SYSTEM_UI` and the name of this function are
# kept because the matrix row, both workflows and two documents refer to them, and a rename would
# touch all of that to no benefit -- read this comment, not the name.
#
# WHAT IS LEFT IS LOAD-BEARING, so do not delete the function as dead weight. It is the 45-second
# window with zero new `hasReadColorBufferDma` aborts. The boot-time aborts land close together --
# 02:28:18 and 02:28:43 in that same run -- and the wait is what puts instrumentation (02:32:42)
# after them rather than inside one. That is what stops a leg reporting `Starting 0 tests`, and it
# is why the three-round retry stays.
#
# THE `pm disable-user` CALL STAYS TOO, for a narrower reason than it was written for: every green
# leg and every measurement quoted anywhere about this row was taken with it applied and SystemUI
# running. Removing it would change the configuration the numbers came from, which is not a change
# to make while fixing a flake.
#
# AND THE FRAMEWORK RESTART IS GONE, having been measured to be worse than nothing. It was written
# as `adb shell stop; adb shell start`, which are root-only; adbd is not root, so every leg printed
# `Must be root` twice and restarted nothing. Adding `adb root` made it real, and api37-debug run
# 34010167885 is what that looks like: `pm disable-user` reports success, the stop lands ~2 s later
# and kills system_server before PackageManager has flushed its delayed write of package
# restrictions, so the state is gone on the way back up -- `NOT DISABLED after the restart`, three
# rounds, `final state: SystemUI STILL ENABLED`, and the leg then reported `expected: 0,
# received: 0`. A 15 s pause before the stop does make the state survive (bisected locally), and it
# still does not help, because of the two measurements above. So the restart is removed rather than
# repaired: it cost the leg every test it had, and there is nothing for it to buy.
# because SystemUI registers a nav-bar luma-sampling listener, so removing the package removes
# the whole chain. Measured cadence of those kills: 20-90 s apart, median 60-70 s, three to five
# in a four-minute window -- fast enough that install and instrumentation start-up do not fit
# inside one gap.
#
# NOTHING HERE TRUSTS A COMMAND'S OWN REPORT, and that is not paranoia: of four runs of an
# earlier one-shot version, one (32646029143) reported `new state: disabled-user` and then
# started SystemUI eight more times. So this reports what `pm list packages -d` says AND what
# `pidof` says, side by side, rather than one line implying both.
# started SystemUI eight more times, with ten more aborts. `pm disable-user` can be accepted by
# a system_server that is SIGKILLed before the state is written, and `pm disable-user` does not
# retract SystemUI's existing region-sampling registration either -- by the time boot completes
# it has already registered, so only a framework restart brings back a SystemUI-less
# surfaceflinger. Hence: disable, take the framework DOWN and confirm system_server is really
# gone (an earlier probe asked `service check` 0.3 s after `stop` and got `found` from the
# system_server that was still exiting, so its wait was not a wait), bring it back, verify the
# package against `pm list packages -d`, and require a 45 s window with zero new aborts.
# Three rounds, because one is not reliable and the failure is silent.
# ---------------------------------------------------------------------------
count_aborts() { adb logcat -d -b crash 2> /dev/null | grep -c 'hasReadColorBufferDma'; }
systemui_disabled() { adb shell pm list packages -d 2> /dev/null | grep -q 'com.android.systemui'; }
systemui_pid() { adb shell pidof com.android.systemui 2> /dev/null | tr -d '\r\n'; }
disable_region_sampling() {
local round=1 i out pid before after
local round=1 i out before after
while [ "$round" -le 3 ]; do
echo "--- round $round ---"
echo "--- SystemUI disable, round $round ---"
for i in $(seq 1 10); do
out="$(adb shell pm disable-user --user 0 com.android.systemui 2>&1 | tr -d '\r')"
echo " pm attempt $i: $out"
@@ -125,20 +93,36 @@ disable_region_sampling() {
sleep 5
done
echo " restarting the framework"
adb shell stop
for i in $(seq 1 20); do
[ -z "$(adb shell pidof system_server 2> /dev/null | tr -d '\r\n')" ] && break
sleep 2
done
echo " system_server down after ~$((i * 2)) s"
adb shell start
for i in $(seq 1 30); do
if adb shell service check package 2> /dev/null | grep -q ': found' \
&& adb shell service check activity 2> /dev/null | grep -q ': found' \
&& [ -n "$(adb shell pidof system_server 2> /dev/null | tr -d '\r\n')" ]; then
echo " services back after ~$((i * 5)) s"
break
fi
sleep 5
done
if systemui_disabled; then
echo " pm list packages -d: com.android.systemui is in it"
echo " verified: com.android.systemui is in pm list packages -d"
else
echo " pm list packages -d: com.android.systemui is NOT in it"
echo " NOT DISABLED after the restart -- the package state did not survive"
round=$((round + 1))
continue
fi
# Printed next to the line above precisely because the two disagree on this image, and a
# reader who sees only the first will believe something that is not true.
pid="$(systemui_pid)"
echo " com.android.systemui pid: ${pid:-none} (expected: a pid -- see the header)"
before="$(count_aborts)"
sleep 45
after="$(count_aborts)"
echo " aborts: $((after - before)) new in 45 s (total ${after:-0})"
echo " abort rate, SystemUI disabled: $((after - before)) new in 45 s (total ${after:-0})"
[ "$((after - before))" -eq 0 ] && break
echo " still aborting after round $round"
round=$((round + 1))
@@ -147,16 +131,16 @@ disable_region_sampling() {
# A warning rather than an exit. If the disable did not take, the run is about to report
# `Starting 0 tests` and fail on its own -- and it will do so with the logcat, the crash
# buffer and the diagnostics attached, which is more useful than dying here with none of it.
if [ "$((after - before))" -eq 0 ]; then
echo " final state: 45 s with no new aborts -- the suite starts here"
if systemui_disabled; then
echo " final state: SystemUI disabled"
else
echo "::warning::E2E api${LABEL}: still aborting after three rounds -- expect INSTRUMENTATION_ABORTED"
echo "::warning::E2E api${LABEL}: SystemUI is still enabled -- expect INSTRUMENTATION_ABORTED"
fi
return 0
}
if [ "${E2E_DISABLE_SYSTEM_UI:-}" = "1" ]; then
echo "::group::E2E api${LABEL} -- waiting out the boot-time gralloc aborts"
echo "::group::E2E api${LABEL} -- removing the region-sampling listener"
disable_region_sampling
echo "::endgroup::"
fi
+28 -22
View File
@@ -28,15 +28,6 @@ name: API 37 debug
# - It does not fork .github/scripts/e2e-run.sh. That script owns the FAILED-vs-WEDGED
# split, the SIGQUIT thread dump and the streamed logcat, and it is the copy CI
# exercises every day. This calls it, exactly as status_check.yml does.
#
# The SystemUI disable below is the exception, and it is a real one: this workflow
# drives it from its own probe step so `disable_system_ui` can be turned off for a
# dispatch, where the real leg gets it through `E2E_DISABLE_SYSTEM_UI`. Two copies of
# that logic therefore exist and must be changed together. **This instrument is also
# what established that the disable half of it does nothing** -- run 34010167885, in
# which making its framework restart real cost the leg every test it had. Read
# .github/scripts/e2e-run.sh's header for the measurements; the restart is gone from
# both copies and what remains is the 45-second quiet window.
# - It does not change status_check.yml. If a configuration here turns out to work,
# the change to the real matrix is proposed separately.
#
@@ -300,30 +291,45 @@ jobs:
sleep 5
done
# NO FRAMEWORK RESTART. There was one here, and making it work (it needed
# `adb root`) is what proved the whole disable is ineffective on this image:
# SystemUI starts anyway, measured on CI and locally, and the restart itself
# loses the package state to PackageManager's delayed write and leaves the leg
# reporting `Starting 0 tests`. e2e-run.sh's header carries the measurements.
# What is left, and what is load-bearing, is the quiet window below.
# pm disable-user does not retract SystemUI's existing region-sampling
# registration -- by the time boot completes it has already registered. Only a
# framework restart brings back a SystemUI-less SurfaceFlinger. See
# disable_region_sampling in tools/local-emulator/run-e2e.sh.
echo " restarting the framework"
adb shell stop
for i in $(seq 1 20); do
[ -z "$(adb shell pidof system_server 2> /dev/null | tr -d '\r\n')" ] && break
sleep 2
done
echo " system_server down after $((i * 2)) s"
adb shell start
for i in $(seq 1 30); do
if adb shell service check package 2> /dev/null | grep -q ': found' \
&& adb shell service check activity 2> /dev/null | grep -q ': found' \
&& [ -n "$(adb shell pidof system_server 2> /dev/null | tr -d '\r\n')" ]; then
echo " services back after $((i * 5)) s"
break
fi
sleep 5
done
if systemui_disabled; then
echo " pm list packages -d: com.android.systemui is in it"
echo " verified: com.android.systemui is in pm list packages -d"
else
echo " pm list packages -d: com.android.systemui is NOT in it"
echo " NOT DISABLED after the restart -- the package state did not survive"
round=$((round + 1))
continue
fi
# Beside it, because the two disagree on this image and the first line alone
# reads as a claim about the process that is not true.
echo " com.android.systemui pid: $(adb shell pidof com.android.systemui 2> /dev/null | tr -d '\r\n')"
before="$(count_aborts)"
sleep 45
after="$(count_aborts)"
echo "--- aborts: $((after - before)) new in 45 s (total ${after:-0}) ---"
echo "--- abort rate, SystemUI disabled: $((after - before)) new in 45 s (total ${after:-0}) ---"
[ "$((after - before))" -eq 0 ] && break
echo " still aborting after round $round"
round=$((round + 1))
done
systemui_disabled && echo "final state: com.android.systemui is disabled in pm (it still runs)" || echo "final state: com.android.systemui is not even disabled in pm"
systemui_disabled && echo "final state: SystemUI disabled" || echo "final state: SystemUI STILL ENABLED -- expect Starting 0 tests"
fi
echo "--- crash buffer (tail 60) ---"
+25 -30
View File
@@ -254,33 +254,31 @@ jobs:
api-level: "36"
# API 37, and it is NOT the same device as the four rows above it.
#
# THE CAVEAT THAT USED TO BE HERE IS WITHDRAWN, 2026-09-05, and the
# withdrawal is good news. It said this leg "runs with SystemUI disabled
# and the framework restarted under it", that no other leg or Pixel run
# uses that configuration, and that anything depending on system UI must
# not trust this row. **None of that was ever true.** Measured: the
# framework restart is two root-only adb commands that answered `Must be
# root` on every leg ever run, and `pm disable-user` does not stop SystemUI
# starting on this image anyway -- in run 34006456986 the package is
# verified disabled at 02:29:33 and SystemUI (pid 4275) is up from 02:28:52
# for the whole run. So this row's device configuration is the same as the
# other four's, and a green here means what a green on 33-36 means.
# CAVEAT, read this before trusting a green here: this leg runs with
# SystemUI disabled and the framework restarted under it. No other leg
# and no Pixel run uses that configuration. It is defensible only because
# nothing THIS LEG RUNS touches system UI -- Media3, FFmpeg and
# WorkManager tests -- and because the alternative is no CI coverage of
# the level this app targets. **Anything that ever does depend on system
# UI must not trust this row.** E2E_DISABLE_SYSTEM_UI is what does it;
# .github/scripts/e2e-run.sh explains the mechanism and why every step of
# it is verified rather than assumed.
#
# E2E_DISABLE_SYSTEM_UI still exists and still runs, because what it
# actually buys is a 45-second window with no new gralloc aborts before the
# suite starts -- the boot-time ones land close together and instrumentation
# 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.
# "this leg" and not "this suite", since 2026-08-24, and the difference is
# now load-bearing: SafPickerRoundTripTest DOES touch system UI. It drives
# DocumentsUI and rotates the display, and both reach the gralloc mapper
# this image aborts in -- disabling SystemUI removes the IDLE trigger, not
# those. Measured per method on android-37.0: the ROTATION test takes the
# framework down (INSTRUMENTATION_ABORTED) and carries
# @FailsOnEmulatorApi37, so notAnnotation below keeps it off this row; the
# PICKER test passes and runs here like anything else. A rotation rebuilds
# every surface at once, and starting another app's activity does not.
#
# notAnnotation below keeps six 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 THREE of that class's tests now
# carry the marker -- the save through the picker (#226) joined on 2026-09-06
# by inheritance rather than measurement, since it opens 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.
# So this row does now run one test that depends on system UI, and the
# caveat above still applies to it: a green here is not evidence the picker
# works on a device with SystemUI running -- the Pixel release check is.
# docs/api-37-emulator-crash.md has the per-method measurements, and the
# correction that produced them.
#
# 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
@@ -290,12 +288,9 @@ 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 six tests that cannot be RUN on this image; they
# notAnnotation removes the three tests that do not pass on this image; they
# run in the advisory job below, off the same marker so they cannot end up
# 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 two picker tests behind it never report at all.
# docs/api-37-emulator-crash.md has the measurements.
# in both or neither. docs/api-37-emulator-crash.md has the measurements.
- label: "37"
api-level: "37.0"
disable-system-ui: "1"
+11 -198
View File
@@ -76,53 +76,17 @@ 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. **Six** of the 70 instrumented
tests cannot be *run* on that image, for three measured reasons and one 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
save through the picker (#226), carries the marker because it opens the same picker and a second
DocumentsUI dialog on top of it — **not** because it has ever been observed here. It cannot be:
the rotation test runs first and takes the framework down, so **all four** advisory runs at this
baseline report `expected: 6, received: 4, failed: 4`, and the four are the three Media3 tests
plus the rotation — runs 34041156680, 34041593697, 34042397320 and 34043502322. **Neither picker
test has ever reported on the advisory leg**, which is a correction to what the marker's own KDoc
says. All six carry
`@FailsOnEmulatorApi37` and run in a separate `continue-on-error` job; the gating leg runs the
other 64 — **the same 64 as before**, which is exactly how this paragraph went stale unnoticed.
**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
count `.github/scripts/e2e-report-shape.sh` greps. Cross-check against any run's shape rather
than trusting the sentence: a leg below 37 reports the first as `expected`, and the API 37
gating leg reports the second.
**That third reason is why "cannot pass" became "cannot be run" on 2026-09-05.** Four gating
runs were read logcat-first — 34006456986, 34001744574, 34001377499 and the green 34002313300 —
and each carries exactly two `hasReadColorBufferDma` aborts before the suite (surfaceflinger,
during boot and the SystemUI disable) and exactly **one** during it: `system_server`, thread
`TaskSnapshotPer`, always inside the picker test's window, and nothing else in the gating set
reached the mapper at all. Whether the leg went red was luck — one run passed the test and lost
the leg anyway with `failed: 0`, another passed it 0.6 s after the abort and went green. That is
#108, it cost roughly a third of the gating legs over the wave-4 landings (#190), and a marker
is what it needed. `docs/api-37-emulator-crash.md` has the timings.
**A second thing came out of those logcats, and it withdraws a caveat rather than adding one.**
The API 37 row was documented as the one leg running "with SystemUI disabled and the framework
restarted under it", which nothing else does. Neither half was ever happening: `adb shell stop`
and `start` are root-only and answered `Must be root` on every leg ever run, and `pm
disable-user` does not stop SystemUI starting on this image anyway — measured on CI and locally,
with and without a real restart. **So this row's device configuration is the same as the other
four's, and a green here means what a green at 33–36 means.** `E2E_DISABLE_SYSTEM_UI` is kept
under its now-stale name because what it really buys is a 45-second window with no new gralloc
aborts before the suite starts, which is load-bearing; `.github/scripts/e2e-run.sh`'s header is
where that is written down.
- **CI runs API 37, and it gates.** The matrix is 33/34/35/36/37. **Three** of the 60 instrumented
tests cannot pass on that image, for two unrelated reasons: two Media3 hardware transcodes fail
inside the emulator's own `c2.goldfish.h264.decoder`, and one SAF test takes the framework down
when it rotates the display. All three carry `@FailsOnEmulatorApi37` and run in a separate
`continue-on-error` job; the gating leg runs the other 57.
That job is still called `E2E API 37 Media3 hardware transcode (advisory)`, which no longer
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 six tests pass.
not read a green run as evidence those three 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,
@@ -142,7 +106,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 six tests are the one thing CI cannot answer
the Pixel 10 Pro XL before each release.** Those three tests are the one thing CI cannot answer
for.
On a device or emulator, build only the ABI it can execute:
@@ -166,9 +130,9 @@ install for code that can never run — and on API 37 the full APK does not fit
- The `model` package is excluded from `ReturnCount` and `CyclomaticComplexMethod` only. It is the
decision layer, where one branch is one documented user-visible outcome and the metric counts
answers rather than complexity. Every other rule still applies there.
- **Coverage is reported, not gated** — **94.2% of lines (2234/2372), 87.5% of branches
(1171/1338)**, measured 2026-09-05 with `./gradlew :app:jacocoTestReport`, against 628 JVM tests
in 96 classes.
- **Coverage is reported, not gated** — **92.8% of lines (2183/2352), 81.3% of branches
(1091/1342)**, measured 2026-09-02 with `./gradlew :app:jacocoTestReport`, against 584 JVM tests
in 87 classes.
**Every figure this file carried before 2026-08-24 was an artifact, roughly half the real one.**
Robolectric loads classes through its own sandbox classloader with no source location, JaCoCo
@@ -220,29 +184,11 @@ install for code that can never run — and on API 37 the full APK does not fit
- **coverage gaps** — the line never executes. Filtered to sites where JaCoCo reports `mi > 0`, a
concrete instruction no test runs, which is what separates a real gap from a partial branch on
a compound condition. That filter cut the candidate list roughly in half and was right to.
**Wave 4 found it wrong in both directions, though — use the two filters below instead.**
- **assertion gaps** — JaCoCo is green and nothing checks the answer. `MainActivity`'s rail and
bottom bar were both *executed* by `AppRootRestorationTest` and **transposing them passed the
entire suite**; so did swapping the two progress-notification strings, and swapping `Content`'s
two destinations. No coverage number would ever have found any of the three.
**Wave 4 (2026-09-02) corrected that first filter, and the correction is the reusable part.**
`mi > 0` fails in both directions. It *over-reports* on Compose: `JoinScreen.kt:222` reads
`mi=10` and also `ci=38`, and `JoinStateAffordancesTest` already clicks that Save button and
asserts `save:joined.mp4` — the missed instructions are the synthesized `$changed`/`$dirty`
recomposition-skip path, the same codegen this file already warns about for *branch* counts,
showing up in the instruction count too. And it *under-reports* on warm methods with cold arms:
`ConversionViewModel.cancel()` misses no line, yet `activeWorkId?.let(...)` had only ever been
entered on the null side in 584 tests. Use two filters together instead:
- **`ci == 0`** — the line never executed. This is JaCoCo's own missed-line definition, so it
totals exactly the reported missed-line count and needs no judgement.
- **`ci > 0 && mb > 0` at method level** — a covered method with an arm nothing takes. This is
the only one that finds the `cancel()` shape.
Of wave 4's 251 missed branches, just **18** sat on lines that do execute, so the branch gap and
the line gap are largely the same gap; the second filter is about which of them are reachable.
So **every ticket named the mutation that had to go red, and that was its acceptance criterion
rather than a coverage delta**. It caught **two vacuous tests written in the same session**,
before either shipped:
@@ -290,114 +236,6 @@ install for code that can never run — and on API 37 the full APK does not fit
And **re-measure before quoting**: this entry was once written quoting 81.4%, measured four hours
earlier, and was already three points stale by the time it was ready to merge.
**Wave 4's read (2026-09-02) moved no number at all, and that is its result.** It was a triage
rather than a test push: twelve tickets (**#192-#203**), four deferred candidates (**#204**), and
five findings (**F6-F10** in `docs/coverage-read-findings.md`). What it establishes is the shape
of what is left, which is different again from wave 3's:
- Of 169 never-executed lines, **81 are native or device edges and stay that way** —
`FFmpegEngine` 33, `Media3Engine` 24, `ConcatEngine` 14, `MainActivity.onCreate` 10 — their
zeroes being the `testDebugUnitTest`-only measurement boundary that #84, #85, #86 and #88 each
recorded before. A further **34 are device-bound only until a seam moves them**:
`AndroidDeviceCodecs` 20 (#194) and the 14 of `MediaProbe`'s 26 that are `readMediaInformation`
(#195). Do not read that second group as exempt — the two tickets exist because it is not.
- Most of the rest is **already closed with a reason on record**, or compiler-generated: default-arg
bridges, DI factory lambdas, synthetic `NoWhenBranchMatchedException` arms, coroutine completion.
- Six of the ten findings in that document are now "no action" or "not a test gap". By this point
the report's remaining red is mostly arms nothing can reach, members nothing calls, and arms a
test *can* reach but cannot pin — and a coverage number tells none of them apart.
**The biggest single gap it found was not a missed line.** `ConversionViewModel.cancel()` and
`JoinViewModel.cancel()` report every line covered; only the null arm of
`activeWorkId?.let(workManager::cancelWorkById)` had ever been entered, so nothing in 584 tests
connected the Cancel button to WorkManager (#192). That is what the second filter above is for.
It also re-opened a mechanism, not a close: #86 and #133 ruled `AndroidDeviceCodecs.probe()` out
**through `ShadowMediaCodecList`**, on the grounds that the builder cannot set `isAlias` or
`canonicalName`. A pure seam does not have that constraint, and #133 did not evaluate one. Read
#194 before re-arguing either way — and note the reason it is worth cutting is not coverage but
that the `runCatching` fallback logs "assuming permissive" while returning empty sets, which makes
`canEncode` and `canDecode` answer *no* for everything.
**Wave 4's tests then landed on 2026-09-05**, as #206-#217 for the twelve tickets plus #218
(#159) and #219 (#122): 92.8% -> **94.2%** line, 81.3% -> **87.5%** branch, 584 -> 628 tests in 87
-> 96 classes. Missed lines 169 -> 138, missed branches 251 -> 167.
**Its branch move is a different animal from the 2026-08-29 seam work's, and the difference is the
point.** That one gained 6.3 branch points with the numerator up 37 (974 -> 1011) while the
denominator *fell* 70 (1410 -> 1340) — much of the rise was scaffolding leaving the measurement
rather than arms being covered. Here the numerator is up **80** (1091 -> 1171) and
the denominator moved **-4** (1342 -> 1338). So this one is almost entirely tests choosing arms
nothing had chosen, which is what the entry above warns to check before quoting a branch figure.
The line denominator rose the other way, 2352 -> 2372, and that is new production code rather than
untested code: the seams the wave cut — `capabilitiesFrom`, `ffprobeInfoFrom`, `sessionOutcome`,
and `sweepScope`/`startupSweep`.
**The two-filter method above is what found the work**, and its second filter earned its place:
the largest single gap of the wave (#192, the Cancel button never shown to reach WorkManager) sits
on lines that were already green and no line-level filter could see it.
One result worth carrying forward about *evidence* rather than coverage. #218 fixed a flake whose
reproduction is statistical, and running the whole suite six times per arm caught nothing either
way — at the observed rate a clean six-run arm is roughly a coin flip, so the comparison was
underpowered and proved nothing. What settled it was a deterministic mutation, and then the merge
train confirmed it by accident: the race reproduced on #217's Unit tests leg, which sits below
#218 and carries the unfixed scope. **Prefer a mutation that must go red to a repetition count**
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-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.
**It found one test that passes while testing nothing, and it is the one that matters most.**
`HardwareFallbackTest` is the only automated check of the hardware→software fallback against a
*real* codec failure, and on run `34004304566` the API 33, 34, 35 and 37 legs each log
`Routing sample_h264_444.mp4 -> ... via FFMPEG (NO_HARDWARE_ENCODER)` (API 36's logcat artifact on
that run is truncated, so it is unread rather than different): emulators expose no
hardware encoder, so the job never reaches Media3 and the `catch` it exists to prove is never
entered. Its two assertions — succeeded, output non-empty — are true anyway, and it finishes in
448 ms. **Deleting that `catch` reddens nothing on any leg** (#223).
Two things generalise from it. **A test can assert and still not reach**, which no coverage
number and no "does it assert something" review would catch — the filter that works is *does this
test's premise hold on the machine that runs it?*. And the codebase **already knew**: the sibling
`ForcedFailureTest` pins `DeviceCodecs.PERMISSIVE` against exactly this hazard and writes out why,
as does `ConversionWorkerTest`. The difference is that their assertions are about the *path*, so
without the pin they would fail loudly; `HardwareFallbackTest`'s are about the *output*, so it
passes quietly. **Prefer asserting the path over asserting the artefact** where the two differ.
The read was a triage, not a test push, and six of its seven findings are prose rather than code —
the suite itself is in good shape. What had drifted is its self-description.
**Working the tickets then found the thing the read could not: one production defect.** #238 —
joining files picked through the system picker failed outright on the stream-copy path. The
concat demuxer whitelists protocols separately from `-safe 0`, and `ffkitsaf` was not on the
list; only `STREAM_COPY` feeds it a list file, and every existing join test passed
`Uri.fromFile`, so **the one broken combination was the only one a user could reach**. Not a
missed line and not an unasserted value — two covered things no test put together, which is the
gap shape a coverage number is worst at.
**E7 is the other reusable result**, because it re-scoped its own ticket. A real
`DocumentsProvider` cannot be reached without the picker: an unprotected one is refused at
install, instrumentation runs in the app's uid so the test APK's identity is no help, and shell
identity is denied too — each denial naming `ACTION_OPEN_DOCUMENT`. So #226 has no cheap headless
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.
- **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.
@@ -525,29 +363,4 @@ Because versions float, a build can change without a commit. `./gradlew :app:dep
run instead is `timeout` on the `Test` tasks plus the jstack watchdog beside it in
`app/build.gradle.kts`, neither of which moves a thread. `HangBoundTest` guards both numbers,
and **a timed-out run writes no XML for the class that hung** — the dump is its only
attribution, so do not delete the watchdog as stray config. It has since been exercised in anger:
on 2026-09-05 it caught #125's Room/WorkManager deadlock on CI, failing in 10m57s with the hung
test named, where that ticket had predicted a 60-minute cap and no cause. #125 is closed as
bounded on the strength of it — the inversion itself is internal to the two libraries and still
live at `work-runtime` 2.11.2 / `room` 2.7.0.
- **The JVM suite does not run `LibreMediaConverterApp`.** `app/src/test/resources/robolectric.properties`
names `TestLibreMediaConverterApp` for every test, and it differs from the real class in exactly
one thing: `sweepScope` is `Dispatchers.Unconfined`, so the startup staging sweep finishes before
`onCreate()` returns instead of running on `Dispatchers.IO`.
**That line is load-bearing — do not delete it as stray config.** Robolectric builds an
`Application` per test class that asks for one, and each `onCreate` launched a sweep over the
shared `<cacheDir>/conversions/` that nothing joined. So a test asserting about a staged file was
racing every sweep the classes before it had left in flight (#159). It was CI-only until wave 4
added ten Robolectric classes, at which point `OutputPublisherStagingTest` failed on roughly one
local run in six. Per-test opt-in was measured and rejected: **27 of the 58 Robolectric classes
touch that directory**. The `SupervisorJob` is kept in the test scope so a throwing sweep is
swallowed there exactly as in production — the dispatcher is the only intended difference.
**It cost one assertion, knowingly.** `AppStartSweepTest` used to open by asserting that the
manifest's `android:name` is what Robolectric instantiated, so the sweep is code that actually
runs. An `application=` override *replaces* the manifest rather than being checked against it, and
`applicationInfo.className` reports the override too — measured — so that claim is not merely
unasserted on the JVM now, it is unobservable, and a rewritten version would assert the override
against itself. **The manifest link is device-only.** What remains is the `as LibreMediaConverterApp`
cast in that class's `setUp`, which catches only the test app ceasing to extend the real one.
attribution, so do not delete the watchdog as stray config.
-22
View File
@@ -42,28 +42,6 @@
<action android:name="android.content.action.DOCUMENTS_PROVIDER" />
</intent-filter>
</provider>
<!--
A PLAIN provider, for the ffkitsaf bridge on the success path.
FFmpegKitConfig.getSafParameterForRead is on every real user conversion and was on no
passing test: they all pass Uri.fromFile, which takes the other arm. Only its failure
side was covered, by UnopenableUriTest naming an authority that does not exist.
The documents provider above cannot serve this. Any DOCUMENTS_PROVIDER must hold
MANAGE_DOCUMENTS or the platform refuses to install it, instrumentation runs in the
target app's process and so carries the app's uid, and the resulting denial says what
is actually required: access obtained through ACTION_OPEN_DOCUMENT. That means a picker,
and the flake it brings. See issue #226.
The bridge does not need a documents provider. It opens a descriptor through the
resolver and hands FFmpeg a saf: path, so any readable content:// URI exercises it, and
an ordinary provider is allowed to be exported without a permission.
-->
<provider
android:name="org.libremediaconverter.saf.FixtureContentProvider"
android:authorities="org.libremediaconverter.test.content"
android:exported="true" />
</application>
</manifest>
@@ -1,7 +1,7 @@
package org.libremediaconverter
/**
* Marks an instrumented test that cannot be run on the `android-37.x` **emulator** system images.
* Marks an instrumented test that does not pass on the `android-37.x` **emulator** system images.
*
* This is a marker, not a skip. Nothing reads it except CI, and CI reads it twice — once with
* `notAnnotation` to build the gating API 37 leg, and once with `annotation` to build the advisory
@@ -9,23 +9,6 @@ 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 three things now, and it covered only the first until 2026-09-05.**
* Four of the six 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 is the new third thing: it is marked by inheritance, not by measurement.**
* `SafPickerRoundTripTest.aSaveWritesToTheDocumentTheSystemPickerCreated` (#226) opens the same
* picker and then a second DocumentsUI dialog on top of it, so it sits on the same task-snapshot
* path its sibling was marked for. It has never been observed at API 37 either way — see the
* measurement under [FAILS_ON_EMULATOR_API37_BASELINE], which is why it cannot be. Marking it was
* the conservative choice, and **the trigger for revisiting it is the rotation test, not itself**:
* 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,
* so this must never be read as "this test is allowed to fail at API 37" — only as "the API 37
@@ -34,7 +17,7 @@ package org.libremediaconverter
*
* Removing it is the goal, and the trigger is written down: a new API 37.x system image, or an
* ATD image for 37. Delete the annotation from the tests, and the advisory job goes empty and
* the gating one grows by [FAILS_ON_EMULATOR_API37_BASELINE].
* the gating one grows by two.
*
* **How many tests carry it is committed below**, as [FAILS_ON_EMULATOR_API37_BASELINE], and the
* advisory job checks the run against it. Adding or removing a marker means changing that number
@@ -54,37 +37,10 @@ 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 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 two
* of the six 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 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
* `e2e-report-shape.sh` compares `failed` only on a run that finished. `expected` is compared
* always — it comes from `Starting N tests`, which is printed before anything can abort, so it is
* the field that answers "is the marked set the size this number says". Read a *clean* run
* 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.** With six carriers the rotation test truncates the run before them: **all four** advisory
* runs at this baseline — 34041156680, 34041593697, 34042397320 and 34043502322 — report
* `expected: 6, received: 4, failed: 4`, and the four are the three Media3 tests plus the
* rotation. 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.
* **One number, both checks, and that is what the marker means.** A test carrying it cannot pass
* 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.
*
* 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
@@ -96,4 +52,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 = 6
const val FAILS_ON_EMULATOR_API37_BASELINE = 3
@@ -7,10 +7,6 @@ import androidx.media3.common.MimeTypes
import androidx.media3.common.util.UnstableApi
import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.test.platform.app.InstrumentationRegistry
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.cancelAndJoin
import kotlinx.coroutines.delay
import kotlinx.coroutines.launch
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.withTimeout
import org.junit.After
@@ -18,7 +14,6 @@ import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertNull
import org.junit.Assert.assertTrue
import org.junit.Assert.fail
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
@@ -267,92 +262,6 @@ class Media3EngineTest {
}
}
/**
* Cancelling a *running* export stops it, completing #224's third engine.
*
* The two FFmpeg engines were done first (`ad2a75d`, `d293646`); this is
* `Media3Engine.transcode`'s `invokeOnCancellation`, which posts `transformer.cancel()` onto the
* engine's own `HandlerThread` because `cancel()` has the same single-thread requirement as
* `start()`.
*
* ## Why the assertion is the output file here, and was not for FFmpeg
*
* The FFmpeg side could not use the file: `invokeOnCancellation` unlinks it, and on POSIX ffmpeg
* keeps writing to the unlinked inode, so the path stays gone whether or not the cancel landed.
* It asserted the session's return code instead.
*
* `Media3Engine` deletes nothing — the partial is `ConversionWorker`'s to clean up — so the file
* *is* the evidence. An export that was cancelled leaves no moov atom, so `MediaExtractor`
* either finds no video track or refuses the file outright with
* `IOException: Failed to instantiate extractor` — measured, and both mean interrupted. One
* that ran to completion leaves a playable HEVC file, which is the only outcome treated as a
* miss. The wait before
* reading it is deliberately several times the length of the export, so a *non*-cancelled export
* has certainly finished by then: the failure direction is "the file became valid", never "we
* did not wait long enough".
*
* ## Why it retries
*
* Same reason as the other two, measured there: the committed fixture is 3 s at 320x240 and the
* export outruns a naive cancel on a loaded runner. An attempt whose export finished before the
* cancel landed has tested nothing, so it is a miss and is retried; only exhausting
* [CANCEL_ATTEMPTS] fails. With `transformer.cancel()` removed every attempt produces a playable
* file, so the mutation still bites — it just takes five tries to say so.
*
* Progress having been reported is what proves the export really started, so a miss is
* distinguishable from an export that never ran at all — which matters on the API 37 image,
* where the decoder is what fails.
*/
@Test
@FailsOnEmulatorApi37
fun cancellingARunningExportStopsIt(): Unit = runBlocking {
val outcomes = mutableListOf<String>()
repeat(CANCEL_ATTEMPTS) { attempt ->
val partial = File(context.cacheDir, "cancelled_export_$attempt.mp4").apply { delete() }
val job = launch(Dispatchers.IO) {
engine.transcode(
input = Uri.fromFile(input),
output = partial,
request = ConversionRequest(OutputFormat.MP4_H265.spec),
)
}
// The muxer creating the file is proof the export really started, and it is the
// earliest such proof available -- earlier than the first progress tick.
withTimeout(TIMEOUT_MS) {
while (!partial.exists() && job.isActive) delay(POLL_MS)
}
val started = partial.exists()
job.cancelAndJoin()
if (!started) {
// The export failed before writing anything. That is not a cancellation result
// either way, so it is not allowed to pass as one.
outcomes += "attempt $attempt never produced an output file to cancel"
return@repeat
}
// Several times the export's own length, so a cancel that did not land has certainly
// finished. The failure direction is "the file became playable", never "too soon".
delay(SETTLE_MS)
// A cancelled export reports itself two ways and both mean the same thing: no video
// track, or MediaExtractor refusing the file outright with "Failed to instantiate
// extractor" because there is no moov atom to read. Only a *playable* file is a miss.
val video = runCatching { videoMimeTypeOf(partial) }.getOrNull()
partial.delete()
if (video == null) return@runBlocking
outcomes += "attempt $attempt produced a playable $video"
}
fail(
"never interrupted a running export in $CANCEL_ATTEMPTS attempts, so either every " +
"export finished first or cancellation does not reach the transformer: $outcomes",
)
}
private fun videoMimeTypeOf(file: File): String? {
val extractor = MediaExtractor()
try {
@@ -371,19 +280,6 @@ class Media3EngineTest {
private companion object {
const val TIMEOUT_SECONDS = 120L
/** Bounds the wait for the muxer to create the file; a hang here is a defect. */
const val TIMEOUT_MS = 30_000L
const val POLL_MS = 25L
/**
* How long to let a *failed* cancel finish. Several times the export's own length, so
* "the file is not playable" cannot mean "not yet".
*/
const val SETTLE_MS = 10_000L
/** See the KDoc: a miss is the loaded-runner case, not a defect. */
const val CANCEL_ATTEMPTS = 5
/**
* Short on purpose. Nothing is decoded or encoded on this path — the builder refuses the
* input outright — so anything approaching this is a hang, which is what the test is
@@ -13,7 +13,6 @@ import androidx.work.WorkManager
import androidx.work.Worker
import androidx.work.WorkerParameters
import androidx.work.workDataOf
import kotlinx.coroutines.CompletableDeferred
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.withTimeout
@@ -28,10 +27,7 @@ import org.junit.runner.RunWith
import org.libremediaconverter.join.JoinState
import org.libremediaconverter.join.JoinViewModel
import org.libremediaconverter.model.ConcatStrategy
import org.libremediaconverter.model.ConversionRequest
import org.libremediaconverter.model.Engine
import org.libremediaconverter.model.OutputFormat
import org.libremediaconverter.model.QualityTier
import org.libremediaconverter.work.ConcatWorker
import org.libremediaconverter.work.ConversionWorker
import org.libremediaconverter.work.JobTags
@@ -68,26 +64,6 @@ class EchoWorker(context: Context, params: WorkerParameters) : Worker(context, p
* path, foreground service included — into a synchronous test double, depending on class order.
*/
@UnstableApi
/**
* A [SoftwareTranscoder] that holds the worker in [WorkInfo.State.RUNNING] until released.
*
* Declared here rather than in `FakeFailures` because it is the only test that needs a job to stay
* live on demand, and the shape is specific to that: the others fake a *failure*, this fakes
* *duration*.
*/
private class BlockingTranscoder(private val released: CompletableDeferred<Unit>) : SoftwareTranscoder {
override suspend fun run(
request: ConversionRequest,
inputPath: String,
output: File,
durationMs: Long,
onProgress: (Int) -> Unit,
) {
released.await()
output.writeBytes(ByteArray(1_024))
}
}
@RunWith(AndroidJUnit4::class)
class ReattachOnLaunchTest {
@@ -99,13 +75,7 @@ class ReattachOnLaunchTest {
fun clearTheQueue() = emptyQueueAndStaging()
@After
fun leaveNothingBehind() {
// The suite runs without Android Test Orchestrator, so every class shares one process and
// a swapped seam outlives the class that set it. Only one test here swaps one, but a
// BlockingTranscoder left in place would hang the next class that converts anything.
ConversionDependencies.reset()
emptyQueueAndStaging()
}
fun leaveNothingBehind() = emptyQueueAndStaging()
/**
* The claim the whole fix rests on, checked against the production request builder rather
@@ -291,69 +261,6 @@ class ReattachOnLaunchTest {
return request.id
}
/**
* Reattaching to a conversion that is **running right now**, which nothing had ever driven.
*
* This class covers a job that finished, one whose staged file is gone, an ambiguous pair, one
* still queued, and one the user cancelled. [Reattachment.rank] gives
* [WorkInfo.State.RUNNING] the **highest** rank of all — "live work outranks a finished result
* because a running job is holding a foreground service" — and no test on either source set
* ever produced one. `ReattachmentTest` exercises the ranking as a pure function over
* fabricated snapshots; what was missing is a ViewModel meeting a real running job.
*
* It is also the likeliest reattachment there is: the user starts a conversion, leaves, and
* comes back while it is still going.
*
* ## Why the engine is a fake here, and why that is not a weakening
*
* The job has to still be running when the ViewModel is built, and every real conversion in
* this suite finishes in about a second — racing that is what made the cancellation tests flaky
* enough to need retries (#224). A [SoftwareTranscoder] that blocks until released removes the
* race outright: the job is `RUNNING` for exactly as long as the test wants.
*
* Nothing about reattachment depends on which engine is transcoding. What is under test is the
* tag query, [Reattachment.choose] over live WorkManager state, and `observe` mapping it to
* [ConversionState.Converting] — all of which run identically whatever is doing the work.
*
* ## What this does not do, and cannot (#230)
*
* It does not kill the process. `docs/defect-audit.md` D3/D13 record that `am kill` refuses a
* process holding a foreground service, and there is a more basic obstacle: **instrumentation
* runs in the app's own process**, so any route that really killed it would take the test
* runner with it and there would be nothing left to assert with. A relaunch-and-observe test
* needs two instrumentation runs, which the runner does not provide.
*
* So process death stays device-manual, and this is the closest observable analogue: a fresh
* ViewModel, with no memory of the work, meeting a job that is genuinely mid-flight.
*/
@Test
fun reattachesToAConversionThatIsStillRunning(): Unit = runBlocking {
val released = CompletableDeferred<Unit>()
ConversionDependencies.software = { BlockingTranscoder(released) }
val request = ConversionWorker.request(
inputUri = Uri.fromFile(stage("running_input.mp3")),
displayName = RUNNING_NAME,
sizeBytes = RUNNING_SIZE,
spec = OutputFormat.MP3.spec,
quality = QualityTier.FAST,
)
workManager.enqueue(request).result.get()
// Deterministic: the worker cannot finish until this test lets it.
withTimeout(TIMEOUT_MS) {
workManager.getWorkInfoByIdFlow(request.id).first { it?.state == WorkInfo.State.RUNNING }
}
val reattached = awaitConversion<ConversionState.Converting>()
assertEquals(RUNNING_NAME, reattached.input.displayName)
assertEquals(RUNNING_SIZE, reattached.input.sizeBytes)
released.complete(Unit)
workManager.cancelWorkById(request.id).result.get()
}
/**
* Enqueues a job that stays [WorkInfo.State.ENQUEUED]. The delay is what holds it there: it
* is long enough that nothing can run it during a test, and it is cancelled either way.
@@ -419,9 +326,5 @@ class ReattachOnLaunchTest {
* against WorkManager's database, so this is generous rather than tuned.
*/
const val SETTLE_MS = 5_000L
/** Read back off the job's tags by the reattaching ViewModel, so both have to survive. */
const val RUNNING_NAME = "still_running.mp3"
const val RUNNING_SIZE = 4_242L
}
}
@@ -12,17 +12,11 @@ import kotlinx.coroutines.withTimeout
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Assume.assumeTrue
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.codec.AndroidDeviceCodecs
import org.libremediaconverter.model.ConversionRequest
import org.libremediaconverter.model.ConversionRouter
import org.libremediaconverter.model.Engine
import org.libremediaconverter.model.OutputFormat
import org.libremediaconverter.model.QualityTier
import org.libremediaconverter.model.VideoCodec
import org.libremediaconverter.work.ConversionWorker
import java.io.File
@@ -40,47 +34,6 @@ import java.io.File
* hand — a regression test that silently skips is worse than no test, because the count
* still reads as coverage.
*
* ## Why this skips on emulators, and why that is the honest answer (#223)
*
* **This test used to pass everywhere while proving nothing.** Two independent facts stop the
* fallback happening on an emulator, and both were measured rather than reasoned:
*
* 1. **The router never sends the job to Media3.** A Fast MP4/H.265 job goes to the hardware path
* only when `device.canEncode(H265)`, and emulators expose no hardware encoder — every leg of
* run `34004304566` logged
* `Routing sample_h264_444.mp4 -> ... via FFMPEG (NO_HARDWARE_ENCODER)`. The whole test
* finished in 448 ms, which is not long enough to fail an export and then re-encode.
* 2. **Forcing it to Media3 does not help either, which is the part that settles it.** Pinning
* `ConversionDependencies.deviceCodecs` to [DeviceCodecs.PERMISSIVE] — the trick
* [ForcedFailureTest] uses — makes the router choose Media3, and the export then *succeeds*.
* Measured on a local API 34 emulator: `MediaCodecInfo` logs
* `NoSupport [codec.profileLevel, avc1.F4000C, video/avc]` for **both**
* `c2.goldfish.h264.decoder` and `c2.android.avc.decoder`, and ExoPlayer allocates the
* goldfish decoder anyway, which decodes the file regardless of the profile it declares.
* `c2.android.hevc.encoder` then encodes the result and the job reports `MEDIA3`.
*
* So the class KDoc above — "Media3 fails partway through the export on every device" — **is not
* true of the emulator images**, and no amount of routing pressure makes this fixture force a
* fallback there. The emulator cannot answer this question, so the test says so out loud instead
* of passing.
*
* That is why the gate is [assumeTrue] on the *production* premise (`canEncode(H265)`) rather than
* a pinned profile: pinning would also swap in software codecs, which is not the path a real
* device takes and is what made the forced run succeed. **This is now the third permanent skip**;
* the other two are [org.libremediaconverter.bench.RealMediaBenchmark]'s.
*
* `ForcedFailureTest.hardwareFailureFallsBackToSoftware` still covers the fallback *wiring* on
* every leg, with an `ExplodingHardware` double. What only a device with a real hardware encoder
* can show is two real engines disagreeing about a real file, and that is what this is for.
*
* ## Why the assertion is a pair
*
* `KEY_ENGINE_USED` is `FFMPEG` whether the fallback fired **or** the router went straight there,
* so asserting it alone would not have caught any of the above. The premise is asserted
* separately: [ConversionRouter.route] chooses `MEDIA3` for this request on this device. Static
* routing wanted hardware, the runtime result was software — together, and only together, that is
* the fallback.
*
* The fixture was produced with x264, which the host toolchain cannot do (Fedora's
* ffmpeg ships openh264, which is Constrained Baseline only):
*
@@ -113,15 +66,6 @@ class HardwareFallbackTest {
@Test
fun aFileMedia3CannotDecodeStillConvertsViaFfmpeg(): Unit = runBlocking {
// See "Why this skips on emulators" on the class. Without a real hardware encoder the
// router never chooses Media3, and forcing it makes the export succeed instead of fail --
// so there is no fallback to observe and a green run would mean nothing.
assumeTrue(
"no hardware HEVC encoder, so the router cannot choose Media3 and there is no " +
"fallback to exercise",
AndroidDeviceCodecs.get().canEncode(VideoCodec.H265),
)
val request = ConversionWorker.request(
inputUri = Uri.fromFile(input),
displayName = SAMPLE,
@@ -131,19 +75,6 @@ class HardwareFallbackTest {
// the tier where the fallback has to rescue the conversion.
quality = QualityTier.FAST,
)
// The premise, asserted rather than assumed: this request is one the router wants to send
// to hardware on this device. Without it the test is green whether the fallback fired or
// the job never went near Media3, which is exactly how #223 stayed invisible.
val decision = ConversionRouter.route(
ConversionRequest(OutputFormat.MP4_H265.spec, quality = QualityTier.FAST),
AndroidDeviceCodecs.get(),
)
assertEquals(
"this test only means something if the router sends this job to Media3",
Engine.MEDIA3,
decision.engine,
)
workManager.enqueue(request).result.get()
val terminal = withTimeout(TIMEOUT_MS) {
@@ -157,14 +88,6 @@ class HardwareFallbackTest {
terminal?.state,
)
// The outcome. Paired with the routing assertion above this is the fallback and nothing
// else: hardware was chosen, software is what ran.
assertEquals(
"the router chose Media3, so a successful job must have fallen back to FFmpeg",
Engine.FFMPEG.name,
terminal?.outputData?.getString(ConversionWorker.KEY_ENGINE_USED),
)
val out = File(terminal!!.outputData.getString(ConversionWorker.KEY_OUTPUT_PATH)!!)
assertTrue("no output produced", out.exists() && out.length() > 0)
out.delete()
@@ -4,20 +4,10 @@ import android.media.MediaExtractor
import android.media.MediaFormat
import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.test.platform.app.InstrumentationRegistry
import com.arthenica.ffmpegkit.FFmpegKit
import com.arthenica.ffmpegkit.FFmpegSession
import com.arthenica.ffmpegkit.ReturnCode
import com.arthenica.ffmpegkit.SessionState
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.cancelAndJoin
import kotlinx.coroutines.delay
import kotlinx.coroutines.launch
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.withTimeout
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Assert.fail
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
@@ -123,11 +113,6 @@ class FFmpegEngineTest {
fun encodesFlacLosslessAudio() {
val out = convert(OutputFormat.FLAC)
assertTrue("no FLAC produced", out.exists() && out.length() > 0)
// "fLaC", the native FLAC stream marker. Without this the test passed on any non-empty
// file, so a builder arm emitting the wrong encoder into a .flac name shipped green
// (#228) -- the same shape the five assertions above already guard against.
val magic = out.inputStream().use { String(it.readNBytes(4), Charsets.US_ASCII) }
assertEquals("fLaC", magic)
}
@Test
@@ -142,165 +127,6 @@ class FFmpegEngineTest {
fun encodesOpus() {
val out = convert(OutputFormat.OPUS)
assertTrue("no Opus produced", out.exists() && out.length() > 0)
// OutputFormat.OPUS is Container.OGG, so the file is an Ogg stream: "OggS" (#228).
// Deliberately the container marker rather than the codec -- it is what the other
// container-level assertions in this class check, and it is four bytes at offset 0.
val magic = out.inputStream().use { String(it.readNBytes(4), Charsets.US_ASCII) }
assertEquals("OggS", magic)
}
/**
* The percentage itself, which every other test in this class computes and none of them reads.
*
* `FFmpegEngine` derives progress as `stats.time / durationMs * 100`, and the statistics
* callback runs on every conversion here — but every call site omits `onProgress`, so until
* this test nothing on any source set had ever looked at the number (#229). #196 covered the
* *worker's* progress lambda, and did it with a fake engine that reports whatever the test
* tells it to; `ProgressNotificationTest` covers throttling the same way. The arithmetic was
* the one part with no reader.
*
* ## Why the duration is deliberately wrong
*
* `sample_h264.mp4` is exactly 3.000 s, and this passes **30 s** as the duration. So the
* conversion still encodes the whole clip, `stats.time` still climbs to about 3000 ms, and the
* reported percentage tops out around **10** rather than 100.
*
* That is what makes the assertion bite. A range check alone is worthless here: replacing
* `percent` with a constant `0` satisfies "every value is in 0..100" and "the values never go
* backwards", and so does a list of `[0, 100]`. Pinning the *band* rejects every constant, and
* — because the band is a tenth of the way up — it also rejects an implementation that ignores
* `durationMs`, which would report ~100 for the same run.
*
* The bound is deliberately loose (5..25 for an expected 10). The last statistics callback can
* land slightly before the final frame, so the peak is "about 3000 ms of a claimed 30 000",
* not exactly it.
*/
@Test
fun progressIsReportedAsAFractionOfTheDurationItWasGiven() {
val seen = mutableListOf<Int>()
val out = outputFor("out_progress.mp4")
runBlocking {
engine.run(
request = ConversionRequest(spec = OutputFormat.MP4_H264.spec, quality = QualityTier.BEST),
inputPath = input.absolutePath,
output = out,
// Ten times the fixture's real 3 s. See the KDoc.
durationMs = 30_000,
onProgress = { percent -> seen += percent },
)
}
assertTrue("the statistics callback never reported progress", seen.isNotEmpty())
assertTrue("progress out of range: $seen", seen.all { it in 0..100 })
assertEquals("progress went backwards: $seen", seen.sorted(), seen)
// The band. Rejects any constant, and rejects ignoring durationMs (which would read ~100).
val peak = seen.max()
assertTrue(
"3 s of media against a claimed 30 s should peak near 10%, got $peak from $seen",
peak in 5..25,
)
}
/**
* Cancelling a *running* conversion actually stops the native session.
*
* Nothing on any source set did this before (#224). Every `cancel` in `app/src/androidTest` is
* `WorkManager.cancelWorkById` against work that is **queued or already finished** — the two in
* `ReattachOnLaunchTest` cancel a job carrying a one-hour initial delay, and one immediately
* after enqueue. On the JVM, `WorkerCancellationTest` and `HardwareFallbackTest`'s cancellation
* case drive a `SoftwareTranscoder` double that records the call. No test had ever asked a real
* native session to stop. This is `docs/defect-audit.md` **D10**'s forcing condition.
*
* It is the one path where cancelling wrong is silently expensive rather than loudly broken: a
* missed `FFmpegKit.cancel` leaves the native process encoding to completion while the UI says
* the job is cancelled, and nothing reports the battery and thermal cost.
*
* ## Why the assertion is the session's return code, not the output file
*
* The obvious assertion — the partial output is gone — **cannot fail**, so it would have been a
* vacuous test. `invokeOnCancellation` deletes the path, and on POSIX unlinking a file ffmpeg
* still holds open leaves ffmpeg writing to the unlinked inode; the path stays gone whether or
* not the cancel ever reached the session. Deleting `FFmpegKit.cancel` and keeping
* `output.delete()` passes that check every time.
*
* What distinguishes them is the session's own verdict: a cancelled session ends with the
* cancel return code, a completed one ends successfully. That is a fact about the session
* rather than about timing, so it is read *after* waiting for the session to leave
* [SessionState.RUNNING] rather than at a fixed delay.
*
* ## Why it cancels on RUNNING rather than on the first progress callback
*
* Measured. Cancelling from the first `onProgress` was tried first and **failed on a local API
* 34 emulator with `state=COMPLETED rc=0`** — every committed fixture is 2-3 s at 320x240, and
* the encode finishes before the first statistics callback has been delivered and acted on. The
* progress callback proves the session is running, but arrives too late to interrupt anything.
* `FFmpegKit.listSessions` shows the session [SessionState.RUNNING] far earlier.
*
* ## Why it retries, which is the part that took two attempts to get right
*
* Waiting for `RUNNING` is not on its own enough. With `MP4_H265` at [QualityTier.BEST] this
* passed four consecutive local runs and all five CI legs, then failed on the API 34 and 35 legs
* of the next PR with `state=COMPLETED rc=0`. Nothing had changed: on a loaded runner the thread
* that observed `RUNNING` can be descheduled long enough for a short encode to finish before it
* calls `cancel`. A longer timeout does not help — the wait already succeeded.
*
* Two changes together, because neither is sufficient:
*
* - **A slower encode.** `WEBM_VP9` at `BEST` is the slowest thing this builder emits:
* `libvpx-vp9 -crf 31 -b:v 0`, with `-deadline realtime` added **only** on
* [QualityTier.FAST]. Probed on an API 34 emulator, that session is still `RUNNING` at 1 s
* and finished by 2 s, against well under a second for x265 `-preset medium`.
* - **Retrying the attempt.** An attempt whose session finished before the cancel landed has
* not tested anything, so it is not a failure — it is a miss, and it is retried. Only
* exhausting [CANCEL_ATTEMPTS] is a failure, and its message says which case it hit.
*
* That keeps the mutation honest: with `FFmpegKit.cancel` removed **every** attempt ends
* `COMPLETED`, so the test still fails — it just takes [CANCEL_ATTEMPTS] tries to say so.
*
* The session is identified by diffing against the ids present before each attempt, because
* this class has already produced eight of them by the time this executes.
*/
@Test
fun cancellingARunningConversionCancelsTheNativeSession(): Unit = runBlocking {
val outcomes = mutableListOf<String>()
repeat(CANCEL_ATTEMPTS) { attempt ->
val before = FFmpegKit.listSessions().map { it.getSessionId() }.toSet()
val out = outputFor("out_cancelled_$attempt.webm")
val job = launch(Dispatchers.IO) {
engine.run(
// The slowest target this builder emits -- see the KDoc. Not decoration:
// with a faster one this loses the race on a loaded CI runner.
request = ConversionRequest(spec = OutputFormat.WEBM_VP9.spec, quality = QualityTier.BEST),
inputPath = input.absolutePath,
output = out,
durationMs = 3_000,
)
}
val ours = withTimeout(TIMEOUT_MS) {
var found: FFmpegSession? = null
while (found == null) {
found = FFmpegKit.listSessions().firstOrNull { it.getSessionId() !in before }
if (found == null) delay(POLL_MS)
}
found
}
job.cancelAndJoin()
withTimeout(TIMEOUT_MS) {
while (ours.getState() == SessionState.RUNNING) delay(POLL_MS)
}
if (ReturnCode.isCancel(ours.getReturnCode())) return@runBlocking
// The encode beat us to it. That attempt proved nothing either way, so try again.
outcomes += "state=${ours.getState()} rc=${ours.getReturnCode()}"
}
fail(
"never interrupted a running session in $CANCEL_ATTEMPTS attempts, so either every " +
"encode finished first or cancellation does not reach it: $outcomes",
)
}
// --- the quality tier the GPL licence was taken for --------------------
@@ -340,19 +166,4 @@ class FFmpegEngineTest {
}.exceptionOrNull()
assertTrue("expected an FFmpegException, got $failure", failure is FFmpegEngine.FFmpegException)
}
private companion object {
/** Generous: it bounds a hang, and every wait here normally settles in well under a second. */
const val TIMEOUT_MS = 30_000L
const val POLL_MS = 50L
/**
* How many times to try to catch the session mid-encode.
*
* Each miss costs about the length of one VP9 encode -- a second or two -- and a miss is
* the loaded-runner case rather than a defect. Five is enough that exhausting them means
* cancellation is not reaching the session, which is what the failure message says.
*/
const val CANCEL_ATTEMPTS = 5
}
}
@@ -5,29 +5,17 @@ import android.media.MediaFormat
import android.net.Uri
import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.test.platform.app.InstrumentationRegistry
import com.arthenica.ffmpegkit.FFmpegKit
import com.arthenica.ffmpegkit.FFmpegSession
import com.arthenica.ffmpegkit.ReturnCode
import com.arthenica.ffmpegkit.SessionState
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.cancelAndJoin
import kotlinx.coroutines.delay
import kotlinx.coroutines.launch
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.withTimeout
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Assert.fail
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.convert.MediaProbe
import org.libremediaconverter.convert.StagingNames
import org.libremediaconverter.ffmpeg.ConcatEngine
import org.libremediaconverter.ffmpeg.FFmpegEngine
import org.libremediaconverter.model.ConcatStrategy
import org.libremediaconverter.work.ConcatWorker
import java.io.File
/**
@@ -62,82 +50,6 @@ class ConcatEngineTest {
(staged + listOf(clipA, clipB, clipMismatched)).forEach { it.delete() }
}
/**
* Cancelling a *running* join actually stops the native session.
*
* The `FFmpegEngine` half of #224 landed first (PR #236); this is the same gap in
* [ConcatEngine]. Before these two, no test on any source set had ever asked a real native
* session to stop — every `cancel` in `app/src/androidTest` targets WorkManager entries that
* are queued or already finished.
*
* ## Two things carried over from the conversion side, both measured there
*
* **The assertion is the session's return code.** A cancelled session ends with the cancel
* code, a completed one does not. The alternative — checking the output file — is even less
* available here than it was for conversions: [ConcatEngine] does not delete its output on
* cancellation at all. Its `invokeOnCancellation` is `FFmpegKit.cancel(...)` and nothing else,
* where [org.libremediaconverter.ffmpeg.FFmpegEngine]'s also deletes the partial. Whether that
* asymmetry is deliberate is a separate question from this test, which is why this asserts the
* thing that is true of both.
*
* **The cancel is triggered on [SessionState.RUNNING], not on progress.** `ConcatWorker`
* publishes no progress at all, so there is no callback to hang it on even in principle — but
* the conversion side established the deeper reason: the committed clips are 2 s at 320x240 and
* the encode outruns a callback-triggered cancel.
*
* **And the attempt is retried**, for the reason the conversion side measured the hard way: on
* a loaded runner the thread that observed `RUNNING` can be descheduled long enough for a short
* encode to finish before it calls `cancel`, which failed two CI legs there. An attempt whose
* session finished first has tested nothing, so it is a miss rather than a failure; only
* exhausting [CANCEL_ATTEMPTS] fails, and with `FFmpegKit.cancel` removed every attempt misses,
* so the mutation still bites.
*
* The inputs are deliberately the **mismatched** pair, so [ConcatStrategy.REENCODE] is chosen.
* A stream copy of two short clips is close to instantaneous and would leave nothing to
* interrupt; re-encoding is the case where a user would actually reach for Cancel.
*
* *Mutation:* drop `FFmpegKit.cancel(session.getSessionId())` from `ConcatEngine`'s
* `invokeOnCancellation` — the session runs to completion and this fails.
*/
@Test
fun cancellingARunningJoinCancelsTheNativeSession(): Unit = runBlocking {
val outcomes = mutableListOf<String>()
repeat(CANCEL_ATTEMPTS) { attempt ->
val before = FFmpegKit.listSessions().map { it.getSessionId() }.toSet()
val out = output("cancelled_join_$attempt.mp4")
val job = launch(Dispatchers.IO) {
engine.join(
listOf(Uri.fromFile(clipA), Uri.fromFile(clipMismatched)),
out,
ConcatWorker.DEFAULT_FORMAT,
)
}
val ours = withTimeout(TIMEOUT_MS) {
var found: FFmpegSession? = null
while (found == null) {
found = FFmpegKit.listSessions().firstOrNull { it.getSessionId() !in before }
if (found == null) delay(POLL_MS)
}
found
}
job.cancelAndJoin()
withTimeout(TIMEOUT_MS) {
while (ours.getState() == SessionState.RUNNING) delay(POLL_MS)
}
if (ReturnCode.isCancel(ours.getReturnCode())) return@runBlocking
outcomes += "state=${ours.getState()} rc=${ours.getReturnCode()}"
}
fail(
"never interrupted a running join in $CANCEL_ATTEMPTS attempts, so either every " +
"encode finished first or cancellation does not reach it: $outcomes",
)
}
private fun copyAsset(name: String): File {
val out = File(context.cacheDir, name)
InstrumentationRegistry.getInstrumentation().context.assets
@@ -237,55 +149,6 @@ class ConcatEngineTest {
)
}
/**
* A failed join tells the user the return code and what FFmpeg said.
*
* **This is the device half of #203/#217**, whose PR closed by noting the join legs had not
* been run. Running them would not have answered it: nothing on either source set drove a real
* join *failure*, so the unified message was asserted only against values a JVM test hands to
* `sessionOutcome` directly.
*
* What is device-only here is that the three reads behind that message work against a real
* native session at all — `getReturnCode`, `getFailStackTrace` and `getAllLogsAsString`. If
* the log tail came back null or empty on a device, the user would get `Joining failed (1): `
* with nothing after the colon and every JVM test would still pass.
*
* **What this deliberately does not pin is the preference between the two detail sources.** On
* an ordinary non-zero return code FFmpegKit reports no fail stack trace, so the stack-trace-
* first rule and the log-tail-first rule produce the same text and no assertion here can tell
* them apart. That ordering is [SessionOutcomeTest][org.libremediaconverter.ffmpeg.SessionOutcomeTest]'s
* job, where both sources can be non-blank at once. Asserting it here would be a test whose
* KDoc claims more than it checks — the `probeForConcat` mistake wave 3 caught.
*
* The failure is forced with an input that does not exist, which the concat demuxer rejects
* the same way on every FFmpeg build, rather than with malformed media whose handling varies.
*/
@Test
fun aFailedJoinReportsTheReturnCodeAndWhatFFmpegSaid(): Unit = runBlocking {
val missing = File(context.cacheDir, "no_such_clip.mp4").also { it.delete() }
val out = output("joined_failure.mp4")
val failure = runCatching {
engine.join(listOf(Uri.fromFile(clipA), Uri.fromFile(missing)), out)
}.exceptionOrNull()
assertTrue(
"a join over a missing input must fail, got $failure",
failure is FFmpegEngine.FFmpegException,
)
val message = failure?.message.orEmpty()
assertTrue(
"the message must name the operation and carry the return code, was: '$message'",
message.startsWith("Joining failed ("),
)
// The half a JVM test cannot reach: a real session actually produced detail to show.
val detail = message.substringAfter("): ", "")
assertTrue(
"the message stopped at the return code and told the user nothing, was: '$message'",
detail.isNotBlank(),
)
}
@Test
fun theListFileIsCleanedUpAfterJoining(): Unit = runBlocking {
val out = output("joined_cleanup.mp4")
@@ -317,13 +180,4 @@ class ConcatEngineTest {
a.width != mismatched.width || a.height != mismatched.height,
)
}
private companion object {
/** Generous: it bounds a hang, and both waits here normally settle in well under a second. */
const val TIMEOUT_MS = 30_000L
const val POLL_MS = 50L
/** See the conversion side: a miss is the loaded-runner case, not a defect. */
const val CANCEL_ATTEMPTS = 5
}
}
@@ -1,123 +0,0 @@
package org.libremediaconverter.saf
import androidx.media3.common.util.UnstableApi
import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.test.platform.app.InstrumentationRegistry
import androidx.work.WorkInfo
import androidx.work.WorkManager
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.withTimeout
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.ffmpeg.ConcatEngine
import org.libremediaconverter.model.Engine
import org.libremediaconverter.model.OutputFormat
import org.libremediaconverter.model.QualityTier
import org.libremediaconverter.work.ConversionWorker
import java.io.File
/**
* A `content://` input reaching FFmpeg successfully, which nothing had ever driven (#225).
*
* `FFmpegKitConfig.getSafParameterForRead` stands between a SAF grant and the native process, and
* it is on **every real user conversion**. Every passing convert and join test in this suite hands
* the worker a `Uri.fromFile(...)`, which takes the `uri.path` arm instead — so the bridge was
* exercised only on its failure side, by `UnopenableUriTest` naming an authority that does not
* exist. That proves the error message, not the bridge.
*
* ## Why a plain provider rather than the documents one
*
* [FixtureDocumentsProvider] cannot be reached from the app, measured three ways on an API 34
* emulator (#226): a `DOCUMENTS_PROVIDER` declared without `MANAGE_DOCUMENTS` is refused at install
* — *"Provider must be protected by MANAGE_DOCUMENTS"*; instrumentation runs in the **target app's
* process**, so `Instrumentation.getContext()` still carries the app's uid and is denied; and
* `adoptShellPermissionIdentity(MANAGE_DOCUMENTS)` is denied identically. The denial names the only
* way in: *"you obtain access using ACTION_OPEN_DOCUMENT or related APIs"*.
*
* The bridge does not need one. It opens a descriptor through the resolver and hands FFmpeg a
* `saf:` path, so any readable `content://` URI exercises it — and [FixtureContentProvider] is an
* ordinary provider, which may be exported without a permission. The whole class is headless: no
* DocumentsUI, and none of the flake #190 records.
*
* ## Why MP3
*
* The bridge lives on the FFmpeg arm, and MP3 is the format the router sends there unconditionally
* — no platform encoder exists at any API level, so `ConversionWorkerTest.routesAnMp3JobToFfmpeg…`
* relies on the same fact. Choosing a video target would make the engine depend on the device's
* codecs, and #223 is what that costs.
*
* *Mutation:* make `getSafParameterForRead` return `uri.toString()`. FFmpeg cannot open it and both
* tests fail; nothing else in either suite notices.
*/
@UnstableApi
@RunWith(AndroidJUnit4::class)
class ContentUriInputTest {
private val context = InstrumentationRegistry.getInstrumentation().targetContext
private val workManager = WorkManager.getInstance(context)
@After
fun tearDown() {
File(context.cacheDir, "conversions").listFiles()?.forEach { it.delete() }
}
@Test
fun aContentUriInputConvertsThroughTheSafBridge(): Unit = runBlocking {
val input = FixtureContentProvider.uriFor(SAMPLE)
val request = ConversionWorker.request(
inputUri = input,
displayName = SAMPLE,
sizeBytes = 0L,
spec = OutputFormat.MP3.spec,
quality = QualityTier.FAST,
)
workManager.enqueue(request).result.get()
val terminal = withTimeout(TIMEOUT_MS) {
workManager.getWorkInfoByIdFlow(request.id).first { it != null && it.state.isFinished }
}
val error = terminal?.outputData?.getString(ConversionWorker.KEY_ERROR)
assertEquals(
"a content:// input must convert, but failed with: $error",
WorkInfo.State.SUCCEEDED,
terminal?.state,
)
// The bridge is on the FFmpeg arm only, so this is part of the claim rather than colour.
assertEquals(Engine.FFMPEG.name, terminal?.outputData?.getString(ConversionWorker.KEY_ENGINE_USED))
val out = File(terminal!!.outputData.getString(ConversionWorker.KEY_OUTPUT_PATH)!!)
assertTrue("no output produced from a content:// input", out.exists() && out.length() > 0)
out.delete()
}
/**
* The same bridge on the join path, which has its own copy of the call (`ConcatEngine:36`).
*
* Driven through the engine rather than `ConcatWorker` because the engine is where the branch
* is; the worker adds a foreground service and nothing else this is about.
*/
@Test
fun contentUriInputsJoinThroughTheSafBridge(): Unit = runBlocking {
val out = File(context.cacheDir, "joined_from_content.mp4").apply { delete() }
val result = ConcatEngine(context).join(
listOf(FixtureContentProvider.uriFor(CLIP_A), FixtureContentProvider.uriFor(CLIP_B)),
out,
OutputFormat.MP4_H264,
)
assertTrue("no output produced from content:// inputs", result.output.length() > 0)
out.delete()
}
private companion object {
const val SAMPLE = "sample_h264.mp4"
const val CLIP_A = "clip_a.mp4"
const val CLIP_B = "clip_b.mp4"
const val TIMEOUT_MS = 300_000L
}
}
@@ -1,135 +0,0 @@
package org.libremediaconverter.saf;
import android.content.ContentProvider;
import android.content.ContentValues;
import android.database.Cursor;
import android.database.MatrixCursor;
import android.net.Uri;
import android.os.ParcelFileDescriptor;
import android.provider.OpenableColumns;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
/**
* A plain {@link ContentProvider} serving the committed media fixtures over {@code content://}.
*
* <p><b>Why this exists alongside {@link FixtureDocumentsProvider}.</b> Every passing convert and
* join test hands the worker a {@code Uri.fromFile(...)}, which takes the {@code uri.path} arm and
* never touches {@code FFmpegKitConfig.getSafParameterForRead}. That bridge is on 100% of real user
* conversions and was on 0% of tested ones; only its failure side was covered, by
* {@code UnopenableUriTest} pointing at an authority that does not exist.
*
* <p><b>Why not the documents provider.</b> It cannot be reached. Measured three ways on an API 34
* emulator: a {@code DOCUMENTS_PROVIDER} declared without {@code MANAGE_DOCUMENTS} is refused at
* install ("Provider must be protected by MANAGE_DOCUMENTS"); instrumentation runs in the target
* app's process, so {@code Instrumentation.getContext()} still carries the app's uid and is denied;
* and {@code adoptShellPermissionIdentity(MANAGE_DOCUMENTS)} is denied identically. The denial says
* what is required — <i>"you obtain access using ACTION_OPEN_DOCUMENT or related APIs"</i> — so a
* documents provider is reachable only through a picker-issued grant. See issue #226.
*
* <p>The bridge does not need one. {@code getSafParameterForRead} opens a file descriptor through
* the resolver and hands FFmpeg a {@code saf:} path; any readable {@code content://} URI exercises
* it. An ordinary provider may be exported without a permission, so this one is, and the whole test
* stays headless — no DocumentsUI, and none of the flake #190 records.
*
* <p>Unlike {@link FixtureDocumentsProvider} this may use {@code androidx} and Kotlin freely — it is
* loaded into the app process like any other provider, not into the bare test process. It is kept
* in Java anyway, next to its sibling, so the two read alike.
*/
public final class FixtureContentProvider extends ContentProvider {
/** Authority. Distinct from the documents provider's, and from anything the app declares. */
public static final String AUTHORITY = "org.libremediaconverter.test.content";
/** Builds a URI for one of this source set's committed assets, e.g. {@code sample_h264.mp4}. */
public static Uri uriFor(String assetName) {
return new Uri.Builder().scheme("content").authority(AUTHORITY).appendPath(assetName).build();
}
@Override
public boolean onCreate() {
return true;
}
@Override
public ParcelFileDescriptor openFile(Uri uri, String mode) throws FileNotFoundException {
if (!"r".equals(mode)) {
throw new FileNotFoundException("this provider is read-only: " + mode);
}
return ParcelFileDescriptor.open(unpack(assetOf(uri)), ParcelFileDescriptor.MODE_READ_ONLY);
}
/**
* Enough of {@link OpenableColumns} for {@code InputQuery.describe} to name and size the input.
*
* <p>Without these the app reaches the "Size unknown" screen, which is a different test.
*/
@Override
public Cursor query(Uri uri, String[] projection, String selection, String[] args, String sort) {
String asset = assetOf(uri);
File file;
try {
file = unpack(asset);
} catch (FileNotFoundException e) {
return null;
}
MatrixCursor cursor = new MatrixCursor(
new String[] {OpenableColumns.DISPLAY_NAME, OpenableColumns.SIZE});
cursor.newRow().add(OpenableColumns.DISPLAY_NAME, asset).add(OpenableColumns.SIZE, file.length());
return cursor;
}
@Override
public String getType(Uri uri) {
return assetOf(uri).endsWith(".m4a") ? "audio/mp4" : "video/mp4";
}
@Override
public Uri insert(Uri uri, ContentValues values) {
throw new UnsupportedOperationException("read-only fixture provider");
}
@Override
public int delete(Uri uri, String selection, String[] args) {
throw new UnsupportedOperationException("read-only fixture provider");
}
@Override
public int update(Uri uri, ContentValues values, String selection, String[] args) {
throw new UnsupportedOperationException("read-only fixture provider");
}
private static String assetOf(Uri uri) {
String asset = uri.getLastPathSegment();
return asset == null ? "" : asset;
}
/**
* The asset on disk, unpacked the first time anything asks.
*
* <p>Reported as {@link FileNotFoundException} rather than swallowed: a provider answering with
* a zero-byte file would fail the conversion for a reason nothing states.
*/
private File unpack(String asset) throws FileNotFoundException {
File file = new File(getContext().getCacheDir(), "provided_" + asset);
if (file.length() > 0L) {
return file;
}
try (InputStream source = getContext().getAssets().open(asset);
OutputStream sink = new FileOutputStream(file)) {
byte[] buffer = new byte[8192];
int read;
while ((read = source.read(buffer)) != -1) {
sink.write(buffer, 0, read);
}
} catch (IOException e) {
throw new FileNotFoundException("could not unpack " + asset + ": " + e);
}
return file;
}
}
@@ -14,8 +14,6 @@ import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.util.ArrayList;
import java.util.List;
/**
* One file, offered to the system file picker, so that picking one can be tested at all.
@@ -106,18 +104,6 @@ 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}. */
private static final List<String> DELETED = new ArrayList<>();
/** 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";
@@ -161,7 +147,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 | Root.FLAG_SUPPORTS_CREATE)
.add(Root.COLUMN_FLAGS, Root.FLAG_LOCAL_ONLY)
.add(Root.COLUMN_ICON, android.R.drawable.ic_menu_gallery);
return cursor;
}
@@ -173,8 +159,6 @@ 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);
}
@@ -194,86 +178,10 @@ public final class FixtureDocumentsProvider extends DocumentsProvider {
@Override
public ParcelFileDescriptor openDocument(String documentId, String mode, CancellationSignal signal)
throws FileNotFoundException {
if (FIXTURE_DOCUMENT_ID.equals(documentId)) {
return ParcelFileDescriptor.open(fixtureFile(), ParcelFileDescriptor.MODE_READ_ONLY);
}
if (documentId == null || !documentId.startsWith(DESTINATION_PREFIX)) {
if (!FIXTURE_DOCUMENT_ID.equals(documentId)) {
throw new FileNotFoundException("no such document: " + documentId);
}
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);
}
synchronized (DELETED) {
DELETED.add(documentId);
}
destinationFile(documentId).delete();
}
/**
* Document ids {@link #deleteDocument} was called with, newest last.
*
* <p><b>Nothing reads this yet, and that is recorded rather than hidden (#250).</b> It was
* added with #226 to assert {@code OutputPublisher.deletePartialOutput} — D4's cleanup — against
* a real {@code DocumentsProvider}. #226 only reached the <i>success</i> path, so the
* {@code catch} that calls it is still asserted only against {@code FakeSafProvider} under
* Robolectric. It is kept because the forcing condition is one {@code openDestination} override
* away and #250 says exactly what to add; if that ticket is closed any other way, delete this
* and {@link #DELETED} with it rather than leaving an accessor implying coverage.
*/
public static List<String> deletedDocumentIds() {
synchronized (DELETED) {
return new ArrayList<>(DELETED);
}
}
/** Forgets recorded deletes and removes created destinations. The process outlives one class. */
public static void reset(File filesDir) {
synchronized (DELETED) {
DELETED.clear();
}
File dir = new File(filesDir, "destinations");
File[] children = dir.listFiles();
if (children != null) {
for (File child : children) {
child.delete();
}
}
return ParcelFileDescriptor.open(fixtureFile(), ParcelFileDescriptor.MODE_READ_ONLY);
}
private void addDirectoryRow(MatrixCursor cursor) {
@@ -281,32 +189,10 @@ 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, Document.FLAG_DIR_SUPPORTS_CREATE)
.add(Document.COLUMN_FLAGS, 0)
.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,24 +1,15 @@
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
import androidx.test.runner.lifecycle.ActivityLifecycleCallback
import androidx.test.runner.lifecycle.ActivityLifecycleMonitorRegistry
import androidx.test.runner.lifecycle.Stage
import androidx.test.uiautomator.By
import androidx.test.uiautomator.BySelector
import androidx.test.uiautomator.Configurator
@@ -26,22 +17,13 @@ import androidx.test.uiautomator.StaleObjectException
import androidx.test.uiautomator.UiDevice
import androidx.test.uiautomator.Until
import org.junit.After
import org.junit.Assert.assertArrayEquals
import org.junit.Assert.assertEquals
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.util.concurrent.atomic.AtomicInteger
import java.util.regex.Pattern
/**
* Choosing a file, through the real system picker, and still having it after a rotation.
@@ -221,8 +203,6 @@ import java.util.regex.Pattern
* driven there at all. That is why this gap survived as long as it did.
* `tools/local-emulator/run-e2e.sh` runs API 33-36 on the development host, and both tests pass
* there: **59 / 0 / 0 / 2 at API 33 and again at API 36**, whole suite, 2026-08-24.
* (Since #223 the skip column reads 3 on an emulator — `HardwareFallbackTest` now announces
* that it cannot run without a hardware HEVC encoder rather than passing vacuously.)
*
* ### Why only the rotation test carries [FailsOnEmulatorApi37]
*
@@ -255,72 +235,12 @@ import java.util.regex.Pattern
* 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)
}
companion object {
var savedBytes: ByteArray = ByteArray(0)
var seenDestination: Uri? = null
var seenIsDocumentUri: Boolean? = null
var seenSizeBefore: Long? = null
fun reset() {
savedBytes = ByteArray(0)
seenDestination = null
seenIsDocumentUri = null
seenSizeBefore = null
}
}
}
@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())
@@ -331,21 +251,6 @@ class SafPickerRoundTripTest {
/** Set by the one test that rotates, read by [restoreOrientation]. See its KDoc. */
private var rotated = false
/** Counts [MainActivity] creations from the moment [watchForRecreation] is called. */
private val recreations = AtomicInteger()
/**
* Counts a rotation's recreation without asking the Activity anything.
*
* Deliberately not `composeRule.activity`, which resolves through `scenario.onActivity` and so
* blocks on the main thread. Polling *that* across a recreation is a plausible reading of the
* 20-minute wedges in #122, which would make the obvious barrier the bug it is meant to fix.
* The runner's lifecycle monitor is a callback: reading the counter touches no looper.
*/
private val recreationWatcher = ActivityLifecycleCallback { activity, stage ->
if (activity is MainActivity && stage == Stage.CREATED) recreations.incrementAndGet()
}
/**
* Leave the device the way it was found — and only if this test moved it.
*
@@ -365,44 +270,13 @@ class SafPickerRoundTripTest {
*/
@After
fun restoreOrientation() {
// The suite runs without Android Test Orchestrator, so a swapped seam outlives the class.
ConversionDependencies.reset()
ActivityLifecycleMonitorRegistry.getInstance().removeLifecycleCallback(recreationWatcher)
if (!rotated) return
device.setOrientationNatural()
device.unfreezeRotation()
device.waitForIdle()
}
/**
* **Marked for API 37 because of what it does to the image, not because it fails there.**
*
* This is the one place the marker's KDoc phrase "cannot pass on this image" does not fit, and
* the distinction is worth keeping rather than smoothing over. Across the four gating API 37
* runs whose logcats were read on 2026-09-05 — 34006456986, 34001744574, 34001377499 and the
* green 34002313300 — the leg carries exactly two `hasReadColorBufferDma` aborts before the
* suite starts (both `surfaceflinger`, during boot and the SystemUI disable) and then exactly
* **one** during it. Every time, that one is `system_server` on the `TaskSnapshotPer` thread,
* and every time it lands inside this test's window. No other test in the gating set reaches
* the mapper at all.
*
* So this test kills the framework on that image whether it passes or not, and whether the leg
* goes red is luck: 34001377499 passed it and lost the leg anyway (`failed: 0`, teardown
* broken), 34002313300 passed it 0.6 s after the abort and went green. That is #108, and it is
* why the leg was failing on unrelated PRs.
*
* `docs/api-37-emulator-crash.md` measured this test on 2026-08-24, recorded "passes, 4 aborts
* in the window", and concluded that a rotation reaches the mapper where starting DocumentsUI
* does not. The aborts were seen; what was not drawn out is that they are this test's own and
* are not intermittent.
*
* The marker is what routes it off the gating leg and into the advisory job beside its
* rotation sibling. **It is not a statement about the picker**: the same test passes on API
* 33–36 on the same runner and on the Pixel 10 Pro XL, which is where API 37's answer comes
* from.
*/
@Test
@FailsOnEmulatorApi37
fun pickingAFileThroughTheSystemPickerFillsInTheFileCard() {
pickTheFixture()
@@ -429,11 +303,9 @@ class SafPickerRoundTripTest {
// The identity hash rather than the Activity itself, so nothing here keeps a destroyed
// Activity reachable across the recreation it is being used to detect.
val before = System.identityHashCode(composeRule.activity)
watchForRecreation()
device.setOrientationLandscape()
rotated = true
awaitRecreation()
composeRule.waitForIdle()
// Two guards before the assertion that matters, because both of the ways this test could
@@ -482,192 +354,6 @@ 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())
}
/**
* 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() {
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) {
awaitNode(TestTags.Converter.CONVERT_ANOTHER, 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 ->
@@ -894,9 +580,6 @@ class SafPickerRoundTripTest {
* It is also why this counts backs rather than pressing a fixed number of them. One back is
* enough from Recent and two are needed from inside the root, but a third from Recent would
* finish `MainActivity` and take the rest of the test with it.
*
* **[forceStopThePicker] is the escalation after the presses, and it exists because a back
* press is not always deliverable.** See its own KDoc for the measurement.
*/
private fun dismissThePicker() {
repeat(BACK_PRESSES) {
@@ -911,50 +594,15 @@ class SafPickerRoundTripTest {
// The check after the last press, and not a spare one: `repeat` presses on its final
// iteration too, so without this a dismissal that worked on the last press would still be
// reported as a failure to close.
if (awaitAppFocus()) return
forceStopThePicker()
if (!awaitAppFocus()) {
throw AssertionError(
"the system picker would not close: after $BACK_PRESSES back presses and a " +
"force-stop of $DOCUMENTS_UI_PACKAGE the app still does not have the window " +
"focus, and ${device.currentPackageName} is in front. What could be seen: " +
describeWindows(),
"the system picker would not close: after $BACK_PRESSES back presses the app " +
"still does not have the window focus, and ${device.currentPackageName} is " +
"in front. What could be seen: " + describeWindows(),
)
}
}
/**
* Kills the picker's process, for when no back press can reach it.
*
* **The failure this exists for cannot be answered with input, and that is the whole point.**
* Measured on the gating API 37 legs of runs 34006456986 and 34001744574, which fail this way
* and whose logcats say the same thing in the same order. `UiObject2.click()` on the fixture's
* root is injected at the node's centre and the framework discards it —
* `InputDispatcher: No new touched window at (539.0, 525.0) in display 0` — because
* `PickActivity` has published accessibility nodes but has no touchable window there yet.
* `click()` cannot see that and returns normally, so the walk goes on to wait out
* [PICKER_TIMEOUT_MS] for a fixture that was never navigated to. By the time this function's
* caller starts pressing back, WindowManager is still saying
* `no window has focus but ...PickActivity may eventually add a window when it finishes
* starting up` — and goes on saying it for another 63 s. Every one of the four presses is
* dropped, and DocumentsUI ANRs on `Input dispatching timed out`.
*
* So the picker is in front, unreachable by key or by touch, and [pickTheFixture]'s whole
* point — that a second `PickActivity` rebuilds every window and list in it — is unreachable
* with it. `am force-stop` goes around input entirely: `UiAutomation` runs shell commands as
* uid 2000, which holds `FORCE_STOP_PACKAGES`, so the picker's process is killed, its
* activity leaves the task it was launched into, and `MainActivity` — the activity below it in
* that same task — is resumed with the focus.
*
* **Only on the failure path**, after every back press has been spent, so a picker that closes
* the ordinary way never reaches this and is not altered by it. If the framework itself is
* gone, this cannot help either, and the caller still reports what it could see.
*/
private fun forceStopThePicker() {
device.executeShellCommand("am force-stop $DOCUMENTS_UI_PACKAGE")
device.waitForIdle()
}
/** True once [MainActivity] has the window focus, false if it does not take it in time. */
private fun awaitAppFocus(): Boolean = try {
composeRule.waitUntil("the app has the window focus back", FOCUS_TIMEOUT_MS) {
@@ -1027,38 +675,8 @@ class SafPickerRoundTripTest {
* `Condition still not satisfied after 30000 ms` — which names neither the node nor the test.
* With the description it says which affordance never arrived, which is the whole finding.
*/
/** Starts counting [MainActivity] creations, so [awaitRecreation] can wait for the next one. */
private fun watchForRecreation() {
recreations.set(0)
ActivityLifecycleMonitorRegistry.getInstance().addLifecycleCallback(recreationWatcher)
}
/**
* Waits for the rotation to actually rebuild [MainActivity], which `waitForIdle` does not.
*
* **This is #122.** `waitForIdle()` waits for the compose hierarchy to settle. Immediately
* after a rotation the window manager has accepted but not yet delivered as a configuration
* change, the *old* Activity's composition is already idle — so it returns, `composeRule
* .activity` still resolves to the old instance, and the guard below reads an unchanged
* identity hash. That is the clean `AssertionError` seen on the API 33 gating leg of #217, and
* the wedges on #122 are the same race taken the other way: land while the composition is
* being torn down and there is nothing coherent for `waitForIdle` to settle on.
*
* A bounded wait is worth having even if that second half is wrong. It turns a 20-minute
* `WEDGE_TIMEOUT` — which costs the leg and names no test — into a fast failure that says which
* test and what it was waiting for.
*/
private fun awaitRecreation() {
composeRule.waitUntil(
"the rotation did not recreate MainActivity within $RECREATION_TIMEOUT_MS ms",
RECREATION_TIMEOUT_MS,
) {
recreations.get() > 0
}
}
private fun awaitNode(tag: String, timeoutMs: Long = APP_TIMEOUT_MS) {
composeRule.waitUntil("a node tagged $tag exists", timeoutMs) {
private fun awaitNode(tag: String) {
composeRule.waitUntil("a node tagged $tag exists", APP_TIMEOUT_MS) {
composeRule.onAllNodesWithTag(tag).fetchSemanticsNodes().isNotEmpty()
}
}
@@ -1073,30 +691,6 @@ 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
/**
* Only bounds a hang, and it is an order of magnitude clear of the real cost: the whole
* test — pick, convert, save — takes **11.8 s** on the API 34 CI leg (run 34043502322).
* Deliberately generous because the engine is not fixed: the default `MP4_H265` at `FAST`
* lands on FFmpeg on an emulator and on Media3 on real hardware, which is faster rather
* than slower — see [convertToTheDefaultFormat].
*/
const val CONVERSION_TIMEOUT_MS = 120_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.
*
@@ -1109,15 +703,6 @@ class SafPickerRoundTripTest {
*/
const val REOPENED_TIMEOUT_MS = 10_000L
/**
* How long a rotation is given to destroy and rebuild the Activity.
*
* Generous against the API 33 and 34 emulators #122 was measured on, where the rotation is
* slow enough for the gap this bound exists to cover to be observable at all — and still
* two orders of magnitude inside the 1200 s `WEDGE_TIMEOUT` it replaces.
*/
const val RECREATION_TIMEOUT_MS = 15_000L
/**
* How long the app is given to take the window focus back after a back press.
*
@@ -1,129 +0,0 @@
package org.libremediaconverter.work
import android.net.Uri
import androidx.media3.common.util.UnstableApi
import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.test.platform.app.InstrumentationRegistry
import androidx.work.OneTimeWorkRequestBuilder
import androidx.work.WorkInfo
import androidx.work.WorkManager
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.withTimeout
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertNotNull
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.model.OutputFormat
import org.libremediaconverter.model.QualityTier
import java.io.File
import java.util.concurrent.TimeUnit
/**
* The Cancel button in the notification shade actually cancels the job.
*
* `ConversionNotifications.build` attaches one action, wired to
* `WorkManager.createCancelPendingIntent(id)`. Before this test `createCancelPendingIntent` had
* **no references anywhere outside its own declaration** — no JVM test, no instrumented test
* (#227).
*
* That matters more than an ordinary uncovered line. A conversion runs in a foreground service and
* the user is invited to leave the app; once they do, this action is the only way to stop it. If
* the `PendingIntent` carries the wrong id, the button does nothing, the notification stays, and
* the job runs to completion — with no error, no log, and no screen to look at.
*
* ## Why this fires the intent rather than reading the shade
*
* The obvious version asks `NotificationManager.getActiveNotifications()` for id 1001 and taps what
* it finds. That was rejected: the instrumented suite grants no runtime permissions, so
* `POST_NOTIFICATIONS` is denied throughout, and whether a suppressed foreground-service
* notification is returned there is a platform detail that varies — the test would be asserting
* something about notification *visibility* rather than about cancellation.
*
* The `PendingIntent` is the subject; where it is read from is incidental. Building the
* notification for a real, live work id and firing its action exercises exactly the thing that can
* be wrong — a real `PendingIntent` dispatch reaching real `WorkManager` — and does it the same way
* on every API level.
*
* ## Why the job is delayed rather than running
*
* A conversion of the committed 3 s fixture finishes in well under a second on an emulator
* (`HardwareFallbackTest` completed one in 448 ms), so racing a cancel against a running job would
* be flaky in the direction that fails. An initial delay keeps the job reliably `ENQUEUED`, which
* is a state `cancelWorkById` acts on identically — what is under test is whether firing the action
* reaches WorkManager with the right id, not which state it interrupts.
*
* *Mutation:* build the `PendingIntent` from `UUID.randomUUID()` instead of the request's id. The
* notification looks identical and the job is never cancelled.
*/
@UnstableApi
@RunWith(AndroidJUnit4::class)
class NotificationCancelActionTest {
private val context = InstrumentationRegistry.getInstrumentation().targetContext
private val workManager = WorkManager.getInstance(context)
private lateinit var input: File
@Before
fun setUp() {
input = File(context.cacheDir, "cancel_action_sample.mp4")
InstrumentationRegistry.getInstrumentation().context.assets
.open("sample_h264.mp4")
.use { asset -> input.outputStream().use { asset.copyTo(it) } }
}
@After
fun tearDown() {
input.delete()
File(context.cacheDir, "conversions").listFiles()?.forEach { it.delete() }
}
@Test
fun theNotificationsCancelActionCancelsThatJob(): Unit = runBlocking {
val request = ConversionWorker.request(
inputUri = Uri.fromFile(input),
displayName = input.name,
sizeBytes = input.length(),
spec = OutputFormat.MP4_H264.spec,
quality = QualityTier.FAST,
).let { base ->
// Rebuild with a delay so the job stays ENQUEUED for the whole test. See the KDoc.
OneTimeWorkRequestBuilder<ConversionWorker>()
.setInputData(base.workSpec.input)
.setInitialDelay(1, TimeUnit.HOURS)
.build()
}
workManager.enqueue(request).result.get()
// The job is queued and waiting, which is the state the cancel has to interrupt.
assertEquals(
WorkInfo.State.ENQUEUED,
withTimeout(TIMEOUT_MS) {
workManager.getWorkInfoByIdFlow(request.id).first { it != null }
}?.state,
)
val notification = ConversionNotifications(context)
.build(request.id, title = input.name, percent = 0, indeterminate = true)
val action = notification.actions?.firstOrNull()
assertNotNull("the progress notification carries no action to cancel with", action)
// The whole point: fire it the way the shade would, and see the job stop.
action!!.actionIntent.send()
val terminal = withTimeout(TIMEOUT_MS) {
workManager.getWorkInfoByIdFlow(request.id).first { it != null && it.state.isFinished }
}
assertEquals(
"firing the notification's Cancel action must cancel the job it was built for",
WorkInfo.State.CANCELLED,
terminal?.state,
)
}
private companion object {
const val TIMEOUT_MS = 30_000L
}
}
@@ -3,7 +3,6 @@ package org.libremediaconverter
import android.app.Application
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.Job
import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.launch
import org.libremediaconverter.convert.OutputPublisher
@@ -18,36 +17,14 @@ import org.libremediaconverter.convert.OutputPublisher
* ever becomes a `Converted` state, or a `reset()`'s delete is cancelled along with the
* Activity. Process start is the one moment those leftovers are reliably observable.
*/
open class LibreMediaConverterApp : Application() {
class LibreMediaConverterApp : Application() {
/**
* Deliberately process-lifetime and never cancelled: the work it carries is a single
* short task that should outlive nothing in particular and be interrupted by nothing.
* A `SupervisorJob` so a failure here could never take a sibling down with it.
*
* **`protected open` for #159.** Robolectric builds an `Application` for every test that asks
* for one, so on the JVM this is not one background sweep but one *per test* — all of them on
* `Dispatchers.IO`, all touching the same `cacheDir`, none of them joined by anything. That is
* a race against any test asserting about a file under `conversions/`, and it grew with the
* suite: wave 4 added ten Robolectric classes and took it from CI-only to roughly one local run
* in six. The JVM suite substitutes a scope that runs the sweep inline — see
* `app/src/test/resources/robolectric.properties` and `TestLibreMediaConverterApp`.
*
* A constructor parameter would be the ordinary way to inject this and is not available: the
* framework builds this class, so the seam has to be a member.
*/
protected open val sweepScope: CoroutineScope = CoroutineScope(SupervisorJob() + Dispatchers.IO)
/**
* The sweep [onCreate] last started, so a caller that needs it finished can wait for it.
*
* Nothing in production reads this — process start does not wait for its own housekeeping. It
* exists because the alternative for a test is a timed poll, and a poll cannot tell "the sweep
* has not run yet" from "the sweep ran and did nothing".
*/
@Volatile
var startupSweep: Job? = null
private set
private val appScope = CoroutineScope(SupervisorJob() + Dispatchers.IO)
override fun onCreate() {
super.onCreate()
@@ -76,6 +53,6 @@ open class LibreMediaConverterApp : Application() {
//
// sweepStaging() also re-reads each timestamp immediately before deleting, which
// closes the window between listing the directory and acting on the listing.
startupSweep = sweepScope.launch { OutputPublisher(this@LibreMediaConverterApp).sweepStaging() }
appScope.launch { OutputPublisher(this@LibreMediaConverterApp).sweepStaging() }
}
}
@@ -21,11 +21,6 @@ import org.libremediaconverter.model.VideoCodec
* words, "cannot be tested for correctness". It is a hint, not a guarantee, which is
* why the router treats a failed hardware export as a signal to fall back rather
* than trusting this up front.
* - **An enumeration that fails answers no to everything**, which sends every job to
* FFmpeg. Empty sets are not a permissive default: `canEncode` looks a MIME type up in
* [hardwareEncodeMimes] and finds nothing there. That is the intended answer — FFmpeg
* can do whatever Media3 can, only slower — but it is the opposite of what this class
* said until #194, so it is written down rather than left to be re-derived.
*/
class AndroidDeviceCodecs private constructor(
private val hardwareEncodeMimes: Set<String>,
@@ -51,62 +46,22 @@ class AndroidDeviceCodecs private constructor(
fun get(): AndroidDeviceCodecs = cached ?: synchronized(this) { cached ?: probe().also { cached = it } }
/**
* One entry of the platform's codec list, reduced to what the rules below read.
*
* The five booleans and the type list are the whole of what [capabilitiesFrom] needs, and
* none of them can be set on a `MediaCodecInfo` from a test: Robolectric ships
* `MediaCodecInfoBuilder`, but it has no `setIsAlias` and no `setCanonicalName`, which is
* exactly the objection #133 raised against reaching this code through
* `ShadowMediaCodecList`. That objection is about the shadow. It does not apply to a
* function that takes its own entry type, which is why this exists.
*/
internal data class CodecEntry(
val canonicalName: String,
val isAlias: Boolean,
val isEncoder: Boolean,
val isHardwareAccelerated: Boolean,
val isSoftwareOnly: Boolean,
val supportedTypes: List<String>,
)
/**
* The enumeration rules, over entries a caller chooses.
*
* [probe] is the only production caller and supplies the real codec list; a test supplies
* its own, which is the point — the two rules this class's KDoc calls out as easy to get
* wrong, the alias skip and the canonical-name dedup, are unreachable any other way.
*
* **`enumerate` returns a `Sequence`, deliberately.** The `runCatching` has to wrap the
* *iteration* rather than a list built before it, because a `MediaCodecInfo` whose
* properties throw does so partway through — and when that happens the codecs already read
* are kept. Taking a `List` here would move that throw outside the loop and silently turn a
* partial answer into an empty one. That behaviour predates this seam; a `List` parameter
* would have changed it as a side effect of a refactor.
*
* **An enumeration that fails answers restrictively, and that is deliberate.** The sets
* come back empty, and `"video/avc" in emptySet()` is `false`, so [canEncode] and
* [canDecode] both answer no and every job routes to FFmpeg. FFmpeg can do everything
* Media3 can, only slower, so refusing the hardware path is the safe reading of "we could
* not find out what this device supports". This used to log "assuming permissive", which
* described the opposite of what the code does.
*/
internal fun capabilitiesFrom(enumerate: () -> Sequence<CodecEntry>): AndroidDeviceCodecs {
private fun probe(): AndroidDeviceCodecs {
val encoders = mutableSetOf<String>()
val decoders = mutableSetOf<String>()
val seen = mutableSetOf<String>()
runCatching {
enumerate().forEach { entry ->
MediaCodecList(MediaCodecList.REGULAR_CODECS).codecInfos.forEach { info ->
// Aliases point at the same underlying codec; counting both would
// double-count capabilities.
if (entry.isAlias) return@forEach
if (!seen.add(entry.canonicalName)) return@forEach
if (info.isAlias) return@forEach
if (!seen.add(info.canonicalName)) return@forEach
entry.supportedTypes.forEach { mime ->
info.supportedTypes.forEach { mime ->
if (!mime.startsWith("video/")) return@forEach
if (entry.isEncoder) {
if (entry.isHardwareAccelerated && !entry.isSoftwareOnly) {
if (info.isEncoder) {
if (info.isHardwareAccelerated && !info.isSoftwareOnly) {
encoders += mime
}
} else {
@@ -114,32 +69,12 @@ class AndroidDeviceCodecs private constructor(
}
}
}
}.onFailure { Log.w(TAG, "Codec enumeration failed; routing everything to FFmpeg.", it) }
}.onFailure { Log.w(TAG, "Codec enumeration failed; assuming permissive.", it) }
Log.i(TAG, "Hardware video encoders: $encoders")
return AndroidDeviceCodecs(encoders, decoders)
}
/**
* The thin edge: the real codec list, mapped onto [CodecEntry] one at a time.
*
* Lazily, so a property that throws does it inside [capabilitiesFrom]'s `runCatching` and
* on the entry that caused it — see that function's note on why the parameter is a
* `Sequence`.
*/
private fun probe(): AndroidDeviceCodecs = capabilitiesFrom {
MediaCodecList(MediaCodecList.REGULAR_CODECS).codecInfos.asSequence().map { info ->
CodecEntry(
canonicalName = info.canonicalName,
isAlias = info.isAlias,
isEncoder = info.isEncoder,
isHardwareAccelerated = info.isHardwareAccelerated,
isSoftwareOnly = info.isSoftwareOnly,
supportedTypes = info.supportedTypes.toList(),
)
}
}
/**
* `internal` rather than `private` so the cross-check test can ask what a [VideoCodec]
* means here and compare it with what [NAME_TO_MIME] says the same codec's names mean.
@@ -673,23 +673,13 @@ class ConversionViewModel @JvmOverloads constructor(
else -> null
}
/**
* The input `convert()` may act on, which is only ever the one on a `Ready` screen.
*
* This used to answer for `Converting`, `Waiting` and `Converted` as well. Those arms were not
* reachable by tapping Convert -- the button renders only in the `Ready` branch -- but they
* were reachable through the POST_NOTIFICATIONS **result**, which `ConverterScreen.kt:91` wires
* to `convert()` rather than to the button. Reaching one of them enqueued a *second* job over a
* live one: `activeWorkId` was overwritten, and the first job kept running with its foreground
* notification orphaned and nothing left holding its id to cancel it.
*
* Narrowed under #202 rather than tested as it stood, because a test written against the old
* shape would have frozen the double-enqueue as intended behaviour -- the F1/F5 failure mode.
*
* `JoinViewModel.join()` has been `(_state.value as? JoinState.Ready)?.inputs ?: return` all
* along. The two screens are the same shape and only one of them was over-general.
*/
private fun currentInput(): InputFile? = (_state.value as? ConversionState.Ready)?.input
private fun currentInput(): InputFile? = when (val s = _state.value) {
is ConversionState.Ready -> s.input
is ConversionState.Converting -> s.input
is ConversionState.Waiting -> s.input
is ConversionState.Converted -> s.input
else -> null
}
private companion object {
/**
@@ -221,39 +221,12 @@ object MediaProbe {
null
}
/**
* The thin edge: spawn FFprobe, hand what it said to [ffprobeInfoFrom].
*
* Everything device-bound is on this line and the null check under it. What FFprobe *said* is a
* `MediaInformation`, which is an ordinary object over a `JSONObject` — so the reading of it is
* a decision a test can choose the inputs for, and it lives below rather than here.
*/
private fun readMediaInformation(path: String): FFprobeInfo? =
FFprobeKit.getMediaInformation(path).getMediaInformation()?.let(::ffprobeInfoFrom)
private fun readMediaInformation(path: String): FFprobeInfo? {
// ffmpeg-kit-next is compiled from Kotlin with private backing fields, so these have to go
// through the Java getters rather than property syntax.
val info: MediaInformation = FFprobeKit.getMediaInformation(path).getMediaInformation()
?: return null
/**
* What FFprobe's answer means, as a function of the answer alone.
*
* `internal` for the same reason [Extracted] and [FFprobeInfo] are: a test cannot name it
* otherwise, and the JVM test source set is a friend of `main`.
*
* **JVM-safe, verified rather than assumed.** `javap` over the committed AAR's runtime jar:
* `MediaInformation(JSONObject, List<StreamInformation>, List<Chapter>)` and
* `StreamInformation(JSONObject)` are plain public constructors, and neither class's `<clinit>`
* touches the native library — so a test builds its own without `libffmpegkit` being present.
* That is the whole reason this split is worth making: `readMediaInformation` was 114 missed
* instructions and 24 missed branches, of which exactly one line needed a device.
*
* The subtle part is the **second argument to [containerFrom]**. `matroska,webm` is reported
* for both MKV and WebM — they share a demuxer — so the video codec is the only thing that
* separates them, and dropping it silently turns every VP9 WebM into an MKV. `containerFrom`
* has thirty-three covered branches of its own and none of them can notice that, because the
* mistake is at the call rather than in the callee.
*
* ffmpeg-kit-next is compiled from Kotlin with private backing fields, so these go through the
* Java getters rather than property syntax.
*/
internal fun ffprobeInfoFrom(info: MediaInformation): FFprobeInfo {
val streams = info.getStreams().orEmpty()
val video = streams.firstOrNull { it.getType() == "video" }
val audio = streams.firstOrNull { it.getType() == "audio" }
@@ -5,6 +5,7 @@ import android.net.Uri
import android.util.Log
import com.arthenica.ffmpegkit.FFmpegKit
import com.arthenica.ffmpegkit.FFmpegKitConfig
import com.arthenica.ffmpegkit.ReturnCode
import kotlinx.coroutines.suspendCancellableCoroutine
import org.libremediaconverter.convert.ConcatJoiner
import org.libremediaconverter.convert.MediaProbe
@@ -65,16 +66,16 @@ class ConcatEngine(private val context: Context) : ConcatJoiner {
private suspend fun execute(args: List<String>) = suspendCancellableCoroutine { cont ->
Log.i(TAG, "ffmpeg ${args.joinToString(" ")}")
val session = FFmpegKit.executeWithArgumentsAsync(args.toTypedArray()) { completed ->
val outcome = sessionOutcome(
rc = completed.getReturnCode(),
prefix = "Joining",
failStackTrace = { completed.getFailStackTrace() },
logTail = { completed.getAllLogsAsString(LOG_TAIL_LIMIT) },
)
when (outcome) {
SessionOutcome.Success -> cont.resume(Unit)
SessionOutcome.Cancelled -> cont.cancel()
is SessionOutcome.Failed -> cont.resumeWithException(FFmpegEngine.FFmpegException(outcome.message))
val rc = completed.getReturnCode()
when {
ReturnCode.isSuccess(rc) -> cont.resume(Unit)
ReturnCode.isCancel(rc) -> cont.cancel()
else -> cont.resumeWithException(
FFmpegEngine.FFmpegException(
"Joining failed (${rc?.value}): " +
completed.getAllLogsAsString(LOG_TAIL_LIMIT).orEmpty(),
),
)
}
}
cont.invokeOnCancellation { FFmpegKit.cancel(session.getSessionId()) }
@@ -35,21 +35,6 @@ object FFmpegConcatCommand {
add("concat")
add("-safe")
add("0")
// And -protocol_whitelist permits the *scheme* those paths carry, which is a
// separate gate (#238). Every input the user actually picks is a content:// URI --
// JoinScreen uses OpenMultipleDocuments -- so ConcatEngine maps it through
// FFmpegKitConfig.getSafParameterForRead and writes an `ffkitsaf:` path into the
// list file. The concat demuxer applies its own whitelist, defaulting to
// "file,crypto,data", and refused every one of them:
//
// [ffkitsaf @ ...] Protocol 'ffkitsaf' not on whitelist 'file,crypto,data'!
//
// This only widens that default. It is on the stream-copy branch alone because it
// is the only one that feeds the demuxer a list file -- REENCODE passes each input
// with its own -i, where the whitelist does not apply, which is why joining over SAF
// worked for mismatched clips and failed for matching ones.
add("-protocol_whitelist")
add(PROTOCOL_WHITELIST)
add("-i")
add(listFile.absolutePath)
add("-c")
@@ -99,12 +84,4 @@ object FFmpegConcatCommand {
add(output.absolutePath)
}
}
/**
* The concat demuxer's protocol whitelist: FFmpeg's own default, plus ffmpeg-kit's SAF scheme.
*
* Spelled out rather than appended to an unknown default, because the default is FFmpeg's and
* could change under us; naming all four keeps the command self-describing. See #238.
*/
private const val PROTOCOL_WHITELIST = "file,crypto,data,ffkitsaf"
}
@@ -4,6 +4,7 @@ import android.util.Log
import com.arthenica.ffmpegkit.FFmpegKit
import com.arthenica.ffmpegkit.FFmpegKitConfig
import com.arthenica.ffmpegkit.Level
import com.arthenica.ffmpegkit.ReturnCode
import kotlinx.coroutines.suspendCancellableCoroutine
import org.libremediaconverter.convert.SoftwareTranscoder
import org.libremediaconverter.model.ConversionRequest
@@ -50,16 +51,19 @@ class FFmpegEngine : SoftwareTranscoder {
val session = FFmpegKit.executeWithArgumentsAsync(
args.toTypedArray(),
{ completed ->
val outcome = sessionOutcome(
rc = completed.getReturnCode(),
prefix = "FFmpeg",
failStackTrace = { completed.getFailStackTrace() },
logTail = { completed.getAllLogsAsString(LOG_TAIL_LIMIT) },
)
when (outcome) {
SessionOutcome.Success -> cont.resume(Unit)
SessionOutcome.Cancelled -> cont.cancel()
is SessionOutcome.Failed -> cont.resumeWithException(FFmpegException(outcome.message))
val rc = completed.getReturnCode()
when {
ReturnCode.isSuccess(rc) -> cont.resume(Unit)
ReturnCode.isCancel(rc) ->
cont.cancel()
else -> cont.resumeWithException(
FFmpegException(
"FFmpeg failed (${rc?.value}): " +
completed.getFailStackTrace().orEmpty().ifBlank {
completed.getAllLogsAsString(LOG_TAIL_LIMIT).orEmpty()
},
),
)
}
},
{ log -> Log.d(TAG, log.message.trimEnd()) },
@@ -1,54 +0,0 @@
package org.libremediaconverter.ffmpeg
import com.arthenica.ffmpegkit.ReturnCode
/**
* What a finished FFmpegKit session means, as a function of its return code.
*
* Both engines had their own copy of this `when`, twelve lines apart in two files, and the copies
* had drifted: [FFmpegEngine] preferred the fail stack trace and fell back to the log tail, while
* [ConcatEngine] only ever read the log tail. Neither was tested — both live inside a callback
* handed to `FFmpegKit`, which does not run on the JVM — so the divergence was invisible.
*
* #203 decided to unify on the stack trace, so a join failure now carries the diagnostics a
* conversion failure always did. The *prefix* stays per-engine: unifying the strategy must not
* unify the sentence, since "FFmpeg failed" and "Joining failed" describe different jobs.
*/
internal sealed interface SessionOutcome {
/** rc 0. The suspension resumes normally. */
data object Success : SessionOutcome
/** rc 255. The suspension is cancelled rather than failed — the user asked for this. */
data object Cancelled : SessionOutcome
/** Anything else, with the sentence the user is shown. */
data class Failed(val message: String) : SessionOutcome
}
/**
* Maps a return code onto the outcome, and builds the failure sentence when there is one.
*
* **The two message parts arrive as lambdas, deliberately.** `getAllLogsAsString` and
* `getFailStackTrace` are calls onto a native session, and only the failure arm needs either. Taking
* them by value would put both on the happy path of every successful conversion, which is a cost the
* shape this replaced did not have — the old code read them inside the `else` branch. That is the
* same reason [org.libremediaconverter.codec.AndroidDeviceCodecs.capabilitiesFrom] takes a
* `Sequence`: a seam should not change what runs when.
*
* A null [rc] is a real input rather than a defensive one — `getReturnCode()` is nullable, and a
* session killed before it reported anything has none. It is neither success nor cancellation, so
* it fails, and the sentence says `null` where the number would be.
*/
internal fun sessionOutcome(
rc: ReturnCode?,
prefix: String,
failStackTrace: () -> String?,
logTail: () -> String?,
): SessionOutcome = when {
ReturnCode.isSuccess(rc) -> SessionOutcome.Success
ReturnCode.isCancel(rc) -> SessionOutcome.Cancelled
else -> SessionOutcome.Failed(
"$prefix failed (${rc?.value}): " + failStackTrace().orEmpty().ifBlank { logTail().orEmpty() },
)
}
@@ -1,8 +1,8 @@
package org.libremediaconverter
import kotlinx.coroutines.runBlocking
import org.junit.Assert.assertNotNull
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Assert.fail
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
@@ -10,6 +10,7 @@ import org.libremediaconverter.convert.StagingSweep
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment
import java.io.File
import java.util.concurrent.TimeUnit
/**
* That process start actually sweeps.
@@ -22,19 +23,8 @@ import java.io.File
* output ever became a `Converted` state, a `reset()` whose delete was cancelled with the Activity.
*
* `onCreate()` is called again rather than a second Application being built: it is what the
* framework calls at process start, and the scope it launches on is already there.
*
* **What this class stopped covering in #159, deliberately.** It used to open by asserting that
* `RuntimeEnvironment.getApplication()` is a [LibreMediaConverterApp] — that the manifest's
* `android:name` points here, so the sweep is code that actually runs. That assertion cannot exist
* on the JVM any more: `robolectric.properties` now names [TestLibreMediaConverterApp] for the
* whole suite, and an `application=` override replaces the manifest rather than being checked
* against it — `applicationInfo.className` reports the override too, measured. So the manifest is
* not merely unasserted here, it is unobservable from this source set, and a rewritten version of
* that test would have asserted the override against itself. **The manifest link is a device-only
* guarantee now**, and it was traded knowingly for the race that override fixes. The cast in
* [setUp] still fails if [TestLibreMediaConverterApp] stops extending the real class, which is a
* smaller claim than the one withdrawn.
* framework calls at process start, the scope it launches on is already there, and the first test
* below is what pins that the framework calls it on *this* class.
*/
@RunWith(RobolectricTestRunner::class)
class AppStartSweepTest {
@@ -44,35 +34,17 @@ class AppStartSweepTest {
@Before
fun setUp() {
// The cast is an assertion in itself: Robolectric builds the Application named in the
// merged manifest, so this fails if `android:name` ever stops pointing here -- in which
// case the sweep below would be perfectly correct code that never runs.
app = RuntimeEnvironment.getApplication() as LibreMediaConverterApp
stagingDir = File(app.cacheDir, "conversions").apply { mkdirs() }
stagingDir.listFiles()?.forEach { it.delete() }
}
/**
* The property the whole substitution exists for, asserted directly rather than waited on.
*
* #159 is not "the sweep is slow", it is "the sweep is still running while some later test
* reads the directory". [TestLibreMediaConverterApp] answers that by finishing the sweep before
* `onCreate()` returns, and this is the only place that claim is checked -- every other test in
* the suite benefits from it silently and would go back to racing without saying why.
*
* Deterministic in the direction that matters: `Dispatchers.Unconfined` runs a `launch` whose
* body never suspends to completion inline, so this cannot flake green-to-red. Putting the test
* app back on `Dispatchers.IO` makes it a race that the assertion loses essentially every time,
* which is what a six-run suite comparison could not show -- at the rate #159 was observed at,
* a clean six-run arm is a coin flip.
*/
@Test
fun `the sweep is finished before onCreate returns`() {
app.onCreate()
val sweep = app.startupSweep
assertNotNull("onCreate() started no sweep", sweep)
assertTrue(
"the JVM suite's sweep outlived onCreate(), so it is in flight during test bodies again",
sweep?.isCompleted == true,
)
fun `the application the manifest starts is the one that sweeps`() {
assertEquals(LibreMediaConverterApp::class.java, RuntimeEnvironment.getApplication().javaClass)
}
@Test
@@ -92,23 +64,35 @@ class AppStartSweepTest {
app.onCreate()
// Joined rather than polled. `onCreate` publishes the sweep it started, so this waits for
// that exact sweep -- where a timed poll could not tell "swept" from "not started yet", and
// answered the second case by failing after ten seconds.
val sweep = app.startupSweep
assertNotNull("onCreate() started no sweep to wait for", sweep)
runBlocking { sweep?.join() }
assertTrue("process start left ${abandoned.name} in staging; nothing swept it", !abandoned.exists())
awaitGone(abandoned)
// The other half, and the one that says the sweep is a sweep rather than a
// `clearStaging()`: the directory is shared by the convert tab, the join tab and
// ConcatEngine's list file, so deleting everything could take a file from a running job.
assertTrue("a file written moments ago belongs to a live job", live.exists())
}
/**
* Waits for [file] to be deleted.
*
* The sweep runs on `Dispatchers.IO`, deliberately: it lists a directory and stats every entry
* on the path that decides how long the launcher icon stays unresponsive. So there is nothing
* to join, and the wait is a bounded poll — long enough for a directory listing, short enough
* that a sweep which never happens fails rather than hangs.
*/
private fun awaitGone(file: File) {
val deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(AWAIT_TIMEOUT_SECONDS)
while (System.nanoTime() < deadline) {
if (!file.exists()) return
Thread.sleep(POLL_INTERVAL_MS)
}
fail("process start left ${file.name} in staging; nothing swept it")
}
private fun stagedFile(name: String): File = File(stagingDir, name).apply { writeBytes(ByteArray(4096)) }
private companion object {
const val ONE_MINUTE_MS = 60L * 1000
const val AWAIT_TIMEOUT_SECONDS = 10L
const val POLL_INTERVAL_MS = 5L
}
}
@@ -1,28 +0,0 @@
package org.libremediaconverter
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.SupervisorJob
/**
* The [LibreMediaConverterApp] the JVM suite runs, differing from it in exactly one thing: the
* startup sweep runs inline on the thread that builds the Application instead of on
* `Dispatchers.Unconfined`.
*
* **This is #159.** Robolectric builds an `Application` per test class that asks for one, and each
* one launches a sweep over the shared `<cacheDir>/conversions/`. Nothing joins them, so a test
* asserting about a staged file is racing however many sweeps the classes before it left in
* flight — `OutputPublisherStagingTest` being the one that lost, at roughly one local run in six
* once wave 4 added ten more Robolectric classes. Making the sweep finish before `onCreate()`
* returns removes the race for every test at once rather than asking each to opt in; 27 of the
* suite's 58 Robolectric classes touch that directory, so opting in was not a real option.
*
* `Dispatchers.Unconfined` is what makes it inline: `sweepStaging()` is a plain function, so an
* `Unconfined` `launch` runs it to completion before returning. The `SupervisorJob` is kept so this
* differs from production in the dispatcher alone — a sweep that throws is logged and swallowed
* here exactly as it is there, rather than taking Application construction down with it and failing
* every test in the class for an unrelated reason.
*/
class TestLibreMediaConverterApp : LibreMediaConverterApp() {
override val sweepScope: CoroutineScope = CoroutineScope(SupervisorJob() + Dispatchers.Unconfined)
}
@@ -1,201 +0,0 @@
package org.libremediaconverter.codec
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.model.VideoCodec
import org.robolectric.RobolectricTestRunner
/**
* The rules `AndroidDeviceCodecs.probe()` applies to the platform's codec list.
*
* ## Why this is not a third run of the #86/#133 spike
*
* #86 closed `probe()` as device-bound. #133 re-opened the question with
* `ShadowMediaCodecList` in hand and closed it again, for a reason that was right about what it
* was answering: `MediaCodecInfoBuilder` "has no `setIsAlias` and no `setCanonicalName`, so the
* alias skip and the canonical-name dedup — the two things the class's KDoc calls out as easy to
* get wrong — are not reachable through it."
*
* **That objection is about the shadow.** It does not apply to a function that takes its own entry
* type, which is what `capabilitiesFrom` now does. The half #133 named as unreachable is the half
* this file spends most of its cases on.
*
* ## What made the seam worth cutting, which is not coverage
*
* The `runCatching` fallback logged *"assuming permissive"* and returned empty sets — and empty
* sets are **restrictive**: `"video/avc" in emptySet()` is `false`, so `canEncode` and `canDecode`
* both answer no and every job routes to FFmpeg. The code was right and the message described the
* opposite of it. That is pinned below, so whichever reading a future change takes, it has to say
* so out loud.
*
* Robolectric only because `capabilitiesFrom` logs what it found; the rules themselves are pure.
*/
@RunWith(RobolectricTestRunner::class)
class CodecEnumerationTest {
/**
* The alias skip, in the one arrangement where it is observable — and finding that arrangement
* is the whole of this test.
*
* A first attempt listed the alias *after* the codec it aliases and passed with the skip
* deleted, because `canonicalName` is shared and the dedup below catches the second entry
* either way. The two rules overlap, so a fixture that does not separate them tests neither.
*
* What separates them is **order**. `MediaCodecInfo.getCanonicalName()` on an alias returns the
* underlying codec's name, so an alias arriving first claims that name in `seen` and has its
* own `supportedTypes` credited — and then the real codec is dropped by the dedup. Without the
* alias skip the device is described by whichever entry the platform happened to list first.
*
* That also says what the rule is worth. With a `Set` accumulator, an alias declaring the same
* types as its codec changes nothing whichever order they arrive in; the skip earns its place
* only when the two disagree, which is exactly when believing the wrong one matters.
*/
@Test
fun `an alias listed before the codec it aliases does not describe the device`() {
val codecs = capabilities(
entry("c2.qti.avc.encoder", encoder = true, types = listOf(HEVC), alias = true),
entry("c2.qti.avc.encoder", encoder = true, types = listOf(AVC)),
)
assertTrue("the real codec's types are the device's", codecs.canEncode(VideoCodec.H264))
assertFalse(
"an alias must not be credited with types the codec it aliases never claimed",
codecs.canEncode(VideoCodec.H265),
)
}
@Test
fun `two entries sharing a canonical name are read once`() {
val codecs = capabilities(
entry("c2.qti.avc.encoder", encoder = true, types = listOf(AVC)),
entry("c2.qti.avc.encoder", encoder = true, types = listOf(HEVC)),
)
assertEquals(setOf(AVC), codecs.hardwareEncoders())
}
/**
* Both halves of the hardware predicate, one arm at a time.
*
* A vendor may declare a codec hardware-accelerated *and* software-only; the class KDoc is
* explicit that the first flag "cannot be tested for correctness", so the second is what stops
* a mislabelled software encoder being treated as the fast path.
*/
@Test
fun `an encoder counts as hardware only when it is accelerated and not software-only`() {
assertEquals(
setOf(AVC),
capabilities(entry("hw", encoder = true, accelerated = true, types = listOf(AVC))).hardwareEncoders(),
)
assertEquals(
emptySet<String>(),
capabilities(entry("sw", encoder = true, accelerated = false, types = listOf(AVC))).hardwareEncoders(),
)
assertEquals(
"a codec claiming both must not be trusted as hardware",
emptySet<String>(),
capabilities(
entry("both", encoder = true, accelerated = true, softwareOnly = true, types = listOf(AVC)),
).hardwareEncoders(),
)
}
/**
* Decoders are collected regardless of the hardware flags, and that asymmetry is the design.
*
* `canDecode` asks whether the platform can read the input at all — a software decoder answers
* that as well as a hardware one. `canEncode` asks whether the *fast path* exists, which is a
* different question and why only encoders are filtered.
*/
@Test
fun `a software decoder still counts as something the platform can read`() {
val codecs = capabilities(
entry(
"c2.android.avc.decoder",
encoder = false,
accelerated = false,
softwareOnly = true,
types = listOf(AVC),
),
)
assertTrue(codecs.canDecode("h264"))
}
@Test
fun `audio types are ignored on both sides`() {
val codecs = capabilities(
entry("aac.encoder", encoder = true, accelerated = true, types = listOf("audio/mp4a-latm")),
entry("aac.decoder", encoder = false, types = listOf("audio/mp4a-latm")),
)
assertEquals(emptySet<String>(), codecs.hardwareEncoders())
// Not "the platform cannot decode AAC" -- `canDecode` is asked about *video* codec names,
// and an unknown name is answered permissively. The point is that nothing audio reached
// either set.
assertTrue("an unknown name stays permissive", codecs.canDecode("something-nobody-named"))
}
/**
* The failure fallback, pinned as the restrictive answer it actually is.
*
* #194 decided this rather than assuming it: the code stays, the message changes. If a later
* change wants the permissive reading its old log line described, this test is what makes that
* a decision instead of a drift.
*/
@Test
fun `an enumeration that fails sends every job to FFmpeg`() {
val codecs = AndroidDeviceCodecs.capabilitiesFrom { error("MediaCodecList exploded") }
assertFalse("a failed enumeration must not claim a hardware encoder", codecs.canEncode(VideoCodec.H264))
assertFalse(codecs.canDecode("h264"))
assertEquals(emptySet<String>(), codecs.hardwareEncoders())
}
/**
* A list that throws partway keeps what it already read.
*
* This predates the seam — `runCatching` has always wrapped the iteration rather than a list
* built before it — and it is asserted here because the seam is where it could quietly have
* been lost. Taking a `List` instead of a `Sequence` would move the throw outside the loop and
* turn this partial answer into an empty one, with no test to notice.
*/
@Test
fun `codecs read before a failing entry are kept`() {
val codecs = AndroidDeviceCodecs.capabilitiesFrom {
sequence {
yield(entry("good", encoder = true, accelerated = true, types = listOf(AVC)))
error("the sixth codec's properties threw")
}
}
assertEquals(setOf(AVC), codecs.hardwareEncoders())
}
private fun capabilities(vararg entries: AndroidDeviceCodecs.Companion.CodecEntry) =
AndroidDeviceCodecs.capabilitiesFrom { entries.asSequence() }
private fun entry(
canonicalName: String,
encoder: Boolean,
accelerated: Boolean = true,
softwareOnly: Boolean = false,
alias: Boolean = false,
types: List<String>,
) = AndroidDeviceCodecs.Companion.CodecEntry(
canonicalName = canonicalName,
isAlias = alias,
isEncoder = encoder,
isHardwareAccelerated = accelerated,
isSoftwareOnly = softwareOnly,
supportedTypes = types,
)
private companion object {
const val AVC = "video/avc"
const val HEVC = "video/hevc"
}
}
@@ -1,180 +0,0 @@
package org.libremediaconverter.convert
import android.app.Application
import androidx.media3.common.util.UnstableApi
import androidx.work.OneTimeWorkRequestBuilder
import androidx.work.WorkInfo
import androidx.work.WorkManager
import androidx.work.workDataOf
import kotlinx.coroutines.Dispatchers
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.join.JoinState
import org.libremediaconverter.join.JoinViewModel
import org.libremediaconverter.model.InputProbe
import org.libremediaconverter.work.ConcatWorker
import org.libremediaconverter.work.ConversionWorker
import org.libremediaconverter.work.JobTags
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment
import java.util.UUID
import java.util.concurrent.TimeUnit
/**
* That `cancel()` cancels the job, on both screens.
*
* ## Why this was missing, which is the interesting part
*
* Both `cancel()` methods are one line — `activeWorkId?.let(workManager::cancelWorkById)` — and
* **JaCoCo reports every line of both as covered**. `SettingsEditsTest`'s
* `cancelling with no active job does nothing rather than throwing` runs the method, and its own
* comment names which half it drives: "`activeWorkId?.let(...)` -- the null side". The other side
* had never been entered, and `JoinViewModel.cancel()` had no test at all.
*
* So no line-level coverage filter could see this. What surfaces it is a method-level read —
* `mi=11, ci=7, mb=1, cb=1` on both — a covered method with an arm nothing takes. That is the
* second of the two filters #194 records, and this is the gap that argued for it.
*
* The affordance tests are not this. `ConverterStateAffordancesTest` and `JoinStateAffordancesTest`
* click `TestTags.CANCEL` and assert the *action* fires into a stub; `ScreenWiringTest` asserts the
* action calls `viewModel.cancel()`. Both halves were pinned and the join between them was not, so
* nothing in 584 tests connected the button to WorkManager.
*
* ## Why the job is enqueued with a delay
*
* The test WorkManager runs on a `SynchronousExecutor`, so an ordinary request finishes inline —
* which is exactly why only the null half was ever covered: by the time a test could call
* `cancel()`, `convert()`'s job was already terminal. `setInitialDelay` is what `TestScheduler`
* honours, so the job sits in `ENQUEUED` until the test lets it go, and it never does.
*
* **Production never sets a delay**, so the request is built here rather than through
* `ConversionWorker.request`. The *state* is not synthetic: `ENQUEUED` at `runAttemptCount == 0` is
* what every job passes through before the scheduler picks it up, `Reattachment.choose` ranks it
* `QUEUED`, and `conversionStateFrom` maps it to `Converting(input, 0)`. The delay changes how long
* the job stays in a real state, not which state it is in.
*
* ## What is asserted, and in which order
*
* WorkManager's own record first, then the screen. The screen alone would be a weaker claim than it
* looks: `CANCELLED` maps to `Idle` for a reattached job, and `Idle` is also where a ViewModel that
* did nothing at all would sit.
*/
@UnstableApi
@RunWith(RobolectricTestRunner::class)
class CancelReachesWorkManagerTest {
private lateinit var app: Application
private lateinit var workManager: WorkManager
@Before
fun setUp() {
app = RuntimeEnvironment.getApplication()
ConversionDependencies.publisher = { RecordingPublisher(app) }
ConversionDependencies.probe = { _, _ -> InputProbe() }
installTestWorkManager(app, workDataOf())
workManager = WorkManager.getInstance(app)
}
@After
fun tearDown() {
ConversionDependencies.reset()
}
@Test
fun `cancelling a queued conversion cancels that job`() {
val id = enqueueQueuedConversion()
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
awaitState(viewModel.state, "Converting") { it is ConversionState.Converting }
viewModel.cancel()
assertEquals(
"Cancel must reach WorkManager, not just the screen",
WorkInfo.State.CANCELLED,
stateOf(id),
)
awaitState(viewModel.state, "Idle") { it is ConversionState.Idle }
}
@Test
fun `cancelling a queued join cancels that job`() {
val id = enqueueQueuedJoin()
val viewModel = JoinViewModel(app, Dispatchers.Unconfined)
awaitState(viewModel.state, "Joining") { it is JoinState.Joining }
viewModel.cancel()
assertEquals(
"Cancel must reach WorkManager, not just the screen",
WorkInfo.State.CANCELLED,
stateOf(id),
)
awaitState(viewModel.state, "Idle") { it is JoinState.Idle }
}
/**
* The negative that bounds both: cancelling must cancel the job the screen is showing, and only
* that one.
*
* Without this, `cancel()` could cancel everything in the queue — `cancelAllWork()` in place of
* `cancelWorkById(activeWorkId)` — and both tests above would still pass.
*/
@Test
fun `cancelling one conversion leaves another queued job alone`() {
val bystander = enqueueQueuedConversion(displayName = "beach.mp4")
val id = enqueueQueuedConversion(displayName = "holiday.mp4")
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
val converting = awaitState(viewModel.state, "Converting") { it is ConversionState.Converting }
val onScreen = (converting as ConversionState.Converting).input.displayName
viewModel.cancel()
// Which of the two the ViewModel reattached to is the query's business, not this test's --
// the comparator leaves queued jobs tied deliberately, per Reattachment's ordering notes.
// So assert the shape rather than the identity: exactly one is cancelled, and the other is
// untouched.
val cancelled = listOf(id, bystander).filter { stateOf(it) == WorkInfo.State.CANCELLED }
assertEquals(
"exactly one job may be cancelled, with $onScreen on screen",
1,
cancelled.size,
)
}
private fun stateOf(id: UUID): WorkInfo.State =
requireNotNull(workManager.getWorkInfoById(id).get()) { "no WorkInfo for $id" }.state
/**
* A conversion sitting in the queue, which is where every job starts.
*
* Built by hand rather than through `ConversionWorker.request` for the reason in the class
* KDoc; the display-name tag is included because `reattach()` reads it for the file card, and a
* job without one would exercise the `UNKNOWN_INPUT_NAME` fallback instead of this test's
* subject.
*/
private fun enqueueQueuedConversion(displayName: String = "holiday.mp4"): UUID {
val request = OneTimeWorkRequestBuilder<ConversionWorker>()
.addTag(JobTags.displayName(displayName))
.setInitialDelay(QUEUE_HOLD_HOURS, TimeUnit.HOURS)
.build()
workManager.enqueue(request).result.get()
return request.id
}
private fun enqueueQueuedJoin(inputCount: Int = 2): UUID {
val request = OneTimeWorkRequestBuilder<ConcatWorker>()
.addTag(JobTags.inputCount(inputCount))
.setInitialDelay(QUEUE_HOLD_HOURS, TimeUnit.HOURS)
.build()
workManager.enqueue(request).result.get()
return request.id
}
private companion object {
/** Long enough that `TestScheduler` never releases the job during a test run. */
const val QUEUE_HOLD_HOURS = 1L
}
}
@@ -1,192 +0,0 @@
package org.libremediaconverter.convert
import com.arthenica.ffmpegkit.MediaInformation
import com.arthenica.ffmpegkit.StreamInformation
import org.json.JSONObject
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertNull
import org.junit.Assert.assertTrue
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.model.Container
import org.robolectric.RobolectricTestRunner
/**
* What FFprobe's answer means, read as a function of the answer alone.
*
* `readMediaInformation` was 114 missed instructions and 24 missed branches — the second-biggest
* block on the wave-4 report — of which **exactly one line needed a device**:
*
* ```kotlin
* FFprobeKit.getMediaInformation(path).getMediaInformation()
* ```
*
* Everything after it reads an ordinary object. `javap` over the committed AAR's runtime jar:
* `MediaInformation(JSONObject, List<StreamInformation>, List<Chapter>)` and
* `StreamInformation(JSONObject)` are plain public constructors, and neither class's `<clinit>`
* loads the native library — so the fixtures below are built without `libffmpegkit` present.
*
* ## The one that matters
*
* `containerFrom(formatName, video?.getCodec())`. FFprobe reports `matroska,webm` for **both** MKV
* and WebM, because they share a demuxer, so the video codec is the only thing separating them.
* `containerFrom` has thirty-three covered branches of its own and not one of them can notice the
* argument being dropped — the mistake would be at the call, not in the callee, and every existing
* `containerFrom` test would stay green while every VP9 WebM quietly became an MKV.
*
* Robolectric only for `org.json`, which is a stub in a plain JVM test.
*/
@RunWith(RobolectricTestRunner::class)
class FFprobeMappingTest {
@Test
fun `the video codec decides between matroska and webm`() {
assertEquals(
Container.WEBM,
MediaProbe.ffprobeInfoFrom(info("matroska,webm", stream("video", "vp9"))).container,
)
assertEquals(
Container.MKV,
MediaProbe.ffprobeInfoFrom(info("matroska,webm", stream("video", "h264"))).container,
)
}
/**
* The same format name with no video stream at all, which is what makes the case above about
* the *argument* rather than about the format string.
*/
@Test
fun `a matroska container with no video track cannot be told from webm and is not guessed`() {
val read = MediaProbe.ffprobeInfoFrom(info("matroska,webm", stream("audio", "opus")))
assertEquals(Container.MKV, read.container)
assertNull(read.videoCodec)
}
@Test
fun `the first stream of each type wins`() {
val read = MediaProbe.ffprobeInfoFrom(
info(
"mov,mp4,m4a,3gp,3g2,mj2",
stream("video", "h264", width = 1920, height = 1080),
stream("video", "hevc", width = 640, height = 480),
stream("audio", "aac"),
stream("audio", "mp3"),
),
)
assertEquals("h264", read.videoCodec)
assertEquals("aac", read.audioCodec)
assertEquals(1920, read.width)
assertEquals(1080, read.height)
}
/**
* Dimensions come from the stream the codec came from, not from whichever stream has some.
*
* The fixture is deliberately awkward: the chosen video stream carries **no** dimensions and a
* later one does. That is a real shape — FFprobe omits `width`/`height` for a stream it could
* not measure — and it is the only arrangement that separates the two readings.
*
* A first version of this file asserted the dimensions inside the case above, where the chosen
* stream was also the first one carrying any. Replacing `video?.getWidth()` with
* `streams.firstNotNullOfOrNull { it.getWidth() }` gave the same answer there and **the
* mutation survived**. It reddens here.
*/
@Test
fun `a video stream with no dimensions reports none rather than borrowing another stream's`() {
val read = MediaProbe.ffprobeInfoFrom(
info(
"mov,mp4,m4a,3gp,3g2,mj2",
stream("video", "h264"),
stream("video", "hevc", width = 640, height = 480),
),
)
assertEquals("h264", read.videoCodec)
assertEquals(0, read.width)
assertEquals(0, read.height)
}
/**
* Stream order is the file's, not a promise. An audio-first container must read the same as a
* video-first one.
*/
@Test
fun `an audio track listed first does not become the video track`() {
val read = MediaProbe.ffprobeInfoFrom(
info("mov,mp4,m4a,3gp,3g2,mj2", stream("audio", "aac"), stream("video", "h264")),
)
assertEquals("h264", read.videoCodec)
assertEquals("aac", read.audioCodec)
}
@Test
fun `a duration in seconds becomes milliseconds`() {
assertEquals(12_345L, MediaProbe.ffprobeInfoFrom(info("mp4", duration = "12.345")).durationMs)
}
/**
* Both ways a duration can be absent, and neither may throw.
*
* FFprobe reports `"N/A"` for a stream it could not measure, and omits the key entirely for
* some containers. `toDoubleOrNull` is what keeps the second from being an exception on the
* file-pick path, where there is no user-visible failure to report it as.
*/
@Test
fun `a duration that is not a number is no duration rather than a crash`() {
assertEquals(0L, MediaProbe.ffprobeInfoFrom(info("mp4", duration = "N/A")).durationMs)
assertEquals(0L, MediaProbe.ffprobeInfoFrom(info("mp4", duration = null)).durationMs)
}
@Test
fun `a file with no streams reports nothing rather than defaults that look measured`() {
val read = MediaProbe.ffprobeInfoFrom(info("mp4"))
assertNull(read.videoCodec)
assertNull(read.audioCodec)
assertEquals(0, read.width)
assertEquals(0, read.height)
}
@Test
fun `an image format is reported as one`() {
assertTrue(MediaProbe.ffprobeInfoFrom(info("png_pipe", stream("video", "png"))).isImage)
assertFalse(MediaProbe.ffprobeInfoFrom(info("mp4", stream("video", "h264"))).isImage)
}
private fun stream(type: String, codec: String, width: Int? = null, height: Int? = null) = StreamInformation(
JSONObject().apply {
put(StreamInformation.KEY_TYPE, type)
put(StreamInformation.KEY_CODEC, codec)
width?.let { put(StreamInformation.KEY_WIDTH, it) }
height?.let { put(StreamInformation.KEY_HEIGHT, it) }
},
)
/**
* The format properties are **nested** under `"format"`, which is how FFprobe reports them and
* what `MediaInformation` reads: `getFormat()` resolves through `getStringFormatProperty`, not
* off the top-level object. A first version of this helper put the keys at the top level and
* every format-dependent case failed with a null container, which is worth recording here so
* the next fixture does not have to rediscover it.
*
* Streams are the other half and are *not* nested — they come from the constructor argument.
*/
private fun info(formatName: String, vararg streams: StreamInformation, duration: String? = "1.0") =
MediaInformation(
JSONObject().apply {
put(
MediaInformation.KEY_FORMAT_PROPERTIES,
JSONObject().apply {
put(MediaInformation.KEY_FORMAT, formatName)
duration?.let { put(MediaInformation.KEY_DURATION, it) }
},
)
},
streams.toList(),
emptyList(),
)
}
@@ -51,13 +51,10 @@ import java.io.File
* here needs. `OutputPublisherPublishTest` owns what a real publish writes.
* - **The screen's two buttons.** `ConverterStateAffordancesTest` and `JoinStateAffordancesTest`
* own what each state renders; this file owns what each state carries.
* - ~~**`ConverterScreen`'s `destinationMime` line itself.**~~ **Withdrawn 2026-09-02 (#201).** The
* exemption read: "it lives in the entry point, above the `ScreenContent` seam, and reaching it
* needs a real ViewModel inside a composition". That was true when written and is no longer:
* `AdaptiveShellTest` (#173) established composing the real screens with real ViewModels, and
* #200 added the `ShadowActivity` mechanics for reading what a launcher launched. `RetrySaveMimeTest`
* now asserts the line directly. What this file still owns is the half below the seam -- what each
* state *carries* -- which is why `pendingSave()?.mimeType` is also asserted here.
* - **`ConverterScreen`'s `destinationMime` line itself.** It lives in the entry point, above the
* `ScreenContent` seam, and reaching it needs a real ViewModel inside a composition. What it
* reads -- `pendingSave()?.mimeType` -- is asserted directly instead, which is why that
* derivation was moved out of the entry point in the first place.
* - **Picking a new input while a `Failed` carries a file.** `onInputPicked` overwrites the state
* without discarding, from `Converted` exactly as much as from a carrying `Failed`, and neither
* branch renders a picker. It is a pre-existing path this change neither opens nor widens: the
@@ -205,34 +205,6 @@ class FileCardTest {
assertNoRow("Length")
}
/**
* A video the app knows a great deal about and cannot name the container of.
*
* Not an edge case. `InputProbe.container`'s own KDoc says `MediaExtractor` cannot report a
* container at all -- it comes from FFprobe -- so any run where FFprobe did not answer produces
* exactly this: real codec, real dimensions, real duration, `container = null`.
*
* **The twin was already tested and this one was not**, which is the argument for adding it.
* `FileCard` renders `probe.container?.label ?: "Unknown"` twice, once in the `AUDIO_ONLY`
* branch (`ConverterScreen.kt:660`) and once in the `VIDEO` branch (`:668`), and
* `an audio-only file nothing else could describe degrades one row at a time` drives only the
* first. Same expression, same fallback, one kind covered. That asymmetry is the same one
* `CLAUDE.md` records for including `ContainerCapabilities:94`.
*
* The other rows are asserted alongside so this is not a copy of the audio-only case: there,
* everything is unknown at once; here, one field is missing from a probe that is otherwise
* complete, and the rest must be unaffected by it.
*/
@Test
fun `a video file whose container nothing identified says so and keeps its other rows`() {
setFileCard(input(probe = VIDEO_PROBE.copy(container = null)))
assertRow("Container", "Unknown")
assertRow("Video", "${VideoCodec.H264.label} · 1920×1080")
assertRow("Audio", AudioCodec.AAC.label)
assertRow("Length", "1:30")
}
/**
* The row is one node, not a label node beside a value node. A test matching on `"Container"`
* alone would pass against either shape.
@@ -1,178 +0,0 @@
package org.libremediaconverter.convert
import android.app.Activity
import android.content.Intent
import android.net.Uri
import androidx.activity.ComponentActivity
import androidx.compose.ui.test.assertIsDisplayed
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.media3.common.util.UnstableApi
import androidx.work.Data
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Before
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.join.JoinScreen
import org.libremediaconverter.model.InputProbe
import org.libremediaconverter.ui.TestTags
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment
import org.robolectric.Shadows.shadowOf
import org.robolectric.shadows.ShadowActivity
/**
* The launcher layer above the `ScreenContent` seam — registered, and until now never resulted.
*
* ## The hazard this exists for
*
* `ConversionViewModel.onInputPicked(uri: Uri)` and `.save(destination: Uri)` are **both
* `(Uri) -> Unit`**, so swapping the two launcher callbacks at `ConverterScreen.kt:70` and `:83`
* compiles, renders, and passes the entire suite. Picking a file would attempt a save to it, and
* choosing a destination would load it as input.
*
* That is precisely the defect class `ScreenWiringTest` exists for, on the one pair it declines to
* cover: it drives `converterActions` directly and says the launcher-backed actions stay
* parameters. Correct for the `actions` seam, and it leaves the edge above that seam unpinned.
*
* Join's equivalents (`JoinScreen.kt:45`, `:55`) are `List<Uri>` and `Uri`, so they are **not**
* transposable and need no such test. The picker filter is a different matter and is covered below
* for both screens.
*
* ## The two mechanics, verified before the assertions were written
*
* Neither is used anywhere else in the suite, so both were spiked first:
*
* - **Reading what was launched** — `shadowOf(activity).nextStartedActivityForResult`, which returns
* the `Intent` with its `EXTRA_MIME_TYPES` intact.
* - **Delivering a result** — `shadowOf(activity).receiveResult(...)`, which reaches
* `ComponentActivity`'s `ActivityResultRegistry` and fires the `rememberLauncherForActivityResult`
* callback.
*
* `createAndroidComposeRule`, as `AdaptiveShellTest` uses and for the reason it gives: the screens
* compose real ViewModels through `viewModel()`, and the plain rule supplies no `ViewModelStoreOwner`.
*/
@UnstableApi
@RunWith(RobolectricTestRunner::class)
class LauncherWiringTest {
@get:Rule
val composeRule = createAndroidComposeRule<ComponentActivity>()
@Before
fun setUp() {
val app = RuntimeEnvironment.getApplication()
installTestWorkManager(app, Data.EMPTY)
// The real screen composes a real ViewModel; neither test here is about probing.
ConversionDependencies.probe = { _, _ -> InputProbe() }
}
@After
fun tearDown() = ConversionDependencies.reset()
/**
* The transposition guard. A picked file has to reach `onInputPicked`, which is observable as
* the screen arriving at `Ready` with the file card showing — `save()` from `Idle` returns at
* its own guard and leaves nothing behind.
*
* ## Why this waits rather than asserting straight away (#220)
*
* `onInputPicked` does not reach `Ready` on the calling thread. It hops twice —
* `withContext(pickDispatcher) { InputQuery.describe(...) }` and then the probe — and
* `pickDispatcher` defaults to `Dispatchers.IO`, a real background thread that Compose's
* idling does not know about. `deliver` therefore returns with the state still `Idle` more
* often than not, and asserting immediately was a race the test usually won.
*
* It lost five times on CI in one day, on PRs whose diffs were instrumented tests and
* documentation, which is what #220 was filed for. `waitUntil` polls through
* `waitForIdle`, so it drains the main looper each time round and sees the recomposition that
* the IO hop eventually posts back.
*
* **Injecting the dispatcher would be better and is not available here.** `pickDispatcher` is
* a constructor parameter precisely so a test can pin it, but this test composes the real
* `ConverterScreen`, which resolves its own ViewModel through `viewModel()` — the seam exists
* one layer below the thing under test. Pinning it would mean not testing the launcher edge,
* which is the whole point of this class.
*
* The wait does not weaken the assertion: transposing the two callbacks leaves the screen in
* `Idle` forever, so it fails on the timeout with the same meaning it failed with before.
*/
@Test
fun `a picked document is loaded as input rather than saved to`() {
composeRule.setContent { ConverterScreen() }
composeRule.onNodeWithTag(TestTags.Converter.CHOOSE_FILE).performClick()
deliver(Uri.parse("content://test/holiday.mkv"))
composeRule.waitUntil(PICK_TIMEOUT_MS) {
composeRule.onAllNodesWithTag(TestTags.Converter.FILE_CARD_NAME)
.fetchSemanticsNodes()
.isNotEmpty()
}
composeRule.onNodeWithTag(TestTags.Converter.FILE_CARD_NAME).assertIsDisplayed()
}
/**
* `ConverterScreen.kt:65-67` records why the all-types wildcard is load-bearing rather than lazy:
*
* > the picker is images and video only, offers no audio at all, and will not reliably surface
* > .mkv/.flac/.webm
*
* Narrowing it would make every audio conversion unreachable from the file picker, and nothing
* would have gone red. (The literal is spelled only in the assertion below: a KDoc cannot
* contain it, because the wildcard's second half closes the comment.)
*/
@Test
fun `the converter picker asks for every type, not just the ones a photo picker offers`() {
composeRule.setContent { ConverterScreen() }
composeRule.onNodeWithTag(TestTags.Converter.CHOOSE_FILE).performClick()
val intent = launched().intent
assertEquals(Intent.ACTION_OPEN_DOCUMENT, intent.action)
assertEquals(listOf("*/*"), intent.getStringArrayExtra(Intent.EXTRA_MIME_TYPES)?.toList())
}
@Test
fun `the join picker asks for video and accepts more than one file`() {
composeRule.setContent { JoinScreen() }
composeRule.onNodeWithTag(TestTags.Join.CHOOSE_FILES).performClick()
val intent = launched().intent
assertEquals(Intent.ACTION_OPEN_DOCUMENT, intent.action)
assertEquals(listOf("video/*"), intent.getStringArrayExtra(Intent.EXTRA_MIME_TYPES)?.toList())
// A join of one file is not a join; the contract is what asks for several.
assertEquals(true, intent.getBooleanExtra(Intent.EXTRA_ALLOW_MULTIPLE, false))
}
private fun launched(): ShadowActivity.IntentForResult {
composeRule.waitForIdle()
return requireNotNull(shadowOf(composeRule.activity).nextStartedActivityForResult) {
"nothing was launched for a result"
}
}
private fun deliver(uri: Uri) {
val started = launched()
shadowOf(composeRule.activity).receiveResult(
started.intent,
Activity.RESULT_OK,
Intent().setData(uri),
)
composeRule.waitForIdle()
}
private companion object {
/**
* Long enough that a slow CI runner is not the reason this fails, short enough that a
* genuinely transposed callback does not stall the suite. The pick normally lands in
* single-digit milliseconds.
*/
const val PICK_TIMEOUT_MS = 10_000L
}
}
@@ -1,213 +0,0 @@
package org.libremediaconverter.convert
import android.app.Application
import android.net.Uri
import androidx.media3.common.util.UnstableApi
import androidx.work.Data
import androidx.work.ListenableWorker
import androidx.work.testing.TestListenableWorkerBuilder
import androidx.work.workDataOf
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.runBlocking
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertNotNull
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.join.JoinState
import org.libremediaconverter.join.JoinViewModel
import org.libremediaconverter.model.ConversionRequest
import org.libremediaconverter.model.EnginePreference
import org.libremediaconverter.model.InputProbe
import org.libremediaconverter.model.OutputFormat
import org.libremediaconverter.work.ConcatWorker
import org.libremediaconverter.work.ConversionWorker
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment
import java.io.File
import java.util.UUID
/**
* A failure that says nothing still has to say something.
*
* Three sites, all `ci == 0` before this file, and all the same rule:
*
* ```
* work/ConversionWorker.kt:316 cause.message ?: GENERIC_FAILURE_MESSAGE
* convert/ConversionViewModel.kt:631 e.message ?: SAVE_FAILED_MESSAGE
* join/JoinViewModel.kt:416 e.message ?: SAVE_FAILED_MESSAGE
* ```
*
* Every existing test throws *with* a message, so the right-hand side had never been evaluated
* anywhere in the suite. A `Throwable` carrying none is not exotic — `RuntimeException()`,
* `IOException()` and most platform exceptions raised without an argument all have a null message.
*
* ## Held in one class, against the ticket's suggestion
*
* #193 proposed putting each case beside the behaviour it neighbours. They are together instead,
* because they are one rule at three layers and because the trap below has to be explained once
* rather than three times. `FailedSaveRetryTest` sets the precedent for both ViewModels in one
* file; this extends it by one worker.
*
* ## The trap, which is why the worker case asserts what it does
*
* `ConversionStateMappingTest`'s *"a failure with nothing said still says something"* looks like it
* already covers the worker site. It does not: it drives the **read** side, `map(FAILED, Data.EMPTY)`,
* and that side has a fallback of its own (`ConversionViewModel.kt:147-149`):
*
* ```kotlin
* update.outputData.getString(ConversionWorker.KEY_ERROR)
* ?.takeIf { it.isNotBlank() }
* ?: ConversionWorker.GENERIC_FAILURE_MESSAGE
* ```
*
* So mutating the worker's fallback to `.orEmpty()` writes `KEY_ERROR to ""`, and the ViewModel
* turns that straight back into the same constant. **A test asserting on the resulting `Failed`
* state stays green under the mutation**, which is most likely why the write-side fallback survived
* three waves of test work. The worker case therefore reads `KEY_ERROR` off the worker's own
* `Result`, before anything downstream can repair it.
*
* The two save cases have no such second line: both write `_state.value` directly, so the state is
* the right thing to assert there.
*/
@UnstableApi
@RunWith(RobolectricTestRunner::class)
class MessagelessFailureTest {
private lateinit var app: Application
private lateinit var publisher: RecordingPublisher
private lateinit var staged: File
@Before
fun setUp() {
app = RuntimeEnvironment.getApplication()
publisher = RecordingPublisher(app)
ConversionDependencies.publisher = { publisher }
ConversionDependencies.probe = { _, _ -> InputProbe() }
staged = publisher.createStagingFile("holiday.mp4").apply { writeBytes(ByteArray(4096)) }
}
@After
fun tearDown() {
ConversionDependencies.reset()
}
/**
* The engine gives up without saying why, which is what a native crash looks like from here.
*
* Asserted on the worker's own output `Data` rather than on a screen — see the class KDoc.
*/
@Test
fun `a conversion that fails without a message still reports one`() {
installTestWorkManager(app, Data.EMPTY)
ConversionDependencies.software = { MessagelessTranscoder }
val result = runBlocking { failingWorker().doWork() }
assertTrue("the job must fail rather than retry, got $result", result is ListenableWorker.Result.Failure)
assertEquals(
"a failure with no message must still put something on screen",
ConversionWorker.GENERIC_FAILURE_MESSAGE,
(result as ListenableWorker.Result.Failure).outputData.getString(ConversionWorker.KEY_ERROR),
)
}
@Test
fun `a save that fails without a message still reports one`() {
installTestWorkManager(app, conversionOutput())
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
viewModel.onInputPicked(Uri.parse("content://test/holiday.mkv"))
awaitState(viewModel.state, "Ready") { it is ConversionState.Ready }
viewModel.convert()
awaitState(viewModel.state, "Converted") { it is ConversionState.Converted }
publisher.publishFailure = RuntimeException()
viewModel.save(DESTINATION)
val failed = awaitState(viewModel.state, "Failed") { it is ConversionState.Failed } as ConversionState.Failed
assertEquals(SAVE_FAILED_MESSAGE, failed.message)
// The handle travels even on the wordless path. Without this, a fallback that also dropped
// `pending` would pass -- and the file would be unreachable from the screen that just said
// the save failed.
assertNotNull("a wordless failure must still offer the file again", failed.retry)
}
@Test
fun `a join save that fails without a message still reports one`() {
installTestWorkManager(app, joinOutput())
val viewModel = JoinViewModel(app, Dispatchers.Unconfined)
viewModel.onInputsPicked(listOf(Uri.parse("content://test/a.mp4"), Uri.parse("content://test/b.mp4")))
awaitState(viewModel.state, "Ready") { it is JoinState.Ready }
viewModel.join()
awaitState(viewModel.state, "Joined") { it is JoinState.Joined }
publisher.publishFailure = RuntimeException()
viewModel.save(DESTINATION)
val failed = awaitState(viewModel.state, "Failed") { it is JoinState.Failed } as JoinState.Failed
assertEquals(SAVE_FAILED_MESSAGE, failed.message)
assertNotNull("a wordless failure must still offer the file again", failed.retry)
}
/**
* `FORCE_SOFTWARE` so the failure comes straight out of `runFFmpeg`.
*
* `AUTO` would enter `runMedia3OrFallBack`, whose catch runs the job a second time in software
* — the same exception would arrive, but through a path this test is not about and which
* `HardwareFallbackTest` already owns.
*/
private fun failingWorker(): ConversionWorker {
val spec = OutputFormat.MP4_H265.spec
return TestListenableWorkerBuilder<ConversionWorker>(
context = app,
inputData = workDataOf(
ConversionWorker.KEY_INPUT_URI to "file:///tmp/holiday.mp4",
ConversionWorker.KEY_DISPLAY_NAME to "holiday.mp4",
ConversionWorker.KEY_CONTAINER to spec.container.name,
ConversionWorker.KEY_VIDEO_CODEC to spec.videoCodec.name,
ConversionWorker.KEY_AUDIO_CODEC to spec.audioCodec.name,
ConversionWorker.KEY_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name,
),
runAttemptCount = 0,
).setId(JOB_ID).build()
}
private fun conversionOutput() = workDataOf(
ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath,
ConversionWorker.KEY_SUGGESTED_NAME to SUGGESTED_NAME,
ConversionWorker.KEY_MIME_TYPE to JOB_MIME_TYPE,
)
private fun joinOutput() = workDataOf(
ConcatWorker.KEY_OUTPUT_PATH to staged.absolutePath,
ConcatWorker.KEY_SUGGESTED_NAME to SUGGESTED_NAME,
ConcatWorker.KEY_MIME_TYPE to JOB_MIME_TYPE,
)
private companion object {
val DESTINATION: Uri = Uri.parse("content://test/destination.mp4")
val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-00000000019a")
const val SUGGESTED_NAME = "holiday.mp4"
const val JOB_MIME_TYPE = "video/mp4"
}
}
/**
* An engine that gives up without saying why.
*
* `RuntimeException()` rather than a subclass with a blank message: `Throwable.message` is *null*
* here, which is the case the elvis exists for. A blank-but-present message takes the left-hand
* side and is a different path — `ConversionStateMappingTest` covers that one, on the read side.
*/
@UnstableApi
private object MessagelessTranscoder : SoftwareTranscoder {
override suspend fun run(
request: ConversionRequest,
inputPath: String,
output: File,
durationMs: Long,
onProgress: (Int) -> Unit,
): Unit = throw RuntimeException()
}
@@ -122,25 +122,24 @@ class OutputPublisherStagingTest {
/**
* Makes `cacheDir/conversions` a regular file, which is the whole precondition of the test
* above -- and does it in a loop, because a single delete-then-write once lost a race that CI
* caught and this machine did not reproduce.
* above -- and does it in a loop, because a single delete-then-write loses a race that CI
* caught and this machine does not reproduce.
*
* **That race is closed at the source as of #159, and the loop is kept anyway.**
* `LibreMediaConverterApp.onCreate` launched its staging sweep on `Dispatchers.IO`, and
* `sweepStaging` reads `stagingDir`, whose getter calls `mkdirs()`. Robolectric builds an
* application for every test class that asks for one, so that background `mkdirs()` was in
* flight across the whole suite, on a thread the paused main looper does not control. Between
* deleting this path and writing it there is a window where the path does not exist and that
* `mkdirs()` could win -- `FileNotFoundException: ... (Is a directory)` out of `writeBytes`,
* run 33069641674 on #149, once, against 468 tests that passed here. The JVM suite now runs
* `TestLibreMediaConverterApp`, whose sweep finishes before `onCreate()` returns, so nothing is
* sweeping while a test body runs.
* `LibreMediaConverterApp.onCreate` ends with
* `appScope.launch { OutputPublisher(...).sweepStaging() }` on `Dispatchers.IO`, and
* `sweepStaging` reads `stagingDir`, whose getter calls `mkdirs()`. Robolectric instantiates
* the application for every test that asks for one, so that background `mkdirs()` is in flight
* across the whole suite, on a thread the paused main looper does not control. Between deleting
* this path and writing it there is a window where the path does not exist and that `mkdirs()`
* can win, which is `FileNotFoundException: ... (Is a directory)` out of `writeBytes` -- run
* 33069641674 on #149, once, against 468 tests that pass here.
*
* The loop stays because it is what would catch that substitution being undone. Without it the
* regression returns as this one class failing rarely on CI -- the exact shape that took #159
* from a single run on #149 to a wave-4 flake before anyone chased it. Retrying closes the
* window rather than narrowing it, because the race is not symmetric: `mkdirs()` fails on an
* existing regular file, so the invariant only has to survive being *established*.
* Retrying closes it rather than narrowing it, because the race is not symmetric: `mkdirs()`
* fails on an existing regular file, so the invariant only has to survive being *established*.
* Once a write lands, nothing in the suite can turn this back into a directory.
*
* The wider problem -- application-scope IO work racing every Robolectric test that shares
* `cacheDir` -- is #159, and is deliberately not fixed here.
*/
private fun stagingPathAsRegularFile(): File {
val stagingPath = File(cacheDir, "conversions")
@@ -1,136 +0,0 @@
package org.libremediaconverter.convert
import android.app.Application
import android.content.Intent
import android.net.Uri
import androidx.activity.ComponentActivity
import androidx.compose.ui.test.junit4.v2.createAndroidComposeRule
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.work.WorkManager
import androidx.work.workDataOf
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertNotEquals
import org.junit.Before
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.model.InputProbe
import org.libremediaconverter.model.OutputFormat
import org.libremediaconverter.ui.TestTags
import org.libremediaconverter.work.ConversionWorker
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment
import org.robolectric.Shadows.shadowOf
import java.io.File
/**
* The save dialog opens with the type the *job* produced, not the type the picker is showing now.
*
* `ConverterScreen.kt:80` — `state.pendingSave()?.mimeType ?: settings.spec.mimeType` — had never
* taken its left-hand side. Its comment records what the line is for:
*
* > a retry offered after a failed save opens the dialog with the type its first attempt used —
* > the cast answered null for a `Failed`, and the fallback below is the current picker, which a
* > reattached job never set.
*
* So the untested half is the fix, and the tested half is the fallback it was added to stop being
* used.
*
* ## This revises a named exemption, deliberately
*
* `FailedSaveRetryTest`'s KDoc lists this line under "Not asserted here, so each is a decision
* rather than an omission":
*
* > It lives in the entry point, above the `ScreenContent` seam, and reaching it needs a real
* > ViewModel inside a composition.
*
* That was true when written. `AdaptiveShellTest` (#173) then established exactly that capability,
* and #200 added the two `ShadowActivity` mechanics that let a test read what a launcher launched.
* The reason the exemption gave no longer holds, so the exemption is withdrawn rather than left to
* be taken at face value — the same shape as #141 revising #84's boundary. That KDoc is corrected
* in this change.
*
* ## Why the job is reattached rather than run
*
* The screen composes its own ViewModel through `viewModel()`, so nothing can be injected into it.
* A job finished before the composition is the one route to a `Converted` state carrying output
* `Data` this test chose — and it is also the case the line exists for, since a reattached job's
* spec "was never in these settings at all".
*/
@UnstableApi
@RunWith(RobolectricTestRunner::class)
class RetrySaveMimeTest {
@get:Rule
val composeRule = createAndroidComposeRule<ComponentActivity>()
private lateinit var app: Application
private lateinit var staged: File
@Before
fun setUp() {
app = RuntimeEnvironment.getApplication()
ConversionDependencies.probe = { _, _ -> InputProbe() }
staged = OutputPublisher(app).createStagingFile("holiday.mkv").apply { writeBytes(ByteArray(4096)) }
}
@After
fun tearDown() = ConversionDependencies.reset()
@Test
fun `the save dialog offers the type the job produced, not the one the picker is showing`() {
finishAJobProducing(JOB_MIME_TYPE)
composeRule.setContent { ConverterScreen() }
composeRule.waitForIdle()
composeRule.onNodeWithTag(TestTags.SAVE_FILE).performScrollTo().performClick()
composeRule.waitForIdle()
val intent = requireNotNull(shadowOf(composeRule.activity).nextStartedActivityForResult) {
"the save dialog was never launched"
}.intent
assertEquals(Intent.ACTION_CREATE_DOCUMENT, intent.action)
assertEquals(JOB_MIME_TYPE, intent.type)
// The fixture is only meaningful while the two differ; without this the assertion above
// would pass just as well against the fallback.
assertNotEquals(
"the picker's own type must differ, or this test proves nothing",
JOB_MIME_TYPE,
OutputFormat.MP4_H265.spec.mimeType,
)
}
/**
* A conversion that finished while nothing was watching, which is what `reattach()` picks up.
*
* `SucceedingWorkerFactory` reports this output `Data` for whatever is enqueued, so the job
* lands `SUCCEEDED` carrying a staged path that exists — the two things `Reattachment.choose`
* requires of a finished job.
*/
private fun finishAJobProducing(mimeType: String) {
installTestWorkManager(
app,
workDataOf(
ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath,
ConversionWorker.KEY_SUGGESTED_NAME to "holiday.mkv",
ConversionWorker.KEY_MIME_TYPE to mimeType,
),
)
WorkManager.getInstance(app).enqueue(
ConversionWorker.request(
inputUri = Uri.parse("content://test/holiday.mkv"),
displayName = "holiday.mkv",
sizeBytes = 4_096L,
),
).result.get()
}
private companion object {
/** Matroska, against the MP4 the picker defaults to. */
const val JOB_MIME_TYPE = "video/x-matroska"
}
}
@@ -1,147 +0,0 @@
package org.libremediaconverter.convert
import android.app.Application
import android.net.Uri
import androidx.media3.common.util.UnstableApi
import androidx.work.WorkManager
import androidx.work.workDataOf
import kotlinx.coroutines.Dispatchers
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.join.JoinState
import org.libremediaconverter.join.JoinViewModel
import org.libremediaconverter.model.InputProbe
import org.libremediaconverter.work.ConcatWorker
import org.libremediaconverter.work.ConversionWorker
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment
/**
* An answer that arrives after the screen has moved on does nothing.
*
* Four refusal arms, cold before this file:
*
* ```
* convert/ConversionViewModel.kt:513 currentInput() ?: return
* convert/ConversionViewModel.kt:600 pendingSave() ?: return
* join/JoinViewModel.kt:316 (as? Ready)?.inputs ?: return
* join/JoinViewModel.kt:390 pendingSave() ?: return
* ```
*
* They are not merely defensive. `ConverterScreen.kt:91` wires `convert()` to the
* **POST_NOTIFICATIONS result**, and `:83` wires `save()` to the CreateDocument result — so both
* are entered by a system callback rather than by a tap, and a result redelivered after process
* death arrives at a brand-new ViewModel sitting on `Idle`.
*
* ## The production change that came with this
*
* `currentInput()` used to answer for `Converting`, `Waiting` and `Converted` as well as `Ready`.
* Those arms were unreachable by tapping Convert but reachable through that permission callback,
* and reaching one enqueued a **second** job over a live one — `activeWorkId` overwritten, the
* first job still running with an orphaned notification and nothing holding its id.
*
* #202 decided to narrow rather than to test it as it stood, because a test written against the old
* shape would have frozen the double-enqueue as intended behaviour. `JoinViewModel.join()` has been
* `(_state.value as? JoinState.Ready)?.inputs ?: return` all along; the two screens are the same
* shape and only one was over-general.
*/
@UnstableApi
@RunWith(RobolectricTestRunner::class)
class StaleLauncherResultTest {
private lateinit var app: Application
private lateinit var workManager: WorkManager
private lateinit var staged: java.io.File
@Before
fun setUp() {
app = RuntimeEnvironment.getApplication()
val publisher = RecordingPublisher(app)
ConversionDependencies.publisher = { publisher }
ConversionDependencies.probe = { _, _ -> InputProbe() }
// A real staged file, because a SUCCEEDED job with no output path maps to Failed rather
// than Converted -- and Converted is the state this file's second case has to reach.
staged = publisher.createStagingFile("holiday.mp4").apply { writeBytes(ByteArray(4096)) }
installTestWorkManager(
app,
workDataOf(
ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath,
ConversionWorker.KEY_SUGGESTED_NAME to "holiday.mp4",
ConversionWorker.KEY_MIME_TYPE to "video/mp4",
),
)
workManager = WorkManager.getInstance(app)
}
@After
fun tearDown() = ConversionDependencies.reset()
@Test
fun `a permission answer arriving on an empty screen enqueues nothing`() {
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
awaitState(viewModel.state, "Idle") { it is ConversionState.Idle }
viewModel.convert()
assertEquals(ConversionState.Idle, viewModel.state.value)
assertEquals("nothing may be enqueued for a file that is not there", 0, conversionJobs())
}
/**
* The narrowing itself: a permission answer that arrives while a conversion is already running
* must not start a second one.
*
* Reached by converting once — the synchronous test WorkManager finishes it inline, so the
* screen is `Converted`, which is one of the three arms `currentInput()` used to answer for.
* Calling `convert()` again from there is precisely what the permission callback can do.
*/
@Test
fun `a permission answer arriving after the job finished does not start a second one`() {
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
viewModel.onInputPicked(Uri.parse("content://test/holiday.mkv"))
awaitState(viewModel.state, "Ready") { it is ConversionState.Ready }
viewModel.convert()
val converted = awaitState(viewModel.state, "Converted") { it is ConversionState.Converted }
assertEquals("the fixture needs exactly one job to start with", 1, conversionJobs())
viewModel.convert()
assertEquals("a second job must not be enqueued over the first", 1, conversionJobs())
assertEquals("and the screen must not move", converted, viewModel.state.value)
}
@Test
fun `a save answer arriving on an empty screen does nothing`() {
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
awaitState(viewModel.state, "Idle") { it is ConversionState.Idle }
viewModel.save(DESTINATION)
assertEquals(ConversionState.Idle, viewModel.state.value)
}
@Test
fun `a join answer arriving on an empty screen enqueues nothing`() {
val viewModel = JoinViewModel(app, Dispatchers.Unconfined)
awaitState(viewModel.state, "Idle") { it is JoinState.Idle }
viewModel.join()
viewModel.save(DESTINATION)
assertEquals(JoinState.Idle, viewModel.state.value)
assertEquals(0, joinJobs())
}
private fun conversionJobs() = jobsTagged(ConversionWorker::class.java.name)
private fun joinJobs() = jobsTagged(ConcatWorker::class.java.name)
private fun jobsTagged(tag: String) = workManager.getWorkInfosByTag(tag).get().size
private companion object {
val DESTINATION: Uri = Uri.parse("content://test/destination.mp4")
}
}
@@ -190,30 +190,6 @@ class FFmpegCommandBuilderTest {
assertPair(cmd(OutputFormat.M4A_AAC), "-b:a", "192k")
}
/**
* Turning audio off, which the Advanced picker offers and nothing had ever built a command for.
*
* `audioArgs`' `Drop` arm was `ci == 0`. The suite's only `-an` assertion is in
* `gif generates a palette to avoid banding and drops audio`, and that one comes from the image
* path (`FFmpegCommandBuilder.kt:79`/`:90`), which emits `-an` directly and never reaches
* `audioArgs`. Two sites, one string, one tested.
*
* It is a live path rather than defensive code: `AdvancedPicker` renders all of
* `AudioCodec.entries` including `NONE`, `ContainerCapabilities.validate` permits audio-off
* whenever the input has video, and MKV routes the job to FFmpeg.
*
* Both halves are asserted. `-an` alone would still pass if the arm fell through to the `else`
* and emitted an AAC encoder beside it -- a file that is silent because the flag won, carrying
* an encoder nobody asked for.
*/
@Test
fun `turning audio off drops the track instead of encoding one`() {
val args = cmd(OutputSpec(Container.MKV, VideoCodec.H264, AudioCodec.NONE))
assertTrue("audio turned off must emit -an, got $args", args.contains("-an"))
assertFalse("a dropped track must not also carry an encoder, got $args", args.contains("-c:a"))
}
@Test
fun `audio only formats never carry a video encoder`() {
listOf(OutputFormat.MP3, OutputFormat.FLAC, OutputFormat.WAV, OutputFormat.OPUS)
@@ -56,33 +56,6 @@ class FFmpegConcatCommandTest {
assertEquals("0", args[args.indexOf("-safe") + 1])
}
/**
* The gate that `-safe 0` does not open, and the one every real join needs (#238).
*
* `-safe 0` permits absolute *paths*; the concat demuxer separately whitelists the *protocol*,
* defaulting to `file,crypto,data`. `JoinScreen` picks with `OpenMultipleDocuments`, so real
* inputs are `content://` and `ConcatEngine` writes `ffkitsaf:` paths into the list file — which
* the demuxer refused outright, failing every stream-copy join a user could actually start.
*
* The re-encode strategy has no equivalent assertion because it needs none: it passes each
* input with its own `-i` and never feeds the demuxer a list file. That asymmetry is exactly
* why the defect survived — joining mismatched clips over SAF worked.
*/
@Test
fun `stream copy whitelists the protocol its list file entries actually use`() {
val args = FFmpegConcatCommand.build(
ConcatStrategy.STREAM_COPY,
inputs,
listFile,
output,
OutputFormat.MP4_H264,
)
val whitelist = args[args.indexOf("-protocol_whitelist") + 1].split(",")
assertTrue("ffmpeg-kit's SAF scheme must be permitted, got $whitelist", "ffkitsaf" in whitelist)
// The defaults have to survive too: the list file itself is opened over `file`.
assertTrue("the demuxer still reads the list file itself, got $whitelist", "file" in whitelist)
}
@Test
fun `re-encode passes every input separately and builds a filter graph`() {
val args = FFmpegConcatCommand.build(
@@ -1,127 +0,0 @@
package org.libremediaconverter.ffmpeg
import com.arthenica.ffmpegkit.ReturnCode
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Test
/**
* What a finished FFmpegKit session means, for both engines at once.
*
* `FFmpegEngine` and `ConcatEngine` each carried their own copy of this `when`, and the copies had
* drifted: one preferred the fail stack trace and fell back to the log tail, the other only ever
* read the log tail. Neither was tested, because both live inside a callback handed to `FFmpegKit`,
* which does not run on the JVM — so nothing could see that the two disagreed.
*
* **JVM-safe, verified rather than assumed.** `javap` over the committed AAR's runtime jar shows
* `ReturnCode(int)` as a plain public constructor with `SUCCESS`/`CANCEL` int constants and pure
* static `isSuccess`/`isCancel`; its `<clinit>` is constant initialisation and loads no native
* library.
*
* The unification is #203's decision, so the tests pin it as one: a join failure now carries the
* stack trace a conversion failure always did, while the two prefixes stay distinct.
*/
class SessionOutcomeTest {
@Test
fun `a return code of zero is success`() {
assertEquals(SessionOutcome.Success, outcome(ReturnCode(ReturnCode.SUCCESS)))
}
/**
* Cancellation is a separate outcome from failure, and the distinction is the point: the engines
* resume the continuation *cancelled* rather than exceptionally, so a user who pressed Cancel
* does not get an error card.
*/
@Test
fun `a return code of 255 is a cancellation, not a failure`() {
assertEquals(SessionOutcome.Cancelled, outcome(ReturnCode(ReturnCode.CANCEL)))
}
@Test
fun `any other return code fails, and the sentence carries the number`() {
val failed = outcome(ReturnCode(1), stackTrace = "boom") as SessionOutcome.Failed
assertTrue("the code belongs in the message, got: ${failed.message}", failed.message.contains("(1)"))
}
/**
* The half that was different between the two engines before #203, now the same in both.
*/
@Test
fun `the stack trace is preferred over the log tail`() {
val failed = outcome(ReturnCode(1), stackTrace = "the real cause", logTail = "…noise…")
as SessionOutcome.Failed
assertTrue(failed.message.contains("the real cause"))
assertTrue("the log tail must not be appended as well", !failed.message.contains("noise"))
}
@Test
fun `a blank stack trace falls back to the log tail`() {
val blank = outcome(ReturnCode(1), stackTrace = " ", logTail = "the last few lines") as SessionOutcome.Failed
val absent = outcome(ReturnCode(1), stackTrace = null, logTail = "the last few lines") as SessionOutcome.Failed
assertTrue(blank.message.contains("the last few lines"))
assertTrue("a null stack trace is a blank one", absent.message.contains("the last few lines"))
}
/**
* Both sources empty still has to produce a sentence. A message ending in a dangling colon is
* thin, but it is what the user gets when FFmpeg said nothing at all, and it must not be an
* exception on the way to the screen.
*/
@Test
fun `a failure with nothing to say still names the code`() {
val failed = outcome(ReturnCode(1), stackTrace = null, logTail = null) as SessionOutcome.Failed
assertEquals("FFmpeg failed (1): ", failed.message)
}
/**
* `getReturnCode()` is nullable and a session killed before it reported anything has none.
* Neither success nor cancellation, so it fails — and the sentence says so rather than throwing.
*/
@Test
fun `a session with no return code at all fails`() {
val failed = outcome(null, logTail = "whatever was logged") as SessionOutcome.Failed
assertTrue("got: ${failed.message}", failed.message.startsWith("FFmpeg failed (null): "))
}
/**
* Unifying the *strategy* must not unify the *sentence*: the two engines describe different
* jobs, and a join that reports "FFmpeg failed" is a worse message than the one it replaced.
*/
@Test
fun `each engine keeps its own prefix`() {
val join = sessionOutcome(ReturnCode(1), "Joining", { "cause" }, { null }) as SessionOutcome.Failed
assertTrue(join.message.startsWith("Joining failed (1): "))
}
/**
* Neither message source is read unless the outcome is a failure.
*
* They are calls onto a native session, and reading them on the happy path is work every
* successful conversion would do for nothing — which the shape this replaced did not, since it
* read them inside the `else` branch. That is why the parameters are lambdas, and this is what
* would notice if they stopped being.
*/
@Test
fun `a session that succeeded reads neither the stack trace nor the log`() {
var reads = 0
fun counted(): String? {
reads++
return null
}
sessionOutcome(ReturnCode(ReturnCode.SUCCESS), "FFmpeg", ::counted, ::counted)
sessionOutcome(ReturnCode(ReturnCode.CANCEL), "FFmpeg", ::counted, ::counted)
assertEquals("neither source may be touched unless the session failed", 0, reads)
}
private fun outcome(rc: ReturnCode?, stackTrace: String? = null, logTail: String? = null) =
sessionOutcome(rc, "FFmpeg", { stackTrace }, { logTail })
}
@@ -1,89 +0,0 @@
package org.libremediaconverter.ui.theme
import androidx.compose.material3.ColorScheme
import androidx.compose.material3.MaterialTheme
import androidx.compose.ui.test.junit4.v2.createComposeRule
import org.junit.Assert.assertEquals
import org.junit.Assert.assertNotEquals
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.robolectric.RobolectricTestRunner
import org.robolectric.annotation.Config
/**
* The theme called the way the app calls it: with no arguments at all.
*
* [ThemeColorSchemeTest] resolves every branch of the `when` and always passes `darkTheme`
* explicitly, so the `$default` bridge is never entered and **`isSystemInDarkTheme()` is never
* called**. `MainActivity.kt:79` is its only default-argument caller and does not execute on the
* JVM, which left the app's actual call shape the one nothing exercised —
* `LibreMediaConverterTheme` reported `mi=21, mb=6, cb=12` at method level.
*
* ## Not #68
*
* #68 is about the two **unreachable** arms, `DarkColorScheme` and `LightColorScheme`, which cannot
* run because `dynamicColor` is always `true` and nothing can flip it. That is an open product
* decision. This is the reachable half — whether the default follows the system — and closing it
* does not close that.
*
* ## Why the assertion compares schemes rather than reading a number
*
* A luminance threshold would be a guess about the device palette. What is asserted instead is that
* the no-argument call resolves to **the same scheme** an explicit `darkTheme` of the matching
* value does, and a different one from its opposite. That holds whatever palette the platform
* hands back, and it is exactly the claim: the default reads the system rather than picking a side.
*
* Both schemes are resolved in one composition because `setContent` may be called once per test.
*/
@RunWith(RobolectricTestRunner::class)
class ThemeFollowsSystemTest {
@get:Rule
val composeRule = createComposeRule()
@Test
@Config(qualifiers = "+night")
fun `with no arguments the theme follows a system in dark mode`() {
val resolved = resolve()
assertEquals("the default must resolve what darkTheme = true does", resolved.dark, resolved.bare)
assertNotEquals(resolved.light, resolved.bare)
}
@Test
@Config(qualifiers = "+notnight")
fun `with no arguments the theme follows a system in light mode`() {
val resolved = resolve()
assertEquals("the default must resolve what darkTheme = false does", resolved.light, resolved.bare)
assertNotEquals(resolved.dark, resolved.bare)
}
/**
* The three colours are read together as one value, because any single one could coincide
* between the two schemes on some palette while the schemes themselves differ. Background is
* what dark mode is chiefly about; primary and surface are along to make a coincidence
* implausible rather than merely unlikely.
*/
private data class Fingerprint(val background: Long, val primary: Long, val surface: Long)
private fun ColorScheme.fingerprint() =
Fingerprint(background.value.toLong(), primary.value.toLong(), surface.value.toLong())
private class Resolved(val bare: Fingerprint, val dark: Fingerprint, val light: Fingerprint)
private fun resolve(): Resolved {
lateinit var bare: Fingerprint
lateinit var dark: Fingerprint
lateinit var light: Fingerprint
composeRule.setContent {
// No arguments — the call MainActivity makes, and the one nothing exercised.
LibreMediaConverterTheme { bare = MaterialTheme.colorScheme.fingerprint() }
LibreMediaConverterTheme(darkTheme = true) { dark = MaterialTheme.colorScheme.fingerprint() }
LibreMediaConverterTheme(darkTheme = false) { light = MaterialTheme.colorScheme.fingerprint() }
}
composeRule.waitForIdle()
return Resolved(bare, dark, light)
}
}
@@ -21,10 +21,8 @@ import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.convert.ConversionDependencies
import org.libremediaconverter.convert.HardwareTranscoder
import org.libremediaconverter.convert.SoftwareTranscoder
import org.libremediaconverter.convert.installTestWorkManager
import org.libremediaconverter.model.Container
import org.libremediaconverter.model.ConversionRequest
import org.libremediaconverter.model.DeviceCodecs
import org.libremediaconverter.model.EnginePreference
@@ -131,40 +129,6 @@ class ProgressNotificationTest {
)
}
/**
* The same plumbing on the engine most conversions actually use, which had none.
*
* `ConversionWorker.kt:208-210` is a second `onProgress` lambda at a second call site — the one
* handed to `engine.transcode` — and it reported `ci == 0`. Every test above drives the FFmpeg
* path; `HardwareFallbackTest` reaches `runMedia3OrFallBack` but its recording transcoder
* records the call and never invokes the callback it was given. So the two engines' progress
* wiring was one tested and one not, and the untested one is the default: `ConversionRouter`
* sends everything it can to Media3.
*
* `AUTO` with a real H.264 probe, because `FORCE_SOFTWARE` is precisely what keeps the other
* tests out of this branch. The probe and the permissive codec profile are what let the router
* choose Media3 at all — `InputProbe()` reports `UNPARSEABLE`, which routes straight to FFmpeg.
*
* Asserted on the *percentage*, not merely on an update having happened: `publishProgress`
* takes a display name and a percent, and replacing the percent with a constant compiles.
*/
@Test
fun `progress from the hardware engine reaches WorkManager the same way FFmpeg's does`() {
ConversionDependencies.probe = { _, _ -> H264_SOURCE }
val reporting = ReportingHardwareTranscoder { onProgress -> onProgress(PERCENT) }
ConversionDependencies.hardware = { reporting }
runBlocking { workerReporting(EnginePreference.AUTO) { }.doWork() }
assertEquals("the job must have gone to the hardware engine", 1, reporting.attempts)
val progressUpdates = updater.infos.drop(1)
assertEquals("one throttled progress update expected", 1, progressUpdates.size)
assertEquals(
PERCENT,
progressUpdates.single().notification.extras.getInt(Notification.EXTRA_PROGRESS),
)
}
/**
* A worker routed to the software engine, whose engine is [report] and a written output.
*
@@ -173,10 +137,7 @@ class ProgressNotificationTest {
* bridge, which is native. [report] is handed the worker's own progress callback, and runs with
* the worker as its receiver so a test can stop it mid-transcode.
*/
private fun workerReporting(
enginePreference: EnginePreference = EnginePreference.FORCE_SOFTWARE,
report: ConversionWorker.((Int) -> Unit) -> Unit,
): ConversionWorker {
private fun workerReporting(report: ConversionWorker.((Int) -> Unit) -> Unit): ConversionWorker {
val worker = TestListenableWorkerBuilder<ConversionWorker>(
context = app,
inputData = workDataOf(
@@ -186,7 +147,7 @@ class ProgressNotificationTest {
ConversionWorker.KEY_CONTAINER to SPEC.container.name,
ConversionWorker.KEY_VIDEO_CODEC to SPEC.videoCodec.name,
ConversionWorker.KEY_AUDIO_CODEC to SPEC.audioCodec.name,
ConversionWorker.KEY_ENGINE_PREFERENCE to enginePreference.name,
ConversionWorker.KEY_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name,
),
runAttemptCount = 0,
).setId(JOB_ID)
@@ -210,17 +171,6 @@ class ProgressNotificationTest {
const val TICKS = 50
val SPEC = OutputFormat.MP4_H265.spec
val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000021")
/**
* A probe the router can actually route. `InputProbe()` reports `UNPARSEABLE`, which
* `PERMISSIVE.canDecode` refuses, so every job would reach FFmpeg with no test saying why.
*/
val H264_SOURCE = InputProbe(
videoCodec = "h264",
audioCodec = "aac",
container = Container.MP4,
durationMs = 1_000,
)
}
}
@@ -261,22 +211,3 @@ private class ReportingTranscoder(private val report: ((Int) -> Unit) -> Unit) :
const val OUTPUT_BYTES = 512
}
}
/** A hardware engine that reports whatever [report] wants reported, then writes an output. */
@UnstableApi
private class ReportingHardwareTranscoder(private val report: ((Int) -> Unit) -> Unit) : HardwareTranscoder {
var attempts = 0
override suspend fun transcode(input: Uri, output: File, request: ConversionRequest, onProgress: (Int) -> Unit) {
attempts++
report(onProgress)
output.writeBytes(ByteArray(OUTPUT_BYTES))
}
override fun close() = Unit
private companion object {
const val OUTPUT_BYTES = 16
}
}
@@ -10,9 +10,3 @@
# Set here rather than in a @Config on each class so a later Robolectric test does not have
# to rediscover it. Remove it once Robolectric ships an android-all jar for 37.
sdk=36
# Every test gets TestLibreMediaConverterApp, whose only difference from the real one is that the
# startup sweep runs inline rather than on Dispatchers.IO. Set suite-wide because the race it fixes
# (#159) is suite-wide: any class that builds an Application leaves a sweep of the shared staging
# directory in flight for whatever runs next. TestLibreMediaConverterApp explains the choice.
application=org.libremediaconverter.TestLibreMediaConverterApp
+18 -189
View File
@@ -315,90 +315,25 @@ clean zero. Its own post-disable check on the run recorded below printed
So what is reliably achieved is a **rate collapse** — from roughly one abort every fourteen
seconds to one every forty-five — which a 47-second Gradle run survives and a five-minute one
might not.
might not. The 180-second zero above is one measurement on a device that had been up for twelve
minutes and had already cycled its framework several times. The harness prints the quiet-check
delta on every run precisely so this is visible rather than assumed.
**And that restart has never happened — which is how the disable turned out not to work either.**
Corrected 2026-09-05; this replaces the two paragraphs above rather than qualifying them.
`adb shell stop` and `start` are root-only, adbd is not root on a booted emulator, and all three
copies of this logic called them without `adb root`. On CI both printed `Must be root`, between
lines that read as if the restart had happened; `run-e2e.sh` sent them to `/dev/null`, so its
`Must be root` was never even visible. Neither number in those logs was an observation either —
the `pidof` loop breaks when the process is gone and otherwise falls out at its last iteration,
and the old code printed the iteration count either way, so `system_server down after ~40 s` is
what a stop that did nothing looks like.
Adding `adb root` made the restart real, and **that is what proved the disable ineffective**.
`api37-debug` run 34010167885, `disable_system_ui=true`:
```
--- disable round 1 ---
pm attempt 1: Package com.android.systemui new state: disabled-user
restarting the framework
adbd is running as root
system_server down after 2 s
services back after 10 s
NOT DISABLED after the restart -- the package state did not survive
```
Three rounds of that, then `final state: SystemUI STILL ENABLED`, and the leg reported
`expected: 0, received: 0` — `Starting 0 tests`, the exact failure this function exists to
prevent.
Bisected locally on `android-37.0`, which explains the lost state and nothing else:
| arm | sequence | disabled after the restart? |
|---|---|---|
| A | `pm disable-user`, then `stop` at once | **no** |
| B | `pm disable-user`, wait 15 s, then `stop` | **yes** |
That is PackageManager's delayed write of package restrictions: the stop kills `system_server`
before the settings are flushed, and arm A is what CI did. **Arm B does not help either**, which
is the measurement that matters. With the package verified `disabled-user` before *and* after a
further clean restart:
```
package still disabled? YES
processes:
9275 00:17 system_server
9695 00:14 com.android.systemui <- started 3 s after system_server
```
CI's own logcat says the same without any restart at all. In the gating leg of run 34006456986,
`pm disable-user` is accepted at 02:28:37.9 and the package really is in `pm list packages -d` at
02:29:33 — and SystemUI is started at 02:28:39.5 and again at 02:28:52.3, the second of which
(pid 4275) is alive for the whole instrumentation run.
**So `pm disable-user --user 0 com.android.systemui` does not stop SystemUI starting on this
image**, with or without a framework restart, on CI or locally. The premise this section was
built on — "the framework that comes back never starts SystemUI at all" — is false.
Two things follow, pointing in opposite directions.
- **The restart is removed rather than repaired**, in all three copies. It cost a leg every test
it had and there is nothing for it to buy. What is kept is the 45-second window with zero new
aborts, which was always the part doing the work: in that same run the boot aborts land at
02:28:18 and 02:28:43, and the wait is what puts instrumentation at 02:32:42 — after them
rather than inside one. The `pm disable-user` call is kept too, for a narrower reason than it
was written for: every green leg and every number quoted about this row was measured with it
applied, and changing the configuration while fixing a flake is not a trade worth making.
- **The rate collapse recorded above is not evidence of what it says.** Both arms of that
comparison had SystemUI running. What it measured is a device twelve minutes into its uptime
against one that had just booted — a real difference, and a different claim. The quiet gate is
still worth having on exactly that reading.
One ordering detail cost a whole run and is now encoded in `disable_region_sampling`: by the time
`sys.boot_completed` flips, SystemUI has **already registered**, and `pm disable-user` does not
retract an existing registration — it only stops the package being started again. Disabling it
and proceeding straight to the tests fails exactly as before. The harness therefore does
`stop; start` afterwards, so the framework that comes back never starts SystemUI at all.
### The two deviations, stated plainly
1. **The renderer is ANGLE, not the host GPU.** Shared with nothing else in the matrix — API
33–36 run `-gpu host` locally, and CI runs `swiftshader_indirect`.
2. **SystemUI is asked to be disabled, and runs anyway.** This was written as the deviation that
mattered — "anything that ever does depend on system UI must not trust this leg" — and the
measurements above say the deviation does not exist: the package is marked `disabled-user` and
`com.android.systemui` is up for the whole leg regardless. **The correction is good news
rather than bad.** This row is *more* comparable to API 33–36 and to the Pixel than it has
been claiming, not less, and the test that depends on system UI (see the section below) was
never running in the exotic configuration this bullet describes. What `pm disable-user` leaves
behind is a package-manager flag nothing acts on.
2. **SystemUI is disabled.** The API 37 leg does not run the same device configuration as any
other leg or as the Pixel. It was defensible here because nothing in this suite touched
system UI — Media3, FFmpeg and WorkManager tests — and because the alternative is no local
API 37 coverage at all. **Anything that ever does depend on system UI must not trust this
leg.** Something now does; see the section below.
### Something does depend on system UI now, and half of it is excluded
@@ -406,13 +341,12 @@ Added 2026-08-24, and the first entry on this page that is not a codec.
`SafPickerRoundTripTest` drives the real system file picker and rotates the display. Both reach
the gralloc mapper — DocumentsUI is another app's windows, and a rotation rebuilds every surface
on screen — and **disabling SystemUI does not help**. Two reasons now, and only the first was
known when this was written: it removes the *idle* trigger (RegionSamplingThread's nav-bar luma
sampling) and not this one, and — see the section above — it does not remove SystemUI either.
on screen — and **disabling SystemUI does not help**, because it removes the *idle* trigger
(RegionSamplingThread's nav-bar luma sampling) and not this one.
Measured one method per fresh emulator, `android-37.0`, `swangle_indirect`, with the disable
applied and verified quiet — separately, because inferring the second from the first is the
mistake this page's opening correction is about:
Measured one method per fresh emulator, `android-37.0`, `swangle_indirect`, SystemUI disabled and
verified quiet — separately, because inferring the second from the first is the mistake this
page's opening correction is about:
| test | result on android-37.0 | `hasReadColorBufferDma` aborts in the window |
|---|---|---|
@@ -423,111 +357,6 @@ So a rotation, which rebuilds every surface at once, is what the mapper does not
starting DocumentsUI is not. Only the rotation test carries `@FailsOnEmulatorApi37`; the picker
test runs on the gating leg like anything else.
#### That last sentence was wrong for twelve days, and the aborts in the table said so
**Corrected 2026-09-05.** Read the second row again: the picker test passes *and takes four
`hasReadColorBufferDma` aborts with it*. This section counted them, put them in the table, and then
drew the conclusion from the pass/fail column alone. The right question is not "does the test
pass" but "does the image survive it", and the answer had been printed in the right-hand column
from the day it was written.
Four gating API 37 runs read logcat-first — 34006456986, 34001744574, 34001377499, and the **green**
34002313300 — say it without ambiguity. Each carries exactly two aborts before the suite starts
(both `surfaceflinger`, during boot and the SystemUI disable) and then exactly **one** during it:
| run | picker test window | the run's only in-suite abort | leg |
|---|---|---|---|
| 34006456986 | 02:33:04.2 → 02:34:46.9, **failed** | 02:34:46.845 | red, `failed: 1` |
| 34001744574 | 00:55:41.4 → 00:57:23.9, **failed** | 00:57:23.794 | red, `failed: 1` |
| 34001377499 | 00:35:53.3 → 00:36:00.6, passed | 00:35:59.662 | red, `failed: 0` |
| 34002313300 | 00:58:12.7 → 00:58:19.8, passed | 00:58:19.218 | green |
Every one is `system_server`, thread `TaskSnapshotPer`, and every one lands inside that test's
window. Nothing else in the gating set reached the mapper at all. So the picker test is
**deterministic** in what it does to the image and a coin flip in what the leg reports: 34001377499
passed it and lost the leg from teardown with no failing test to name, and 34002313300 passed it
0.6 s after the abort and went green.
That is #108, which had been filed against this behaviour in August and left open because the
trigger was unknown. The trigger is this test. It now carries `@FailsOnEmulatorApi37` too, and the
marker's KDoc had to widen from "does not pass on this image" to "cannot be run on this image" to
say so honestly.
The stack, for the record, is a different caller from either of the two above:
```
Cmdline: system_server name: TaskSnapshotPer
Abort message: 'Assertion failed: !rcEnc->featureInfo()->hasReadColorBufferDma'
#04 mapper.ranchu.so GoldfishMapper::readFromHost(cb_handle_t const&) const+543
#06 libui.so android::Gralloc5Mapper::lock(...)+63
#10 libandroid_runtime.so android::lockImageFromBuffer(...)+374
#15 framework.jar android.media.ImageReader$SurfaceImage.getPlanes+50
#17 services.jar com.android.server.wm.TaskSnapshotConvertUtil.copyToSwBitmapDirect+56
#28 services.jar com.android.server.wm.SnapshotPersistQueue$StoreWriteQueueItem.writeBuffer+66
#32 services.jar com.android.server.wm.SnapshotPersistQueue$1.run+186
```
WindowManager writing a task snapshot to disk, which needs the buffer as a software bitmap, which
is the non-DMA readback path. `PickActivity` is started **into the app's own task** (`Task #11
A=10234:org.libremediaconverter` in the logcat), so the snapshot being persisted is that task's,
and the churn at the end of the pick is what schedules it.
#### There is no shell knob for task snapshots, and that was checked rather than assumed
#108 asks whether `TaskSnapshotPersister` is suppressible the way the region-sampling listener was.
Probed on a local `android-37.0 google_apis x86_64` AVD, 2026-09-05:
```
getprop | grep -i snapshot # nothing but apexd-snapshotde
settings list global | grep -iE 'snapshot|recents' # empty
device_config list window_manager | grep -i snapshot # empty
cmd window help # no snapshot or screenshot command
dumpsys window | grep -i snapshot # mSnapshotEnabled=true, for Task and Activity
```
`mSnapshotEnabled` is real state and there is nothing that sets it from outside. The only
`device_config` hits anywhere in the tree are aconfig flags — e.g.
`windowing_frontend/com.android.window.flags.respect_requested_task_snapshot_resolution` — which
tune the snapshot rather than disable it. So the marker is the available answer, not the lazy one.
#### When the picker test does fail, the abort is the coda and not the cause
Worth separating, because the failure message points the wrong way. In both runs where the test
itself went red, it had been broken for 98 seconds before the abort landed. The discriminator is
one line, present in both reds and absent from the green:
```
I/InputDispatcher: No new touched window at (539.0, 525.0) in display 0
```
(539, 525) is the centre of the fixture's root row — the same coordinates the green run clicks.
The touch reaches no window and is discarded; `UiObject2.click()` cannot see that and returns
normally. DocumentsUI then logs nothing at all, where the green run logs `DocumentStack` and
`Creating new directory loader` 40 ms after its click. The walk waits out its timeout twice for a
fixture it never navigated to, and by the time the back presses start, WindowManager is still
saying `no window has focus but ...PickActivity may eventually add a window when it finishes
starting up` — for another 63 s. All four presses are dropped, DocumentsUI ANRs on
`Input dispatching timed out`, and only *then* does the abort fire and make the failure message
read `no windows at all`.
`SafPickerRoundTripTest.forceStopThePicker` is the answer to that half: `am force-stop` goes around
input entirely, so the picker's process can be removed from a task no key press can reach and
`pickTheFixture`'s whole-picker retry — which exists for exactly this — becomes reachable again.
That is a fix to the test on every level, not to API 37.
**It was made to bite before it was believed.** On a local API 36 emulator, with the walk cut short
so the picker is left open and in front and with `device.pressBack()` removed, so that nothing but
the force-stop can close it:
| | result |
|---|---|
| with `forceStopThePicker()` | **passes** — `ActivityManager: Force stopping com.google.android.documentsui ... from pid 5334`, `Killing 5269:com.google.android.documentsui (adj 0)`, a second `PickActivity` opens, the retry completes the pick |
| with the one call removed | **fails** — `the system picker would not close: after 4 back presses ... com.google.android.documentsui is in front`, which is the API 37 failure verbatim |
The unmutated class passes on that emulator either way, which is the point of running the mutation
at all: the recovery path is unreachable on a healthy device, so a green suite says nothing about it.
#### The correction that produced that table
**The first version of this section said both tests failed, and put the marker on the class.** The
+17 -233
View File
@@ -1,27 +1,22 @@
# Coverage-read findings
**Status:** ten findings, none fixed, none urgent. F1-F4 came from the 2026-08-26 read; F5 was added
on 2026-08-27 while decomposing #132; **F6-F10 were added on 2026-09-02 from the wave-4 read**. Every
entry here is a *code* observation — something a test would document rather than repair. The test
gaps found in the same reads are tickets, not entries here; see [Not covered here](#not-covered-here).
**Scope:** what a JaCoCo read turned up that writing a test would not fix. This is a survey, not a
work order. Acting on any entry is a separate decision and would be its own commit.
**Last verified:** `main` at `54ca2dd`, 2026-09-02. Coverage measured that day with
`./gradlew :app:jacocoTestReport`: **92.8% line (2183/2352), 81.3% branch (1091/1342)**, against
**584 JVM tests in 87 classes**, matching what `CLAUDE.md` quotes.
The wave-4 read that produced F6-F10 also produced twelve test tickets, **#192-#203**, plus **#204**
for four candidates whose cost was not obviously worth paying. The split between them is the same one
this document has always drawn: a ticket is where a test goes, an entry here is where a test would not
help.
**Status:** five findings, none fixed, none urgent. F5 was added on 2026-08-27, found while decomposing #132 into children — it had been listed there as a test gap, and is not one. Every entry here is a *code* observation —
something a test would document rather than repair. The test gaps found in the same read are
tickets #132 and #133, not entries here; see [Not covered here](#not-covered-here).
**Scope:** what a JaCoCo read on 2026-08-26 turned up that writing a test would not fix. This is
a survey, not a work order. Acting on any entry is a separate decision and would be its own commit.
**Last verified:** `main` at `dc8b7c3`, 2026-08-26. Coverage re-measured that day with
`./gradlew :app:jacocoTestReport`: **84.9% line (1971/2321), 63.8% branch (900/1410)**, against
**456 JVM tests in 68 classes**. `CLAUDE.md` quotes 454 in 67 from four hours earlier; the
percentages are unchanged, so no figure there is stale.
## Why this document is separate from `defect-audit.md`
`defect-audit.md` is the record of the 2026-08-22 defect sweep: sixteen entries, each a thing that
is *wrong at runtime*. Nothing here is wrong at runtime today. These are arms that cannot be
reached, accessors nobody calls, and two KDocs that contradict the code beside them — the category
`defect-audit.md` calls **latent**, plus several that are not defects at all and are recorded so the
next coverage read does not re-file them.
reached, accessors nobody calls, and one KDoc that contradicts the code beside it — the category
`defect-audit.md` calls **latent**, plus one that is not a defect at all and is recorded so the
next coverage read does not re-file it.
They are here rather than in that document because folding them in would inflate a sixteen-entry
audit whose status metadata has already gone stale once, and because they share a provenance:
@@ -29,7 +24,7 @@ every one fell out of reading a coverage report, and every one is the kind of th
report is *good* at surfacing and a test is bad at fixing. F5 is the clearest case — it was filed
as a test gap first, and only stopped being one when someone went looking for its callers.
Entry ids are `F1`–`F10` so they cannot be confused with `defect-audit.md`'s `D1`–`D16`.
Entry ids are `F1`–`F5` so they cannot be confused with `defect-audit.md`'s `D1`–`D16`.
## How to read the confidence labels
@@ -267,179 +262,6 @@ no way to make it happen now.
---
## F6 — Four more arms that cannot be reached, and one KDoc among them that is false
**Severity: low · Confirmed by inspection · F4's family, found in the wave-4 read**
```
app/src/main/java/org/libremediaconverter/model/ConversionRouter.kt:178-179
app/src/main/java/org/libremediaconverter/model/ConversionRouter.kt:221
app/src/main/java/org/libremediaconverter/model/ContainerCapabilities.kt:297
app/src/main/java/org/libremediaconverter/model/ContainerCapabilities.kt:324, :340
```
Four sites that a coverage report flags and that no test can reach. Each is recorded with the
upstream guard that makes it unreachable, because that guard is what would have to change first.
- **`ConversionRouter:178-179`** — the missed branch is `orEmpty()`'s absent-key arm on
`MEDIA3_MUXABLE_VIDEO[plan.container]`. `MEDIA3_CONTAINERS` is `setOf(MP4)` and `route()` returns at
`:104` for anything else, so `media3CanMux` only ever sees MP4, which both maps key. Same function
as F4's second pair, one line below it.
- **`ConversionRouter:221`** — `DeviceCodecs.PERMISSIVE.canDecode` returning **false** for
`InputProbe.UNPARSEABLE`. `PERMISSIVE` has no production caller at all (tests only), and the
router's one `canDecode` call at `:128` is already preceded by `:117` returning FFMPEG for
`UNPARSEABLE`. **Its KDoc at `:214-217` is false as written:**
> That exception matters: a device double that claims it can decode an unparseable file would let
> the router send a doomed job to Media3.
It would not — `:117` already caught it. This is F2's shape: a comment that describes a hazard the
code upstream has removed. Correcting it is a one-line change and should not be bundled with
anything.
- **`ContainerCapabilities:297`** — `if (container == GIF || container == IMAGE_SEQUENCE) return null`
in `repair`. `repair`'s only caller is `suggestions` (`:281`); `validate` returns at `:121` for
`isImageOutput` (which is exactly GIF ∥ IMAGE_SEQUENCE) before `suggestions` is reached, and
`firstContainerHolding` filters on `CARRIES_VIDEO`, which is empty for both.
- **`ContainerCapabilities:324` and `:340`** — the `else ->` arms themselves are exercised; what is
missed is the elvis tail, `firstOrNull() ?: VideoCodec.NONE` / `?: AudioCodec.NONE`. Reaching it
needs a container with no encodable codec on that axis. Audio-only containers return early at
`:307`, and the only containers with an empty audio set are GIF and IMAGE_SEQUENCE, excluded at
`:297` above.
**Recorded so the next read does not re-file them.** F4's rule applies unchanged: a second line of
defence that can be provoked is not a second line of defence, and widening a private function to make
one reachable buys a test that asserts a fallback fires when called in a way production cannot call
it.
---
## F7 — `probeWithExtractor`'s catch is unreachable for the same measured reason `probeForConcat`'s is
**Severity: n/a · No action · completes a measurement already on record**
```
app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt:180-182
```
```kotlin
} catch (e: Exception) {
Log.i(TAG, "Platform extractor could not read $uri.", e)
null
}
```
`CLAUDE.md` records the measurement for the *other* extractor site: Robolectric's `MediaExtractor`
never throws from `setDataSource`, checked across an unregistered `content://` authority, a missing
`file://`, a file of garbage bytes and an `http://` URL — all four returned with `trackCount = 0`.
`probeWithExtractor` calls the same overload, three lines apart in the same file, and the measurement
covers it identically. It was simply not written down for this site, so a future read would re-derive
it. It stays device-only, alongside `probeForConcat`'s.
**Two neighbouring line counts are artifacts of this, not separate gaps.** `MediaProbe:184` and
`:331` each report 27 missed instructions and are the `finally` block's synthetic exception-path copy
— JaCoCo duplicates a `finally` per exit path, and the exceptional one is unreachable for the reason
above. Do not read them as a third and fourth site.
---
## F8 — Three more dead members, and six unused defaults
**Severity: low · Confirmed by inspection · F3's family**
```
app/src/main/java/org/libremediaconverter/model/CopyPlanner.kt:28 ConversionPlan.hasVideo
app/src/main/java/org/libremediaconverter/codec/AndroidDeviceCodecs.kt:39 hardwareEncoders()
app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt:30 Result.output
app/src/main/java/org/libremediaconverter/convert/Transcoders.kt:28, :29, :40, :61
app/src/main/java/org/libremediaconverter/work/Reattachment.kt:28, :30
```
- **`ConversionPlan.hasVideo`** — zero callers in `main`, `test` or `androidTest`. Every `hasVideo`
hit in the tree is `InputProbe.hasVideo`, `OutputSpec.hasVideo` or `Container.extensionFor(hasVideo)`,
which are different properties on different types. A test asserting
`plan.hasVideo == (plan.video != VideoPlan.Drop)` is vacuous by construction.
- **`AndroidDeviceCodecs.hardwareEncoders()`** — its only caller is `RealMediaBenchmark`, in
`androidTest`. Production reads capabilities through `DeviceCodecs`, never the raw set.
- **`ConcatEngine.Result.output`** — `ConcatWorker` reads `result.strategy` and uses the `staged`
file it passed in, never `.output`.
- **`Transcoders.kt`'s default arguments** — `request` and `onProgress` on
`HardwareTranscoder.transcode` (`:28`, `:29`), `onProgress` on `SoftwareTranscoder.run` (`:40`),
and `format` on `ConcatJoiner.join` (`:61`). All three production call sites
(`ConversionWorker.kt:208`, `:234`, `ConcatWorker.kt:79`) pass every argument, so the synthesised
`$default` bridges and `$DefaultImpls` copies are never entered. The
`request: ConversionRequest = ConversionRequest(OutputFormat.MP4_H265.spec)` default is the one
worth a second look: nothing anywhere omits it, so an interface silently promises H.265 to a
caller that does not exist.
- **`JobSnapshot`'s `outputModifiedAt` and `tags` defaults** — `JobSnapshots.kt:32-42` passes all
seven fields, so the synthesised `$default` constructor (20 missed instructions at
`Reattachment.kt:14`) is never entered.
**Not a test gap, for F3's reason.** Delete them, or keep them and know they are unused; either is a
decision, and a test restating the compiler is not.
---
## F9 — Both workers' `getForegroundInfo` overrides are dead, and this is why
**Severity: n/a · No action · sharpens #88 rather than reopening it**
```
app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt:342-346
app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt:132-136
```
**#88 already closed on these**, after reading both and finding no decision worth a seam — the
correct call, and it stands. What #88 did not name is the reason they are cold in the first place,
which is stronger than "the JVM cannot reach them":
WorkManager calls `getForegroundInfoAsync()` **only for expedited work**. `ConversionWorker`'s own
KDoc says expedited is deliberately not used, and `grep -rn 'setExpedited\|OutOfQuotaPolicy' app/src`
returns nothing. So both overrides are dead in production today, not merely untested — a test would
assert the shape of something nothing invokes.
They are still correct to keep: `ForegroundInfo` is required by the `CoroutineWorker` contract and
`setForeground` is called explicitly elsewhere. **What would reopen this** is the same trigger #88
named — a `getForegroundInfo` that starts branching — plus one more: the day anything calls
`setExpedited`.
---
## F10 — Three arms that are reachable, uncovered, and cannot be made to bite
**Severity: n/a · No action · the shape a coverage number cannot distinguish**
```
app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt:550, :553
app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt:349, :352, :278
```
F4 and F6 hold arms that cannot be *reached*. These can — and a test written against them would still
pass under the mutation that ought to redden it, which is the harder case to spot and the more
expensive one to discover halfway through writing the test.
- **`observer?.cancel()`'s non-null arm** (`ConversionViewModel:550`, `JoinViewModel:349`). Reachable
by calling `convert()` twice. But `ScreenOwnership`'s token is what actually blocks the superseded
write — the ViewModel's own KDoc at `reset()` says the cancel is "a request honoured at the next
suspension point" and "the claim is what actually stops that write". Delete `observer?.cancel()`
and the suite stays green, correctly.
- **`if (info == null) return@collect`** (`ConversionViewModel:553`, `JoinViewModel:352`). Reachable
through `pruneWork()`. But when the null arrives the state is already terminal, so removing the
guard crashes the collector and **leaves the state unchanged** — a state assertion is green under
the mutation. The only observable is an escaped coroutine exception, which the ViewModel's own KDoc
documents as unreliable on the JVM: kotlinx-coroutines-test's process-wide collector hands it to
whichever `runTest` starts next.
- **`JoinViewModel:278`'s `Ambiguous` arm.** Looks like the twin of `ReattachGuardsTest`'s "a result
two jobs both claim", and is not. An `Ambiguous` requires a shared `outputPath`, so it can only be a
*finished* job — which maps to `Joined`, a state that reads nothing from `inputs`. **The Convert-side
twin does bite**, because `displayNameOf(tags)` reaches the file card; the asymmetry is the point.
**Recorded because each of these was picked up as a candidate and put down again.** The wave-4 read
lost time to all three before the mutation test was run in the head rather than the editor, which is
the cheaper order.
---
## Summary
| ID | Finding | Severity | Evidence | Action |
@@ -449,23 +271,12 @@ the cheaper order.
| F3 | `ConversionRequest.videoCodec` / `.audioCodec` have no callers | low | confirmed by inspection | delete, or keep for symmetry — **not** a test gap |
| F4 | Two private guards reachable only by direct call | n/a | confirmed by inspection | **no action** — named exemption, per #88 |
| F5 | `ConversionNotifications.areEnabled()` is never called | low | confirmed by inspection; grep returns the declaration only | **decide**: act on it or delete it — **not** a test gap |
| 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 |
| 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
user-visible answer — a format the app can produce and does not offer, and a warning the app
documents and does not give — and either answer changes what the tidying should look like. F2, F3 and
F8 are tidying and belong in one commit with each other, not with F1 or F5. F6's KDoc correction is a
third kind: one line, no decision, and it should not wait on the tidying. F4, F7, F9 and F10 are
finished by being written down.
**Six of the ten are now "no action" or "not a test gap", and that is the useful shape.** By wave 4
the report's remaining red is mostly this: arms nothing can reach, members nothing calls, and arms a
test can reach but not pin. A coverage number cannot tell any of them from a real gap, which is why
this document exists and why it grows faster than the percentage moves.
documents and does not give — and either answer changes what the tidying should look like. F2 and F3
are tidying and belong in one commit with each other, not with F1 or F5. F4 is finished by being
written down.
**F1 and F5 share a shape worth naming:** both are places where a comment describes behaviour the
code does not have, and in both the tempting fix (delete the dead arm, test the dead method) would
@@ -476,15 +287,7 @@ freeze the wrong answer in place. The decision comes first.
**The test gaps from the same read.** Seven JVM-side gaps (**#132**) and three seam questions
(**#133**) came out of this coverage read and are tracked there, because they are work rather than
observations. This document holds only what a test would not fix. #133 also records why
`AndroidDeviceCodecs.probe()` was considered and left out **through `ShadowMediaCodecList`**, so that
spike is not run a third time.
**Updated 2026-09-02:** #194 proposes reaching the same code through a *pure seam* instead, which is a
different mechanism and one #133 did not evaluate — the builder objection it turns on (no
`setIsAlias`, no `setCanonicalName`) does not apply to a function taking its own entry type. #133's
close stands for the shadow; it is not a close on the seam. #194 also carries the reason the seam is
worth cutting at all, which is not coverage: the `runCatching` fallback logs "assuming permissive" and
returns empty sets, which makes `canEncode` and `canDecode` answer *no* for everything.
`AndroidDeviceCodecs.probe()` was considered and left out, so that spike is not run a third time.
**`ConversionForegroundType.current()`**, which looked like the sharpest gap in the read and is not.
Its API 33 and 34 arms are cold on the JVM, but issue **#88** already established that the class is
@@ -518,25 +321,6 @@ the real ones — **34 of 383** and **20 of 143** missed — and the screens are
better-covered files in the repo, which is what #52, #57 and #61 were for. **Do not chase the
branch number here.** If a future read wants a screen metric, use lines.
**Updated 2026-09-02: the same codegen inflates the *instruction* count, which wave 3's filter did
not allow for.** Wave 3 selected candidates on `mi > 0` — at least one missed instruction — which was
right to prefer over a bare branch count and is still wrong on these files. `JoinScreen.kt:222` reads
`mi=10` and looks uncovered; it also reads `ci=38`, and `JoinStateAffordancesTest` already clicks that
Save button and asserts `save:joined.mp4`. Every `onClick` lambda body flagged this way turned out to
be covered at method level, the missed instructions being the recomposition-skip path again.
Use `ci == 0` — the line never executed, which is JaCoCo's own missed-line definition — and pair it
with a method-level `ci > 0 && mb > 0` pass for covered methods with cold arms. Neither filter alone
is enough: `ConversionViewModel.cancel()` misses no line at all, yet its non-null arm had never been
entered in 584 tests (#192). `CLAUDE.md`'s coverage entry carries the same correction.
**Also codegen, also not gaps**, recorded once so they are not re-derived: the synthetic
`NoWhenBranchMatchedException` closing an exhaustive `when` (`ConverterScreen:399`, `:686`,
`JoinScreen:278`, `MainActivity:160`); the inner `is Idle -> Unit` arms at `ConverterScreen:253-254`
and `JoinScreen:158-159`, which are structurally unreachable because the outer `when` already routed
`Idle`; and the closing brace of a `launch` block whose `collect` never terminates
(`ConversionViewModel:578`, `JoinViewModel:371`).
**Anything requiring a device.** `MediaProbe`'s FFprobe half (`MediaProbe.kt:151, 156-158, 173-188`)
and `FFmpegEngine` in full report 0% on the JVM and are covered by `androidTest`. JaCoCo measures
`testDebugUnitTest` only; their zeroes are a boundary, as #84, #85, #86 and #88 each recorded
-517
View File
@@ -1,517 +0,0 @@
# E2E-read findings
**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
claims rather than what it runs.
**Scope:** what reading all 60 instrumented tests turned up that writing a 61st would not fix.
**Last verified:** `main` at `4b02294`, 2026-09-05. **60 `@Test` methods in 12 classes**, three
carrying `@FailsOnEmulatorApi37`, gating API 37 leg 57.
## Why this document exists, and why it is separate from the other two
`docs/coverage-read-findings.md` (`F1`–`F10`) came from reading a **JaCoCo report**, and JaCoCo
measures `testDebugUnitTest` only. So four waves of coverage work have been shaped by a number that
**cannot see `app/src/androidTest` at all**. The instrumented suite has never had the equivalent
read: nothing has asked what those 60 tests actually pin, only that they are green.
That is the gap this read is in. It is a **triage, not a test push** — the same shape as wave 4's
read, which "moved no number at all, and that is its result".
`docs/defect-audit.md` (`D1`–`D16`) is the record of things *wrong at runtime*. Nothing here is
wrong at runtime. These are tests whose names, KDoc or reputation overstate what they execute.
Entry ids are `E1`–`E6` so they cannot be confused with `F1`–`F10` or `D1`–`D16`.
## How to read the confidence labels
Same vocabulary as the other two documents, deliberately:
- **Confirmed by inspection** — the control flow is fully readable and the finding follows from it.
- **Confirmed by measurement** — observed in a CI artifact, with the run id recorded.
- **No action** — recorded because it looks like a finding and is not.
## The method, and the one filter that found everything
A coverage number is useless here by construction, so the read used a different question, applied
to every one of the 60 tests:
> **If the behaviour this test is named for stopped working, would it go red?**
Three answers, and only the third is a gap:
- **yes** — the test bites. Most of the suite.
- **no, and that is deliberate and written down** — `RealMediaBenchmark` asserts nothing on purpose
(E2); `transcodesH264ToH265AndReportsProgress` declines to assert progress for a stated reason
(E3). These are entries here, not tickets.
- **no, and nothing says so** — the gap. One test, and it is the most important one in the suite.
**The reusable part is the second filter**, because "does it assert something?" would have cleared
the vacuous test — it asserts two things. What it does not do is *reach the code it names*:
> **Does the test's own premise hold on the machine that runs it?**
`HardwareFallbackTest` asserts `SUCCEEDED` and a non-empty output, and both are true of a
conversion that never went near the path it exists to prove (**#223**). See
[Not covered here](#not-covered-here); it is filed rather than recorded here because a test fixes it.
---
## E1 — `RemuxTest`'s class KDoc argues for engine assertions three of its tests do not make, and they are right not to
**Severity: low · Confirmed by inspection · the KDoc is what is wrong, not the tests**
```
app/src/androidTest/java/org/libremediaconverter/convert/RemuxTest.kt:31-42
```
The class KDoc is headed **"Why these assert the engine, not just the file"** and makes a specific
argument:
> A remux routed to FFmpeg produces a perfectly correct file — `-c copy` moves the same samples
> into the same container. So an output-only assertion passes whether the hardware transmux path
> ran or never executed at all […] which makes "silently always FFmpeg" the most likely way for
> this feature to regress.
Five of its seven tests run a conversion. **Three assert no engine at all:**
| test | output container | asserts engine? |
|---|---|---|
| `mkvToMp4RemuxesOnHardware` | MP4 | **yes** — `MEDIA3` |
| `mp4ToMkvRemuxesOnFFmpeg` | MKV | **yes** — `FFMPEG` |
| `webmToMkvKeepsVp9WithoutReencoding` | MKV | no |
| `audioOnlySourceRemuxesIntoMka` | MKV (`.mka`) | no |
| `mp4ToMpegTsAndAviProduceTheirOwnContainers` | MPEG-TS, then AVI | **TS only**; the AVI half does not |
### Why this is not a gap
`ConversionRouter.MEDIA3_CONTAINERS = setOf(Container.MP4)` (`ConversionRouter.kt:37`), and every
one of the three produces MKV or AVI. **They can only ever be FFmpeg**, so the regression the KDoc
names — "silently always FFmpeg" — is not a thing that can happen to them. The two tests where the
hardware path is genuinely at risk are exactly the two that assert it.
An engine assertion on the other three would be near-tautological given today's router. It would
catch one thing: somebody adding MKV or AVI to `MEDIA3_CONTAINERS` without a muxer to match — which
is what `Media3MuxersTest` is for, on the JVM, where it does not need a device.
### Why it is recorded rather than dropped
**This was the strongest-looking candidate of the whole read and it dissolved on tracing**, which
is the same shape as `F5` in the coverage document (filed as a test gap, and only stopped being one
when someone went looking for its callers). Recorded so the next read does not re-file it.
**The fix is one line of KDoc**, not three tests: the class asserts the engine *where the engine is
in doubt*, which is a better rule than the one it currently states.
---
## E2 — three of the 60 instrumented tests assert nothing, and two of them never run
**Severity: n/a · No action — deliberate, documented, and load-bearing as documentation**
```
app/src/androidTest/java/org/libremediaconverter/bench/RealMediaBenchmark.kt:25-53
```
`reportDeviceEncoderCapabilities` logs and asserts nothing. `hardwareVersusSoftwareOnRealVideo` and
`av1InputRoutesAccordingToDeviceDecodeSupport` are `assumeTrue`-guarded on media that is **not
committed** and must be staged by hand into the app's internal `filesDir`, so they skip in every
automated run — they are the "2 skipped" every green leg reports, and `docs/local-emulator.md:305`
says so.
The class KDoc is unambiguous: *"This is a benchmark, not part of the automated suite […] Not a
correctness test — the assertions are deliberately loose."*
**No action.** Recorded for one reason: **the suite's headline number is 60, and three of those 60
are not tests.** Any future statement of the form "60 instrumented tests cover X" is off by three,
and two of the three have never executed on CI at all.
**It is the opposite of E-nothing, though** — `reportDeviceEncoderCapabilities` runs on every leg
and logs `BENCH can-encode:`, and **that log line is what confirmed the vacuous test this read
found** (**#223**). An assertion-free test that prints the machine's capabilities turned out to be
the only oracle in the suite. See [Not covered here](#not-covered-here).
---
## E3 — `transcodesH264ToH265AndReportsProgress` does not assert that progress was reported
**Severity: low · No action on the test; the name is the inaccurate part**
```
app/src/androidTest/java/org/libremediaconverter/convert/Media3EngineTest.kt:73, :90-93
```
```kotlin
// Deliberately NOT asserting that progress fired. Polling is on a 250 ms tick,
// and a 3 s 320x240 clip can finish inside one tick on fast hardware, which
// would make the assertion fail intermittently for no real defect.
seen.forEach { assertTrue("progress out of range: $it", it in 0..100) }
```
`seen` is empty-safe: `forEach` on an empty list asserts nothing, so replacing `onProgress` with a
no-op reddens nothing here. The reasoning is sound and the alternative really is a flaky test.
**No action on the body.** The name says `AndReportsProgress` and the body says it does not check
that, which is the `probeForConcat` shape from `CLAUDE.md` — *a passing test with a wrong
explanation is its own failure mode* — in its mildest form, since here the KDoc immediately corrects
the name.
**Contrast the FFmpeg side, which is a real gap and is filed as #229**: `FFmpegEngine`'s percentage
arithmetic is executed by every FFmpeg test and observed by none, because every call site omits
`onProgress` entirely. Media3's is unasserted; FFmpeg's is unobserved. Only the second is a ticket.
---
## E4 — the marker's KDoc says removing it grows the gating leg by two; three tests carry it
**Severity: low · Confirmed by inspection · one line**
```
app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt:20
```
> Delete the annotation from the tests, and the advisory job goes empty and the gating one grows by
> **two**.
Three tests carry it — `Media3EngineTest:72`, `Media3EngineTest:135`, `SafPickerRoundTripTest:320` —
and `FAILS_ON_EMULATOR_API37_BASELINE = 3` eleven lines further down the same file, where the count
is machine-checked by `.github/scripts/e2e-report-shape.sh`.
The third marker was added when the SAF rotation test was excluded; the sentence was not updated
with it. **Everything that is checked is consistent at three**; only the prose says two, which is
exactly why it drifted — and a good argument for the baseline const being a const.
---
## E5 — `coverage-read-findings.md`'s F7 calls covered code uncovered
**Severity: low · Confirmed by inspection · half of F7 is stale**
F7 says `probeWithExtractor`'s catch (`MediaProbe.kt:180-182`) is unreachable on Robolectric and
"stays device-only", measured across four URI shapes. **The unreachability claim is correct and
stands.** The implication readers take from it — that nothing exercises it — does not:
```
app/src/androidTest/java/org/libremediaconverter/convert/RemuxTest.kt:111
```
`probeDistinguishesAudioFromImagesFromRubbish` feeds it a file of random bytes and asserts
`InputKind.UNPARSEABLE`, on a device, on every gating leg.
**"Device-only" holds; "uncovered" does not** — and the difference matters, because F7 is one of the
six entries that document calls "no action", on the grounds that a test would not help. A test
already exists. The entry should say so.
**This is the failure mode the split between the two documents was meant to prevent**, and it caught
this repo out: a JaCoCo-derived document cannot see `androidTest`, so it will keep re-deriving
"uncovered" for anything the instrumented suite covers. That is a structural reason for this
document to exist, not a one-off correction.
---
## E6 — the suite's one device-capability assertion derives its expectation from the call it is testing
**Severity: low · Confirmed by inspection · no independent oracle exists**
```
app/src/androidTest/java/org/libremediaconverter/work/ConversionWorkerTest.kt:151-152
```
```kotlin
val hasHardwareHevc = AndroidDeviceCodecs.get().canEncode(VideoCodec.H265)
```
and then the expectation is `if (hasHardwareHevc) MEDIA3 else FFMPEG`. The test asks
`AndroidDeviceCodecs` what to expect and then checks that the router agreed with
`AndroidDeviceCodecs`. **If the whole enumeration returned empty, this would still pass** — and
empty is precisely what the `runCatching` fallback returns (the reason `#194` was worth cutting;
it logs "assuming permissive" while making `canEncode` answer *no* for everything).
Its KDoc defends the choice, and the defence is good:
> Asserting MEDIA3 unconditionally tests the test machine, not the router.
That is true, and there is no third source of truth on a device: `MediaCodecList` is what
`AndroidDeviceCodecs` reads, so any oracle built from it is the same oracle.
**No action, but read it with #223.** It is the same missing oracle that makes the
vacuous-test fix a judgement call rather than a one-liner — you cannot assert "this device has
hardware HEVC" from inside the suite without asking the class under test. The honest options are a
visible skip or a red test, and that decision is the ticket's.
---
## E7 — a real `DocumentsProvider` cannot be reached without the picker, so there is no cheap SAF test
**Severity: n/a · Confirmed by measurement · this is a platform rule, not a gap**
Added 2026-09-06, from doing #225 and #226 rather than from reading.
`OutputPublisher.publish`'s destination side is asserted only against Robolectric fakes —
`FakeSafProvider`, registered with `asDocumentsProvider = true`, which is the flag that *makes*
`DocumentsContract.isDocumentUri` answer true. #226 split that into a cheap headless half (drive a
real `DocumentsProvider` directly) and an expensive picker-driven half.
**The cheap half does not exist.** Three approaches, all measured on an API 34 emulator:
| approach | result |
|---|---|
| a second `DOCUMENTS_PROVIDER` declared **without** `MANAGE_DOCUMENTS` | refused at install: `SecurityException: Provider must be protected by MANAGE_DOCUMENTS` |
| create the document as the **test APK**, which owns the provider | denied — instrumentation runs *in the target app's process*, so it carries the app's uid whatever `Context` is asked |
| `uiAutomation.adoptShellPermissionIdentity(MANAGE_DOCUMENTS)` | denied identically |
The denial names the only way in:
> `Permission Denial: opening provider …FixtureDocumentsProvider from
> ProcessRecord{… org.libremediaconverter/u0a192} requires that you obtain access using
> ACTION_OPEN_DOCUMENT or related APIs`
And the intent filter is not optional: without it `isDocumentUri` returns false, which is exactly
the branch guarding `deletePartialOutput` — so a provider without the filter tests nothing the
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
documents provider. It opens a descriptor through the resolver, so **any readable `content://` URI
exercises it**, and an ordinary `ContentProvider` may be exported without a permission. That is what
`FixtureContentProvider` is, and it made #225 headless.
**That distinction was worth the trouble**: the first test ever to hand the join path a real
`content://` input found #238, a defect that broke joining for every user who picks matched files.
The expensive gate protects the *destination* side; the *input* side never needed it.
## Summary
| ID | Finding | Severity | Evidence | Action |
|---|---|---|---|---|
| E1 | `RemuxTest`'s KDoc claims engine assertions three of its tests correctly omit | low | confirmed by inspection; traced through `MEDIA3_CONTAINERS` | **one line of KDoc** — the tests are right |
| E2 | Three of the 60 instrumented tests assert nothing; two never run | n/a | confirmed by inspection; `docs/local-emulator.md:305` | **no action** — deliberate; but 60 ≠ 60 |
| E3 | `…AndReportsProgress` does not assert progress fired | low | confirmed by inspection; reason inline | **no action** — the name overstates, the KDoc corrects it |
| E4 | The API 37 marker's KDoc says "two"; three tests carry it | low | confirmed by inspection; baseline const says 3 | **fixed** in #243 — it names the constant now |
| E5 | `coverage-read-findings.md` F7's "uncovered" half is stale | low | confirmed by inspection; `RemuxTest.kt:111` drives it | **amend F7** — "device-only" stands, "uncovered" does not |
| E6 | The device-capability assertion asks the class under test what to expect | low | confirmed by inspection; no third oracle exists on a device | **no action** — read with **#223** |
| E7 | A real `DocumentsProvider` is unreachable without the picker, so #226 has no cheap half | n/a | measured three ways on API 34; each denial names `ACTION_OPEN_DOCUMENT` | **no action** — it re-scoped #226 |
**Six of the seven are prose, not code**, and that is the shape of this read. The instrumented suite
is in good condition: 57 of its 60 tests bite, the fixtures are committed with their generation
recipes, and the one class that asserts nothing says so in its first line. What this read found is
that **the suite's self-description has drifted from the suite** in five small places and one large
one.
**The large one is not in this table**, because a test fixes it: **#223**.
## Not covered here
**The vacuous test.** `HardwareFallbackTest.aFileMedia3CannotDecodeStillConvertsViaFfmpeg` passes on
every CI leg without ever entering the fallback it exists to prove. It is **#223**, not an entry
here, because a test fixes it — and it is the reason this read happened rather than an aside from it.
Measured, not inferred, on run **`34004304566`** (all legs green), from each leg's own
`e2e-diagnostics-api*` logcat:
```
I/AndroidDeviceCodecs: Hardware video encoders: []
I/RealMediaBenchmark: BENCH can-encode: COPY=true, H264=false, H265=false, VP9=false, VP8=false, AV1=false
I/ConversionWorker: Routing sample_h264_444.mp4 -> OutputSpec(container=MP4, videoCodec=H265,
audioCodec=AAC) via FFMPEG (NO_HARDWARE_ENCODER)
```
Identical on **API 33, 34, 35 and 37**. (API 36's logcat artifact on that run is truncated to 838 KB
and carries no test output at all, so it is unread rather than different.) The job is routed
**straight to FFmpeg before Media3 is attempted**, the `catch` in `runMedia3OrFallBack` is never
entered, and the test's two assertions — `SUCCEEDED`, output non-empty — are true anyway. It ran in
448 ms.
**The repository already knew.** `ForcedFailureTest.hardwareFailureFallsBackToSoftware`, in the same
package, pins `ConversionDependencies.deviceCodecs = { DeviceCodecs.PERMISSIVE }` and says why:
> most emulators expose no hardware video encoder at all -- so the router would legitimately send
> the job straight to FFmpeg and the hardware path would never be attempted. Without this the test
> passes on a Pixel and fails on every emulator, which says nothing about the code under test.
`ConversionWorkerTest.routesAFastMp4JobByDeviceCapability` records the same fact a third time. The
knowledge is in two sibling files; `HardwareFallbackTest` is the one that walked into it — and
because its assertions are about the *output* rather than the *path*, it passes where
`ForcedFailureTest` would have failed. **That asymmetry is why nobody noticed.**
**State it precisely.** The fallback *wiring* is covered on every leg by `ForcedFailureTest`, with
fakes. What has never run on any emulator is a fallback triggered by a **real** mid-export codec
failure — which is the case `HardwareFallbackTest` exists for, and the only reason
`sample_h264_444.mp4` is committed at all. That fixture, generated with x264 because Fedora's
ffmpeg ships openh264 and cannot produce High 4:4:4, does nothing on any CI leg today.
The fix is not one assertion. `KEY_ENGINE_USED` is `FFMPEG` **whether the fallback fired or the
router went straight there** — asserting it changes nothing. The vacuity guard is two facts
together: the router chose `MEDIA3` for this request on this device, *and* the worker reported
`FFMPEG`. Whether to reach that with `assumeTrue` (a visible skip on emulators, and the "2 skipped"
becomes 3) or with an assertion (red on emulators, announcing it cannot test what it claims) is a
decision, not a detail — see **E6** for why no third option exists — and **#223** leaves it open.
**The other e2e gaps this read found are tickets too**, and are not repeated here:
| # | Gap |
|---|---|
| # | Gap | Outcome |
|---|---|---|
| **#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` | 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 |
| **#230** | *(spike)* whether a running conversion's process can be killed | closed — it cannot; the runner shares the app's process |
**The read's own result, once the tickets were worked: one production defect.** #238 — joining files
picked through the system picker failed outright on the stream-copy path, because the concat demuxer
whitelists protocols separately from `-safe 0` and `ffkitsaf` was not on the list. Only `STREAM_COPY`
feeds the demuxer a list file, and every existing join test passed `Uri.fromFile`, so the one broken
combination was the only one a user could reach.
That is the argument for this kind of read in one line: the gap was not a missed line or an
unasserted value, it was **a combination of two covered things that no test put together**.
**Nothing here was filed as a coverage delta.** Each names the mutation that has to go red, which is
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** |
| 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.
-12
View File
@@ -312,18 +312,6 @@ on sample media that is deliberately not committed. Its third test,
`reportDeviceEncoderCapabilities`, has no such guard and runs. A level reporting 0 skipped
would mean someone had staged sample files, not that something improved.
**Since #223 there is a third, and it is the interesting one.**
`HardwareFallbackTest.aFileMedia3CannotDecodeStillConvertsViaFfmpeg` is `assumeTrue`-guarded on
`AndroidDeviceCodecs.get().canEncode(H265)`, which is false on every emulator image — so it now
skips here and runs only on the Pixel. It used to *pass* on emulators without ever attempting the
hardware path, which is worse. **Expect `skipped="3"` locally**, and note the guard is a property
of the machine rather than of staged files: a level reporting 2 would mean an emulator image had
gained a hardware HEVC encoder, which is worth knowing.
That test's KDoc carries the measurement, including the part that decides it: forcing the route to
Media3 anyway does *not* produce a fallback, because the goldfish decoder decodes the High 4:4:4
fixture despite declaring `NoSupport` for its profile.
### What the sweep adds, and what it does not
**The renderer rule held four more times.** No boot log contains the string
+14 -16
View File
@@ -355,10 +355,12 @@ boot_emulator() {
# may be in one of its restarts and `pm` is simply not published yet. The first attempt at this
# failed exactly that way, with `cmd: Can't find service: package`.
#
# This used to end with a framework restart, described here as "not optional". It was neither
# optional nor happening -- see the block inside the function. What the first attempt's
# `Starting 0 tests` and four more aborts actually showed is that a `pm disable-user` on its own
# buys nothing, which is still true; what was wrong is the conclusion that a restart would.
# The framework restart at the end is not optional, and finding that out cost a run. By the
# time `sys.boot_completed` flips, SystemUI has already registered its region-sampling listener,
# and `pm disable-user` does not retract a registration that already happened -- it only stops
# the package being started again. So the first attempt disabled SystemUI, reported success, and
# then died exactly as before with `Starting 0 tests` and four more aborts. `stop; start` cycles
# zygote deliberately, and the framework that comes back up does not start SystemUI at all.
disable_region_sampling() {
local api="$1" out i before after ready
case "$api" in 37 | 37.*) ;; *) return 0 ;; esac
@@ -381,18 +383,14 @@ disable_region_sampling() {
return 0
fi
# NO FRAMEWORK RESTART, and the two lines that used to be here are why this comment is long.
# They were `emu_adb shell stop` and `emu_adb shell start`, both redirected to /dev/null, and
# both root-only -- so what they printed there was `Must be root` and what they did was nothing,
# here and in the two CI copies alike. Making them real (2026-09-05) is what established that
# the disable never worked in the first place: with the package verified `disabled-user` before
# AND after a clean restart on android-37.0, `com.android.systemui` comes up 3 s after
# `system_server` regardless, and the same is visible in CI's own logcat. The restart also loses
# the package state to PackageManager's delayed write if it lands too soon after the `pm` call,
# which cost api37-debug run 34010167885 every test in the leg.
#
# So the useful part of this function is the quiet window below, not the disable. See
# .github/scripts/e2e-run.sh's header, and docs/api-37-emulator-crash.md.
echo " restarting the framework so the region-sampling listener goes with it"
emu_adb shell stop > /dev/null 2>&1
emu_adb shell start > /dev/null 2>&1
# There is no property worth waiting on here, and an earlier version of this only looked
# like it was waiting on one: `stop` does not clear sys.boot_completed, so it still reads
# `1` throughout the restart and any loop over it returns at once. The loop below is the
# wait -- and it polls the better thing anyway, since `Can't find service: package` is the
# failure it exists to prevent.
ready=0
for i in $(seq 1 30); do
if emu_adb shell service check package 2> /dev/null | grep -q ': found' \