Compare commits

...
Author SHA1 Message Date
JMR-devandClaude Opus 5 31f249ae04 Cancel a running Media3 export, completing #224's third engine
The two FFmpeg engines were done in ad2a75d and d293646. This is
Media3Engine.transcode's invokeOnCancellation, which posts
transformer.cancel() onto the engine's own HandlerThread because cancel()
has the same single-thread requirement as start().

The assertion is the output file here, where it could not be for FFmpeg.
That side deletes the partial on cancellation, and on POSIX ffmpeg keeps
writing to the unlinked inode, so the path stays gone whether or not the
cancel landed -- it asserts the session's return code instead. Media3Engine
deletes nothing, the partial being ConversionWorker's to clean up, so the
file is the evidence.

A cancelled export reports itself two ways and both mean interrupted: no
video track, or MediaExtractor refusing the file outright with "Failed to
instantiate extractor" because there is no moov atom. The first version
treated only the null as success and the exception failed the test, which
is how that was measured. Only a playable file counts as a miss.

The wait before reading is several times the export's own length, so a
cancel that did not land has certainly finished by then: the failure
direction is "the file became playable", never "we did not wait long
enough". The attempt is retried for the reason the other two engines
measured -- a 3 s 320x240 export outruns a naive cancel on a loaded runner
-- and an export that never wrote a file at all is recorded as
inconclusive rather than allowed to pass as a cancellation.

It carries @FailsOnEmulatorApi37, so FAILS_ON_EMULATOR_API37_BASELINE moves
3 -> 4 in this diff. That file also said removing the marker would grow the
gating leg "by two", which has been wrong since the third marker landed; it
now names the constant instead of restating it.

Verified on a local API 34 emulator: 68 tests, 0 failures, 3 skipped; and
with transformer.cancel() removed all five attempts produce a playable
video/hevc and the test fails, naming each one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-06 02:28:18 -05:00
Jason Ross d6e1e3bf86 Merge pull request #242 from JMR-dev/test/reattach-to-a-running-job
Reattach to a conversion that is still running
2026-09-06 02:17:06 -05:00
JMR-devandClaude Opus 5 6992f0e783 Reattach to a conversion that is still running (#230)
Reattachment.rank gives RUNNING the highest rank of all -- "live work
outranks a finished result because a running job is holding a foreground
service" -- and no test on either source set had ever produced one.
ReattachOnLaunchTest covers a job that finished, one whose staged file is
gone, an ambiguous pair, one still queued, and one the user cancelled.
ReattachmentTest exercises the ranking as a pure function over fabricated
snapshots. What was missing is a ViewModel meeting a real running job,
which is also the likeliest reattachment there is: the user starts a
conversion, leaves, and comes back while it is still going.

The engine is a fake, deliberately. The job has to still be running when
the ViewModel is built, and every real conversion in this suite finishes in
about a second -- racing that is what made the cancellation tests flaky
enough to need retries. A SoftwareTranscoder that blocks until released
removes the race outright. Nothing about reattachment depends on which
engine is transcoding: the tag query, Reattachment.choose over live
WorkManager state, and observe's mapping to Converting all run identically
whatever is doing the work.

This is what #230 can actually deliver, and the ticket asked for the answer
either way. Process death itself stays device-manual. D3/D13 already record
that am kill refuses a process holding a foreground service, and there is a
more basic obstacle underneath it: instrumentation runs in the app's own
process, so any route that really killed it would take the test runner with
it and leave nothing to assert with. Observing a relaunch needs two
instrumentation runs, which the runner does not provide. So the closest
observable analogue is a fresh ViewModel, with no memory of the work,
meeting a job that is genuinely mid-flight.

The teardown now resets ConversionDependencies. The suite runs without
Android Test Orchestrator, so a BlockingTranscoder left in place would hang
the next class that converts anything.

Verified on a local API 34 emulator: 67 tests, 0 failures, 3 skipped; and
making RUNNING unreattachable in Reattachment.rank fails this test and
nothing else -- which is also the evidence that the JVM ranking test was
not already covering it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-06 01:58:24 -05:00
Jason Ross bb920b5bd0 Merge pull request #239 from JMR-dev/test/content-uri-reaches-ffmpeg
Let a join read the files the user actually picked
2026-09-06 01:51:10 -05:00
JMR-devandClaude Opus 5 802997439d Let a join read the files the user actually picked (#238, #225)
Joining files picked through the system picker failed outright whenever
the strategy was stream copy -- the matched-files case the UI advertises
as "joined without re-encoding, no quality loss".

  [ffkitsaf @ ...] Protocol 'ffkitsaf' not on whitelist 'file,crypto,data'!
  Error opening input file .../joined_from_content.concat_list.txt

JoinScreen picks with OpenMultipleDocuments, so real inputs are always
content://. ConcatEngine maps each through getSafParameterForRead and
FFmpegConcatCommand writes the resulting ffkitsaf: paths into the concat
list file. The demuxer applies its own protocol whitelist, defaulting to
file,crypto,data, and -safe 0 does not touch it: that permits absolute
paths, this permits the scheme they carry. Two separate gates, and only
one was open.

Nothing caught it because the two halves of the bug never met. Only
STREAM_COPY feeds the demuxer a list file -- REENCODE passes each input
with its own -i, where the whitelist does not apply -- so joining over SAF
worked for mismatched clips. And every join test passed Uri.fromFile,
which takes ConcatEngine's uri.path arm instead of the bridge, so
matchingClipsAreJoinedByStreamCopy exercised stream copy with a file:
path and passed. The one broken combination was the one no test produced
and the only one a user can reach.

That is #225's gap: FFmpegKitConfig.getSafParameterForRead is on every
real conversion and join, and was on no passing test -- only on
UnopenableUriTest's failure side, which proves the error message rather
than the bridge. ContentUriInputTest now drives both the convert and join
paths from a real content:// URI.

It uses a plain ContentProvider, because the documents provider cannot be
reached. Measured three ways: a DOCUMENTS_PROVIDER without MANAGE_DOCUMENTS
is refused at install, 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 naming ACTION_OPEN_DOCUMENT as the only way in. The bridge needs no
documents provider: it opens a descriptor through the resolver, so any
readable content:// URI exercises it, and an ordinary provider may be
exported unprotected. The whole class stays headless. Recorded on #226,
which that also settles: its cheap half does not exist.

Verified on a local API 34 emulator: 66 tests, 0 failures, 3 skipped; and
removing the -protocol_whitelist pair reproduces the production failure
verbatim in the join test and nothing else. FFmpegConcatCommandTest pins
the flag on the JVM.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-06 01:42:03 -05:00
Jason Ross 495eaa4ab8 Merge pull request #241 from JMR-dev/fix/launcher-wiring-waits-for-the-pick
Wait for the pick the launcher test is about
2026-09-06 01:41:55 -05:00
JMR-devandClaude Opus 5 bc8e67888e Wait for the pick the launcher test is about (#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
Compose's idling knows nothing about. So deliver() returned with the state
still Idle, 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 and could not reach it. Two of those failures came
alongside #125's deadlock and could be argued as fallout; three did not.

waitUntil polls through waitForIdle, draining the main looper each time, so
it sees the recomposition 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 is one layer below the launcher
edge this class exists to cover, and reaching for it would mean not testing
that edge.

The evidence is the mutation rather than the repetition count, per the note
#218 left: transposing the two launcher callbacks at ConverterScreen.kt:70
and :83 -- the exact defect this test guards -- still fails it, so the wait
did not make it vacuous. A transposed callback leaves the screen in Idle
forever and it fails on the timeout with the meaning it had before.
Supporting evidence, eight consecutive green runs of the class.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-06 01:34:17 -05:00
Jason Ross 706eea8709 Merge pull request #240 from JMR-dev/fix/cancel-tests-need-a-slower-encode
Stop the cancel tests losing their race on a loaded runner
2026-09-06 01:20:57 -05:00
JMR-devandClaude Opus 5 163ce54b77 Stop the cancel tests losing their race on a loaded runner
Both cancellation tests I added in ad2a75d and d293646 wait for
SessionState.RUNNING and then cancel. That is not enough. The conversion
one passed four consecutive local runs and all five CI legs, then failed
the API 34 and 35 legs of the next PR with state=COMPLETED rc=0, on a diff
that could not reach it. 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, because neither is sufficient alone.

A slower encode. The conversion test now targets WEBM_VP9 at BEST, the
slowest thing FFmpegCommandBuilder emits -- libvpx-vp9 -crf 31 -b:v 0,
with -deadline realtime added only on 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.

A bounded retry. An attempt whose session finished before the cancel
landed has not tested anything, so it is a miss rather than a failure and
is retried; only exhausting five attempts fails, and the message reports
every attempt's state and return code so a real breakage is distinguishable
from a slow machine.

The retry does not soften the test. With FFmpegKit.cancel removed from
both engines, every attempt ends COMPLETED, so both still fail -- verified,
each listing five [state=COMPLETED rc=0] outcomes. Two clean runs
beforehand at 64/0/0/3.

The join test gets the same treatment. It has not flaked yet, but it is
the same mechanism and the same fragility, and finding out on CI again is
not worth the round trip.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-06 01:04:03 -05:00
Jason Ross d293646f69 Merge pull request #237 from JMR-dev/test/cancelling-a-running-join
Cancel a running join session too
2026-09-06 00:11:33 -05:00
JMR-devandClaude Opus 5 5416788274 Cancel a running join session too (#224)
The FFmpegEngine half landed in ad2a75d; this is the same gap in
ConcatEngine. Between them, a real native session being asked to stop is
now covered on both FFmpeg paths.

The assertion is the session's return code again, and here that is not
merely the better choice but close to the only one: ConcatEngine does not
delete its output on cancellation at all. Its invokeOnCancellation is
FFmpegKit.cancel and nothing else, where FFmpegEngine's also deletes the
partial. Whether that asymmetry is deliberate is a separate question, so
this asserts what is true of both engines rather than depending on it.

The cancel triggers on SessionState.RUNNING rather than on progress.
ConcatWorker publishes no progress at all, so there is no callback to hang
it on even in principle -- and the conversion side already measured the
deeper reason, that the committed clips outrun a callback-triggered cancel.

The inputs are the mismatched pair on purpose, so ConcatPlanner chooses
REENCODE. A stream copy of two 2 s clips is close to instantaneous and
would leave nothing to interrupt; re-encoding is also the case where a user
would actually reach for Cancel.

Verified on a local API 34 emulator: three runs green at 64/0/0/3, and
replacing invokeOnCancellation's body with an empty block fails this test
and nothing else, with state=COMPLETED rc=0.

Media3Engine's transformer.cancel() is still uncovered and #224 stays open
for it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-06 00:02:52 -05:00
Jason Ross ad2a75d9a0 Merge pull request #236 from JMR-dev/test/cancelling-a-running-session
Cancel a running FFmpeg session, which nothing had ever done
2026-09-05 23:53:13 -05:00
Jason Ross 2b921fafe4 Merge pull request #235 from JMR-dev/test/notification-cancel-action
Press the Cancel button in the notification
2026-09-05 23:45:53 -05:00
JMR-devandClaude Opus 5 98c0e4dba2 Cancel a running FFmpeg session, which nothing had ever done (#224)
Every cancel in app/src/androidTest is WorkManager.cancelWorkById against
work that is queued or already finished: ReattachOnLaunchTest cancels a job
carrying a one-hour initial delay, and another immediately after enqueue.
On the JVM, WorkerCancellationTest and HardwareFallbackTest's cancellation
case drive a SoftwareTranscoder double that records the call. No test on
any source set had asked a real native session to stop. That 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.

Two things were measured rather than assumed, and both changed the test.

The output file cannot be the assertion. invokeOnCancellation deletes the
path, and on POSIX unlinking a file ffmpeg still holds open leaves ffmpeg
writing to the unlinked inode -- so the path stays gone whether or not the
cancel reached the session, and removing FFmpegKit.cancel passes that check
every time. The session's own verdict is what separates them: a cancelled
session ends with the cancel return code, a completed one does not.

Cancelling from the first progress callback loses the race. It was tried
first and failed with state=COMPLETED rc=0: every committed fixture is
2-3 s at 320x240, and the encode finishes before the first statistics
callback is delivered and acted on. FFmpegKit.listSessions shows the
session RUNNING far earlier, so that is what the test waits for.
QualityTier.BEST is deliberate for the same reason -- preset medium leaves
more of the encode ahead of the cancel.

Verified on a local API 34 emulator. Four consecutive runs green at
62/0/0/3, and removing FFmpegKit.cancel while keeping output.delete fails
this test and nothing else, with state=COMPLETED rc=1.

ConcatEngine and Media3Engine carry the same shape and are not covered
here; #224 stays open for them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-05 23:45:34 -05:00
Jason Ross cf540f1ecc Merge pull request #234 from JMR-dev/test/ffmpeg-progress-is-observed
Read the progress percentage FFmpeg has always been computing
2026-09-05 23:21:54 -05:00
JMR-devandClaude Opus 5 e0412329ff Press the Cancel button in the notification (#227)
ConversionNotifications.build attaches one action, wired to
WorkManager.createCancelPendingIntent(id). Until now
createCancelPendingIntent had no references anywhere outside its own
declaration -- no JVM test, no instrumented test.

That is worth 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.

The obvious version of this test reads NotificationManager's active
notifications for id 1001 and taps what it finds. 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 and where it is read from is
incidental, so this builds the notification for a real live work id and
fires its action -- a real dispatch reaching real WorkManager, the same way
on every API level.

The job carries an initial delay so it stays ENQUEUED. A conversion of the
3 s fixture finishes in well under a second on an emulator, so racing a
cancel against a running job would be flaky in the direction that fails,
and cancelWorkById acts on ENQUEUED identically. What is under test is
whether firing the action reaches WorkManager with the right id.

Verified both ways on a local API 34 emulator: 61 tests, 0 failures, 3
skipped as written; and building the PendingIntent from a random UUID
instead of the request's id fails the new test and nothing else.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-05 23:20:49 -05:00
JMR-devandClaude Opus 5 948d53b67e Read the progress percentage FFmpeg has always been computing (#229)
FFmpegEngine derives progress as stats.time / durationMs * 100, and the
statistics callback runs on every conversion in FFmpegEngineTest -- they
all pass durationMs = 3_000. But every call site omits onProgress, so
nothing on any source set had ever looked at the number. Replacing percent
with a constant reddened nothing.

What already existed covers the plumbing downstream and not this: #196
covered the worker's progress lambda with a fake engine that reports
whatever the test tells it to, and ProgressNotificationTest covers the
throttling the same way. The arithmetic was the one part with no reader.

The new test passes 30 s as the duration for a fixture that is exactly
3.000 s, so the conversion still encodes the whole clip and the reported
percentage tops out around 10 rather than 100.

That is what makes it bite. A range check alone is worthless: a constant 0
satisfies both "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 sits 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 loose -- 5..25 for an expected 10 -- because the
last statistics callback can land slightly before the final frame.

Verified on a local API 34 emulator: 61 tests, 0 failures, 3 skipped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-05 23:13:36 -05:00
Jason Ross 54932e97c6 Merge pull request #232 from JMR-dev/test/fallback-asserts-the-path
Make the fallback test say when it cannot test the fallback
2026-09-05 23:06:41 -05:00
JMR-devandClaude Opus 5 ba16f5a89b Make the fallback test say when it cannot test the fallback (#223)
HardwareFallbackTest is the only automated check of the hardware->software
fallback against a real codec failure, and it passed on every CI leg
without ever attempting the hardware path.

Measured on run 34004304566: the API 33, 34, 35 and 37 legs each log

  Routing sample_h264_444.mp4 -> ... via FFMPEG (NO_HARDWARE_ENCODER)

Emulators expose no hardware encoder, so the router never chooses Media3
and runMedia3OrFallBack's catch is never entered. The test's assertions --
succeeded, output non-empty -- are true of that conversion too. It
finished in 448 ms, which is not long enough to fail an export and then
software-encode a three-second clip. Deleting the catch reddened nothing.

The ticket offered two fixes and left the choice open. Trying the first
one answered it, and not the way the ticket expected. Pinning
deviceCodecs to PERMISSIVE, as ForcedFailureTest does, makes the router
choose Media3 -- and the export then SUCCEEDS. 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 High
4:4:4 fixture regardless of the profile it declares. c2.android.hevc.encoder
then encodes it and the job reports MEDIA3.

So the class KDoc's "Media3 fails partway through the export on every
device" is not true of the emulator images, and no routing pressure makes
this fixture force a fallback there. Pinning would also swap in software
codecs, which is not the path a real device takes -- it is what made the
forced run succeed.

That leaves assumeTrue on the production premise as the honest answer, now
with a measurement behind it rather than a coin flip. The test skips where
it cannot mean anything and runs on the Pixel, where it always could.

When it does run the assertion is a pair, because KEY_ENGINE_USED is
FFMPEG whether the fallback fired or the router went straight there:
the router chose MEDIA3 for this request on this device, AND the worker
reported FFMPEG. Together, and only together, that is the fallback.

Verified on a local API 34 emulator: the test reports SKIPPED and the
level reports skipped=3. ForcedFailureTest still covers the fallback
wiring on every leg with a double; what needs a real encoder is two real
engines disagreeing about a real file.

The third permanent skip is recorded in docs/local-emulator.md and beside
SafPickerRoundTripTest's run-shape note.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-05 22:56:18 -05:00
Jason Ross bffcff92c7 Merge pull request #233 from JMR-dev/test/flac-and-opus-assert-their-format
Check that FLAC and Opus are FLAC and Opus
2026-09-05 22:45:47 -05:00
JMR-devandClaude Opus 5 9f06eb9988 Check that FLAC and Opus are FLAC and Opus (#228)
encodesFlacLosslessAudio and encodesOpus asserted only that a non-empty
file appeared. Five siblings in the same class check what is in it --
encodesWav reads RIFF two lines away, and GIF, Matroska, MP3, H.264 and
H.265 all assert a container marker or a track MIME.

convert() throws on a non-zero return code, so these two did prove the
command ran. What they could not distinguish is the command running and
producing the wrong thing, which is a failure mode this codebase has
already had once: F1 in docs/coverage-read-findings.md records a live
Vorbis arm in FFmpegCommandBuilder that ContainerCapabilities says cannot
exist. Pointing OutputFormat.FLAC's arm at pcm_s16le left both tests green.

Four bytes each, in the idiom the class already uses. OutputFormat.FLAC is
Container.FLAC, whose ffmpeg format is "flac", so the file opens with the
native stream marker fLaC. OutputFormat.OPUS is Container.OGG -- "ogg" --
so it is an Ogg stream and opens with OggS. Verified against both muxers
directly rather than assumed from the codec name; the marker belongs to the
container, so it does not vary with the ffmpeg build.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-05 22:36:30 -05:00
Jason Ross 06ca167034 Merge pull request #231 from JMR-dev/docs/e2e-read-findings
Read the instrumented suite, and find the test that proves nothing
2026-09-05 22:13:07 -05:00
JMR-devandClaude Opus 5 c757565d64 Read the instrumented suite, and find the test that proves nothing
Four coverage waves have been steered by JaCoCo, which measures
testDebugUnitTest only and cannot see app/src/androidTest at all. So
nothing had ever asked what the 60 device tests pin, only that they were
green. This is that read: a triage, not a test push, in the shape of
docs/coverage-read-findings.md.

Six findings (E1-E6) are things a test would not fix, and five of those
six are prose rather than code — the suite itself is in good condition.
Eight tickets carry the rest (#223-#230), each naming the mutation that
has to go red rather than a coverage delta.

The one that matters is #223. HardwareFallbackTest is the only automated
check of the hardware->software fallback against a real codec failure,
and it has never attempted the hardware path. Measured on run
34004304566: the API 33, 34, 35 and 37 legs each log

  Routing sample_h264_444.mp4 -> ... via FFMPEG (NO_HARDWARE_ENCODER)

because emulators expose no hardware encoder, so the router sends the job
straight to FFmpeg and runMedia3OrFallBack's catch is never entered. Its
two assertions — succeeded, output non-empty — are true anyway. It
finishes in 448 ms, which is not long enough to fail a hardware export
and then software-encode a three-second clip. Deleting that catch reddens
nothing anywhere.

Two things generalise. A test can assert and still not reach, which
neither a coverage number nor a "does it assert something" review can
see; the filter that works is whether the test's premise holds on the
machine running it. And the codebase already knew — ForcedFailureTest
pins DeviceCodecs.PERMISSIVE against this exact hazard and says why, as
does ConversionWorkerTest. Their assertions are about the path, so
without the pin they fail loudly; HardwareFallbackTest's are about the
output, so it passes quietly. That asymmetry is why nobody noticed.

E5 records the structural reason this document is separate: F7 in the
coverage findings calls probeWithExtractor's catch uncovered when
RemuxTest drives it on a device every leg. A JaCoCo-derived document
cannot see androidTest, so it will keep re-deriving that.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-05 21:54:56 -05:00
Jason Ross 4b02294cfb Merge pull request #222 from JMR-dev/docs/wave4-coverage-numbers
Record wave 4's landed coverage, and that the hang watchdog has fired
2026-09-05 21:24:43 -05:00
JMR-devandClaude Opus 5 79097a0256 Record wave 4's landed coverage, and that the hang watchdog has fired
Numbers re-measured on 6004398 rather than inherited: 94.2% line (2234/2372),
87.5% branch (1171/1338), 628 tests in 96 classes, missed 138 lines and 167
branches.

The entry explains its own branch move, because this file has a history of not
doing that. Wave 4's rise is almost entirely numerator -- up 80, with the
denominator moving -4 -- unlike the 2026-08-29 seam work, where the numerator
rose 37 while the denominator fell 70 and much of the gain was scaffolding
leaving the measurement. The line denominator grew 20, which is the seams the
wave cut rather than untested code.

Also records a methodological result worth more than the percentages: #218 fixed
an intermittent flake whose six-runs-per-arm comparison caught nothing either way
and proved nothing, and was settled by a deterministic mutation instead -- then
confirmed when the race reproduced on #217's leg, below the fix.

The JUnit-timeout trap gains the outcome it was waiting for: the jstack watchdog
caught #125's Room/WorkManager deadlock on CI in 10m57s with the hung test named,
against that ticket's prediction of a 60-minute cap and no cause. #125 is closed
as bounded; the inversion is internal to the two libraries and still live at
work-runtime 2.11.2 / room 2.7.0.

Instrumented counts are untouched: #221 is what changes them, so it carries them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-05 21:24:33 -05:00
Jason Ross 6004398a83 Merge pull request #219 from JMR-dev/fix/rotation-waits-for-recreation
Wait for the rotation to rebuild the Activity, not for the composition to idle
2026-09-05 20:43:47 -05:00
Jason Ross a354620bf5 Merge pull request #218 from JMR-dev/fix/injectable-startup-sweep-scope
Finish the startup sweep before onCreate returns, in the JVM suite
2026-09-05 20:43:24 -05:00
Jason Ross 17c91081cd Merge branch 'main' into fix/injectable-startup-sweep-scope 2026-09-05 20:35:39 -05:00
Jason Ross 20f718842d Merge pull request #217 from JMR-dev/test/session-outcome-seam
Give both engines one rc-to-outcome function, and unify the failure message
2026-09-05 20:29:45 -05:00
Jason Ross dec7089b59 Merge branch 'main' into test/session-outcome-seam 2026-09-05 20:21:57 -05:00
Jason Ross b677a9ad02 Merge pull request #216 from JMR-dev/fix/convert-guards-on-ready
Stop a permission answer starting a second conversion
2026-09-05 20:19:19 -05:00
Jason Ross 34e4ab52a4 Merge pull request #215 from JMR-dev/test/retry-save-mime
Open the save dialog with the type the job produced
2026-09-05 20:18:57 -05:00
Jason Ross 1437157a8f Merge pull request #214 from JMR-dev/test/launcher-callback-identity
Pin the launcher layer, where two callbacks share a signature
2026-09-05 20:18:36 -05:00
JMR-devandClaude Opus 5 32ab54da3c Wait for the rotation to rebuild the Activity, not for the composition to idle
thePickedInputSurvivesARealRotation synchronised a rotation with waitForIdle(),
which waits for the compose hierarchy to settle. Right 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 reads an unchanged identity
hash. Nothing waited for MainActivity to be rebuilt.

That is the clean AssertionError on #217's API 33 gating leg, run 33698846104:
it failed the SECOND guard, so the first had passed and the display really had
rotated. The wedges this ticket opened with are the same race taken the other
way -- land while the composition is being torn down and waitForIdle has nothing
coherent to settle on.

awaitRecreation() waits on a counter fed by the runner's lifecycle monitor,
bounded at 15 s. Deliberately not polling composeRule.activity: that resolves
through scenario.onActivity, which blocks on the main thread, so polling it
across a recreation is a plausible reading of the very wedge being fixed.

Both guards stay. The identity-hash one is now a backstop rather than the
primary detector -- a configChanges attribute trips the barrier's timeout first,
with a message saying what was waited for.

Measured on the local API 33 emulator. With the fix, 60/60 green. With the
watcher mutated so the counter never increments, the test fails in 15 s naming
itself and its condition, and the run still reports received 60/60 completed
cleanly -- where the same missing recreation used to cost the leg 20 minutes and
name nothing. The pre-fix flake does not reproduce on this host, so that is a
demonstration of the timeout path, not a before/after.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-05 19:36:42 -05:00
JMR-devandClaude Opus 5 e7caeeac43 Finish the startup sweep before onCreate returns, in the JVM suite
Robolectric builds an Application per test class that asks for one, and each
onCreate launched a staging sweep on Dispatchers.IO over the shared
<cacheDir>/conversions/. Nothing joined them, 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 started failing locally too.

LibreMediaConverterApp gains a protected open sweepScope and publishes the Job
onCreate started; the JVM suite substitutes TestLibreMediaConverterApp, whose
scope is Dispatchers.Unconfined so the sweep -- a plain function that never
suspends -- runs to completion inline. The SupervisorJob is kept so this differs
from production in the dispatcher alone.

Suite-wide rather than per-test: 27 of the 58 Robolectric classes touch that
directory, so opt-in was not a real option.

It costs one assertion, knowingly. AppStartSweepTest opened by asserting that
the manifest's android:name is what Robolectric instantiated. An application=
override replaces the manifest rather than being checked against it, and
applicationInfo.className reports the override too, so that claim is now
unobservable from this source set and a rewritten version would assert the
override against itself. The manifest link is device-only; the cast in setUp
still catches the test app ceasing to extend the real one.

AppStartSweepTest also joins the published Job instead of polling for ten
seconds -- a poll cannot tell "swept" from "not started yet" -- and gains a test
pinning that the sweep is complete when onCreate returns, which is the property
the substitution exists for and the only place it is checked.

Verified by mutation rather than by repetition. Putting the test app back on
Dispatchers.IO reddens that test 5 times out of 5, while running the whole suite
six times per arm caught nothing either way: at the rate #159 was observed at, a
clean six-run arm is roughly a coin flip, so the comparison was underpowered and
is not offered as evidence.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-05 19:36:40 -05:00
JMR-devandClaude Opus 5 ec2cae256f Give both engines one rc-to-outcome function, and unify the failure message (#203)
FFmpegEngine and ConcatEngine each carried their own copy of the same `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 -- both live inside a callback handed to FFmpegKit,
which does not run on the JVM -- so nothing could see that the two disagreed about what a
failed session says.

sessionOutcome() now holds the rule and each engine maps Success/Cancelled/Failed onto its
continuation. Verified JVM-safe rather than assumed: javap over the committed AAR shows
ReturnCode(int) as a plain public constructor with pure static isSuccess/isCancel and a
<clinit> that loads no native library.

Per #203's decision this unifies 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 a join reporting "FFmpeg failed" would be a worse
message than the one it replaces. There is a test for exactly that.

The two message sources are lambdas rather than values, and that is load-bearing.
getFailStackTrace and getAllLogsAsString 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 the shape this replaces did not -- it read them inside the else
branch. Same reasoning as capabilitiesFrom taking a Sequence in #194: a seam should not change
what runs when. There is a test that counts the reads, and the eager mutation reddens it.

A null return code is a real input rather than a defensive one -- getReturnCode() is nullable
and a session killed before reporting has none -- so it fails, with "null" where the number
would be.

Nothing asserted the old join text: `grep -rn 'Joining failed|FFmpeg failed' app/src/` returns
only main, plus ConcatWorker.GENERIC_FAILURE_MESSAGE, which is a different constant this does
not touch. Re-run immediately before committing, as the ticket asked.

Mutations, all run and restored:

  swap the ifBlank operands            1 red
  treat cancellation as a failure      2 red
  read both message sources eagerly    1 red
  hardcode the prefix                  1 red

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-05 19:36:39 -05:00
JMR-devandClaude Opus 5 4e88de3045 Stop a permission answer starting a second conversion (#202)
currentInput() answered for Converting, Waiting and Converted as well as Ready. Those three
arms were unreachable 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 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.

#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 -- the F1/F5 failure
mode docs/coverage-read-findings.md names. currentInput() is now
(_state.value as? ConversionState.Ready)?.input, which is what JoinViewModel.join() has been
all along; the two screens are the same shape and only one of them was over-general.

Four cold refusal arms come with it, all reached the same way -- a system callback arriving
after the screen has moved on, which is what a result redelivered after process death does:

  ConversionViewModel.kt:513   currentInput() ?: return
  ConversionViewModel.kt:600   pendingSave() ?: return
  JoinViewModel.kt:316         (as? Ready)?.inputs ?: return
  JoinViewModel.kt:390         pendingSave() ?: return

One fixture note worth keeping: the second case needs a real staged file in the worker's
output Data. A SUCCEEDED job with no output path maps to Failed rather than Converted, so
Data.EMPTY never reaches the state the case is about -- which cost a timed-out awaitState
before it was spotted.

Mutations, all run and restored:

  restore the over-general four-arm when   1 red  <- the defect this change fixes
  currentInput()!! at :513                 2 red
  pendingSave()!! in save()                1 red
  drop both join guards                    1 red

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-05 19:36:38 -05:00
JMR-devandClaude Opus 5 2db0dc65d3 Open the save dialog with the type the job produced (#201)
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 after a
failed save must open with the type its FIRST attempt used, because the fallback beside it
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 withdraws a named exemption rather than working around it. FailedSaveRetryTest's KDoc
listed the line as not asserted because "it lives in the entry point, above the ScreenContent
seam, and reaching it needs a real ViewModel inside a composition". True when written;
AdaptiveShellTest (#173) then established composing the real screens with real ViewModels,
and #200 added the ShadowActivity mechanics for reading what a launcher launched. The reason
the exemption gave no longer holds, so it is withdrawn in the same change rather than left
to be taken at face value -- the shape of #141 revising #84's boundary.

The job is reattached rather than run because the screen composes its own ViewModel through
viewModel() and nothing can be injected into it. That is also the case the line exists for:
a reattached job's spec was never in these settings at all.

The test asserts the two mime types differ as well as which one is used. Without that, the
assertion would pass just as well against the fallback if the fixture ever drifted onto MP4.

Mutation: collapse :80 to settings.spec.mimeType -- red. Run and restored.

Unrelated, and recorded because it turned up here: #159 now reproduces on this host. The
full suite failed once in six runs on OutputPublisherStagingTest:184, and the isolating
experiment says it is not this change -- three runs WITH the new test all passed, and the
failure occurred on a run with the file removed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-05 19:36:37 -05:00
30 changed files with 2176 additions and 92 deletions
+81 -4
View File
@@ -130,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** — **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.
- **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.
**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
@@ -283,6 +283,58 @@ install for code that can never run — and on API 37 the full APK does not fit
#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-E6**, 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 five of its six findings are prose rather than code —
the suite itself is in good shape. What had drifted is its self-description.
- **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.
@@ -410,4 +462,29 @@ 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.
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.
+22
View File
@@ -42,6 +42,28 @@
<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>
@@ -17,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 two.
* the gating one grows by [FAILS_ON_EMULATOR_API37_BASELINE].
*
* **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
@@ -52,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 = 3
const val FAILS_ON_EMULATOR_API37_BASELINE = 4
@@ -7,6 +7,10 @@ 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
@@ -14,6 +18,7 @@ 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
@@ -262,6 +267,92 @@ 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 {
@@ -280,6 +371,19 @@ 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,6 +13,7 @@ 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
@@ -27,7 +28,10 @@ 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
@@ -64,6 +68,26 @@ 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 {
@@ -75,7 +99,13 @@ class ReattachOnLaunchTest {
fun clearTheQueue() = emptyQueueAndStaging()
@After
fun leaveNothingBehind() = emptyQueueAndStaging()
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()
}
/**
* The claim the whole fix rests on, checked against the production request builder rather
@@ -261,6 +291,69 @@ 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.
@@ -326,5 +419,9 @@ 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,11 +12,17 @@ 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
@@ -34,6 +40,47 @@ 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):
*
@@ -66,6 +113,15 @@ 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,
@@ -75,6 +131,19 @@ 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) {
@@ -88,6 +157,14 @@ 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,10 +4,20 @@ 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
@@ -113,6 +123,11 @@ 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
@@ -127,6 +142,165 @@ 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 --------------------
@@ -166,4 +340,19 @@ 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,10 +5,20 @@ 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
@@ -16,6 +26,7 @@ import org.libremediaconverter.convert.MediaProbe
import org.libremediaconverter.convert.StagingNames
import org.libremediaconverter.ffmpeg.ConcatEngine
import org.libremediaconverter.model.ConcatStrategy
import org.libremediaconverter.work.ConcatWorker
import java.io.File
/**
@@ -50,6 +61,82 @@ 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
@@ -180,4 +267,13 @@ 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
}
}
@@ -0,0 +1,123 @@
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
}
}
@@ -0,0 +1,135 @@
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;
}
}
@@ -10,6 +10,9 @@ import androidx.compose.ui.test.performClick
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
@@ -24,6 +27,7 @@ import org.junit.runner.RunWith
import org.libremediaconverter.FailsOnEmulatorApi37
import org.libremediaconverter.MainActivity
import org.libremediaconverter.ui.TestTags
import java.util.concurrent.atomic.AtomicInteger
/**
* Choosing a file, through the real system picker, and still having it after a rotation.
@@ -203,6 +207,8 @@ import org.libremediaconverter.ui.TestTags
* 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]
*
@@ -251,6 +257,21 @@ 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.
*
@@ -270,6 +291,7 @@ class SafPickerRoundTripTest {
*/
@After
fun restoreOrientation() {
ActivityLifecycleMonitorRegistry.getInstance().removeLifecycleCallback(recreationWatcher)
if (!rotated) return
device.setOrientationNatural()
device.unfreezeRotation()
@@ -303,9 +325,11 @@ 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
@@ -675,6 +699,36 @@ 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) {
composeRule.waitUntil("a node tagged $tag exists", APP_TIMEOUT_MS) {
composeRule.onAllNodesWithTag(tag).fetchSemanticsNodes().isNotEmpty()
@@ -703,6 +757,15 @@ 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.
*
@@ -0,0 +1,129 @@
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,6 +3,7 @@ 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
@@ -17,14 +18,36 @@ 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.
*/
class LibreMediaConverterApp : Application() {
open 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.
*/
private val appScope = CoroutineScope(SupervisorJob() + Dispatchers.IO)
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
override fun onCreate() {
super.onCreate()
@@ -53,6 +76,6 @@ 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.
appScope.launch { OutputPublisher(this@LibreMediaConverterApp).sweepStaging() }
startupSweep = sweepScope.launch { OutputPublisher(this@LibreMediaConverterApp).sweepStaging() }
}
}
@@ -673,13 +673,23 @@ class ConversionViewModel @JvmOverloads constructor(
else -> null
}
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
}
/**
* 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 companion object {
/**
@@ -5,7 +5,6 @@ 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
@@ -66,16 +65,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 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(),
),
)
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))
}
}
cont.invokeOnCancellation { FFmpegKit.cancel(session.getSessionId()) }
@@ -35,6 +35,21 @@ 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")
@@ -84,4 +99,12 @@ 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,7 +4,6 @@ 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
@@ -51,19 +50,16 @@ class FFmpegEngine : SoftwareTranscoder {
val session = FFmpegKit.executeWithArgumentsAsync(
args.toTypedArray(),
{ completed ->
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()
},
),
)
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))
}
},
{ log -> Log.d(TAG, log.message.trimEnd()) },
@@ -0,0 +1,54 @@
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 org.junit.Assert.assertEquals
import kotlinx.coroutines.runBlocking
import org.junit.Assert.assertNotNull
import org.junit.Assert.assertTrue
import org.junit.Assert.fail
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
@@ -10,7 +10,6 @@ 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.
@@ -23,8 +22,19 @@ import java.util.concurrent.TimeUnit
* 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, the scope it launches on is already there, and the first test
* below is what pins that the framework calls it on *this* class.
* 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.
*/
@RunWith(RobolectricTestRunner::class)
class AppStartSweepTest {
@@ -34,17 +44,35 @@ 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 application the manifest starts is the one that sweeps`() {
assertEquals(LibreMediaConverterApp::class.java, RuntimeEnvironment.getApplication().javaClass)
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,
)
}
@Test
@@ -64,35 +92,23 @@ class AppStartSweepTest {
app.onCreate()
awaitGone(abandoned)
// 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())
// 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
}
}
@@ -0,0 +1,28 @@
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)
}
@@ -51,10 +51,13 @@ 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.** 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.
* - ~~**`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.
* - **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
@@ -6,6 +6,7 @@ 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
@@ -77,6 +78,28 @@ class LauncherWiringTest {
* 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`() {
@@ -85,6 +108,11 @@ class LauncherWiringTest {
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()
}
@@ -138,4 +166,13 @@ class LauncherWiringTest {
)
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
}
}
@@ -122,24 +122,25 @@ 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 loses a race that CI
* caught and this machine does not reproduce.
* 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.
*
* `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.
* **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.
*
* 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.
* 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*.
*/
private fun stagingPathAsRegularFile(): File {
val stagingPath = File(cacheDir, "conversions")
@@ -0,0 +1,136 @@
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"
}
}
@@ -0,0 +1,147 @@
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")
}
}
@@ -56,6 +56,33 @@ 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(
@@ -0,0 +1,127 @@
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 })
}
@@ -10,3 +10,9 @@
# 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
+327
View File
@@ -0,0 +1,327 @@
# E2E-read findings
**Status:** six findings, none fixed, none urgent — **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.
---
## 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 | **fix the sentence** |
| 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** |
**Five of the six 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 |
|---|---|
| **#223** | `HardwareFallbackTest` never attempts the hardware path on any emulator leg |
| **#224** | Cancelling a *running* native session, in any of the three engines |
| **#225** | No `content://` input has reached a *successful* conversion — the ffkitsaf bridge |
| **#226** | `OutputPublisher.publish` against a real `DocumentsProvider`, and the SAF premise it rests on |
| **#227** | The notification's Cancel action has never been fired |
| **#228** | `encodesFlacLosslessAudio` and `encodesOpus` pass on any non-empty file |
| **#229** | FFmpeg's progress percentage is computed everywhere and asserted nowhere |
| **#230** | *(spike)* whether a running conversion's process can be killed under instrumentation |
**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.
+12
View File
@@ -312,6 +312,18 @@ 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