Compare commits

..
Author SHA1 Message Date
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
Jason Ross 1041faf920 Merge branch 'main' into test/launcher-callback-identity 2026-09-05 20:11:15 -05:00
Jason Ross fe68f839c1 Merge pull request #213 from JMR-dev/test/theme-follows-system-dark
Call the theme the way MainActivity calls it
2026-09-05 20:10:01 -05:00
Jason Ross f65578b1f7 Merge branch 'main' into test/theme-follows-system-dark 2026-09-05 20:01:47 -05:00
Jason Ross b38ad6a683 Merge pull request #212 from JMR-dev/test/hardware-progress-reaches-workmanager
Report hardware progress to WorkManager, which nothing had checked
2026-09-05 20:00:46 -05:00
Jason Ross 9a0f494e26 Merge pull request #211 from JMR-dev/test/ffprobe-mapping-seam
Read FFprobe's answer without spawning FFprobe
2026-09-05 20:00:22 -05:00
Jason Ross f3478706b3 Merge pull request #210 from JMR-dev/test/device-codec-enumeration-seam
Cut a seam through the codec enumeration, and say what a failed one actually does
2026-09-05 19:59:58 -05:00
Jason Ross 61c400d2c6 Merge pull request #209 from JMR-dev/test/unknown-container-row
Render the container row for a video nothing could name
2026-09-05 19:59:36 -05:00
Jason Ross d83775d5c6 Merge pull request #208 from JMR-dev/test/audio-drop-arm
Build a command for the audio the user turned off
2026-09-05 19:59:14 -05:00
Jason Ross e90f5a801c Merge branch 'main' into test/audio-drop-arm 2026-09-05 19:49:49 -05:00
Jason Ross 68015b3374 Merge pull request #207 from JMR-dev/test/null-message-fallbacks
Make a failure that says nothing still say something
2026-09-05 19:48:11 -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
JMR-devandClaude Opus 5 6334dcba34 Pin the launcher layer, where two callbacks share a signature (#200)
ConversionViewModel.onInputPicked(uri: Uri) and .save(destination: Uri) are both
(Uri) -> Unit, so swapping the two launcher callbacks at ConverterScreen.kt:70 and :83
compiles, renders, and passed the entire suite. Picking a file would attempt a save to it;
choosing a destination would load it as input.

That is the defect class ScreenWiringTest exists for, on the one pair it declines to cover:
it drives converterActions directly and says the launcher-backed actions stay parameters.
Right about the actions seam, and it leaves the edge above that seam unpinned. Join's
equivalents are List<Uri> and Uri, so they are not transposable and get no such test.

Two mechanics, neither used anywhere else in the suite, so both were spiked before any
assertion was written:

  shadowOf(activity).nextStartedActivityForResult   reads the launched Intent, EXTRA_MIME_TYPES intact
  shadowOf(activity).receiveResult(...)             reaches ComponentActivity's ActivityResultRegistry
                                                    and fires the rememberLauncherForActivityResult callback

createAndroidComposeRule for AdaptiveShellTest's reason: the screens compose real ViewModels
through viewModel(), and the plain rule supplies no ViewModelStoreOwner.

The picker filter rides along, since the harness is the same. ConverterScreen.kt:65-67
records why the all-types wildcard is load-bearing -- the photo picker offers no audio and
misses mkv/flac/webm -- and narrowing it would have made every audio conversion unreachable
from the picker with nothing going red.

One incidental: a KDoc cannot contain the all-types wildcard, because its second half closes
the block comment. The literal is spelled only in the assertion, and the KDoc says why.

Mutations, all run and restored:

  transpose onInputPicked and save          1 red
  narrow the converter picker to video only 1 red
  widen the join picker to every type       1 red

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-05 19:36:36 -05:00
JMR-devandClaude Opus 5 7e09f010c7 Call the theme the way MainActivity calls it (#197)
ThemeColorSchemeTest resolves every branch of the `when` and always passes darkTheme
explicitly, so the $default bridge is never entered and isSystemInDarkTheme() is never
called. MainActivity.kt:79 is its only default-argument caller and does not execute on the
JVM, which left the app's actual call shape -- no arguments at all -- the one nothing
exercised. LibreMediaConverterTheme reported mi=21, mb=6, cb=12 at method level.

Not #68. That issue is the two unreachable arms, DarkColorScheme and LightColorScheme,
which cannot run because dynamicColor is always true and nothing can flip it; it is an open
product decision and stays open. This is the reachable half.

The assertion compares schemes rather than reading a luminance threshold, which would be a
guess about the device palette. What is asserted is that the no-argument call resolves the
SAME scheme an explicit darkTheme of the matching value does, and a different one from its
opposite -- true whatever palette the platform hands back, and exactly the claim being made:
the default reads the system rather than picking a side. The two assertions are also what
stops the pair passing vacuously if all three resolutions were identical.

Two @Config(qualifiers = ...) cases rather than two classes: qualifiers are settable per
method, unlike the sdk pinning ForegroundTypeRegimeTest needed nested classes for.

Mutations, all run and restored, and each reddening a different half -- which is also what
shows the qualifiers take effect rather than both cases running in one mode:

  darkTheme defaulted to false            night case red
  darkTheme defaulted to true             light case red
  isSystemInDarkTheme() inverted          both red

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-05 19:36:34 -05:00
JMR-devandClaude Opus 5 49249be280 Report hardware progress to WorkManager, which nothing had checked (#196)
ConversionWorker.kt:208-210 is a second onProgress lambda at a second call site -- the one
handed to engine.transcode -- and it reported ci == 0. Every test in this file drives the
FFmpeg path; HardwareFallbackTest reaches runMedia3OrFallBack but its recording transcoder
records the call and never invokes the callback it was handed.

So the two engines' progress wiring was one tested and one not, and the untested one is the
default: ConversionRouter sends everything it can to Media3, which makes this the lambda
most conversions actually use. Same asymmetry argument CLAUDE.md records for
ContainerCapabilities:94.

It goes in this file rather than beside HardwareFallbackTest because this is where progress
plumbing lives and where RecordingForegroundUpdater already is -- and because the software
and hardware cases now sit side by side, which is what makes the asymmetry visible rather
than merely fixed. workerReporting gains an engine-preference parameter defaulted to
FORCE_SOFTWARE, so no existing case changes.

AUTO with a real H.264 probe, because FORCE_SOFTWARE is exactly what keeps the other tests
out of this branch, and because InputProbe() reports UNPARSEABLE -- which PERMISSIVE.canDecode
refuses, sending every job to FFmpeg with no test saying why.

The percentage is asserted, not merely that an update happened: publishProgress takes a
display name and a percent, and replacing the percent with a constant compiles fine.

Mutations, both run and restored:

  empty the hardware onProgress lambda            1 red
  report a constant percent instead of the engine's   1 red

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-05 19:36:33 -05:00
JMR-devandClaude Opus 5 2125763ebf Read FFprobe's answer without spawning FFprobe (#195)
readMediaInformation was 114 missed instructions and 24 missed branches -- the second
largest block on the wave-4 report -- and exactly one line of it needed a device:

    FFprobeKit.getMediaInformation(path).getMediaInformation()

Everything after it reads an ordinary object, so it moves into ffprobeInfoFrom and the edge
keeps the call and the null check. Verified JVM-safe rather than assumed: javap over the
committed AAR's runtime jar shows MediaInformation(JSONObject, List<StreamInformation>,
List<Chapter>) and StreamInformation(JSONObject) as plain public constructors whose <clinit>
does not load the native library, so a test builds its own without libffmpegkit present.

The decision worth reaching is containerFrom's SECOND argument. FFprobe reports
"matroska,webm" for both MKV and WebM because they share a demuxer, so the video codec is
the only thing separating them. containerFrom has thirty-three covered branches and not one
can notice that argument being dropped -- the mistake is at the call, not in the callee, so
every existing containerFrom test stays green while every VP9 WebM quietly becomes an MKV.

Two things the tests found rather than confirmed.

The format properties are NESTED under "format": getFormat() resolves through
getStringFormatProperty, not off the top-level object. The first fixture put the keys at the
top level and four cases failed with a null container. The helper says so now.

And one mutation SURVIVED on the first pass -- reading dimensions with
streams.firstNotNullOfOrNull { it.getWidth() } instead of video?.getWidth(). The fixture put
the dimensions on the chosen video stream, which is also the first stream carrying any, so
the two readings agreed and the test could not tell them apart. Separating them needs a
chosen video stream with NO dimensions and a later one that has them, which is a real shape:
FFprobe omits width/height for a stream it could not measure. That case is now its own test
and the mutation reddens it.

Mutations, each run and restored:

  drop the video codec argument to containerFrom        1 red
  take the LAST video stream instead of the first       1 red
  read dimensions from any stream, not the chosen one   0 red -> 1 red after the new case
  let an unparseable duration throw instead of zero     1 red

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-05 19:36:32 -05:00
JMR-devandClaude Opus 5 6d700f0014 Cut a seam through the codec enumeration, and say what a failed one actually does (#194)
probe() was 20 never-executed lines, the biggest single block on the report. It has been
looked at twice and left out twice, and both closes were right about what they closed:
#86 ruled it device-bound, and #133 re-checked that with ShadowMediaCodecList in hand and
still declined, because MediaCodecInfoBuilder has no setIsAlias and no setCanonicalName --
"so the alias skip and the canonical-name dedup, the two things the class's KDoc calls out
as easy to get wrong, are not reachable through it."

That objection is about the shadow. It does not apply to a function taking its own entry
type, which #133 did not evaluate. capabilitiesFrom(enumerate: () -> Sequence<CodecEntry>)
holds every rule; the edge keeps only the mapping from MediaCodecList onto CodecEntry.

The parameter is a Sequence rather than a List on purpose. runCatching has always wrapped
the *iteration*, so a MediaCodecInfo whose properties throw partway leaves the codecs
already read in place. A List parameter would move that throw outside the loop and turn a
partial answer into an empty one -- a behaviour change smuggled in as a refactor. There is
now a test for the partial case, and swapping the Sequence for an eager toList() reddens it.

The behaviour change this DOES make is one line of log, and it is the reason the seam was
worth cutting at all. The fallback said "Codec enumeration failed; assuming permissive" and
returned empty sets -- but "video/avc" in emptySet() is false, so canEncode and canDecode
answer no to everything and every job routes to FFmpeg. That is the restrictive answer, and
it is the right one: FFmpeg does whatever Media3 does, only slower. The code stays; the
message and the class KDoc now describe it.

Seven tests. One of them was wrong first and the mutation is what said so: the alias case
originally listed the alias *after* the codec it aliases and passed with the skip deleted,
because canonicalName is shared and the dedup catches the second entry either way. The two
rules overlap, so a fixture that does not separate them tests neither. Order separates
them -- an alias arriving first claims the canonical name in `seen` and gets its own types
credited, and the real codec is then dropped by the dedup. That is now the test, and it
also says what the rule is worth: with a Set accumulator, an alias declaring the same types
as its codec changes nothing, so the skip earns its place only when the two disagree.

Mutations, each run and restored, each reddening the test that owns it:

  drop !isSoftwareOnly from the encoder predicate     1 red
  remove the alias skip                               1 red  (0 before the fixture was fixed)
  remove the canonical-name dedup                     1 red
  remove the video/ prefix filter                     1 red
  apply the hardware predicate to decoders too        1 red
  make the failure fallback permissive                2 red
  eager toList() instead of the lazy Sequence         1 red

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-05 19:36:31 -05:00
JMR-devandClaude Opus 5 da8d53851b Render the container row for a video nothing could name (#199)
ConverterScreen.kt:668's null arm -- DetailRow("Container", probe.container?.label ?:
"Unknown") in the VIDEO branch -- had never rendered. Every video case in FileCardTest uses
VIDEO_PROBE, which carries container = MP4.

The argument for adding it is the asymmetry, not the coverage. FileCard renders that exact
expression twice, once in AUDIO_ONLY (:660) and once in VIDEO (:668), and "an audio-only
file nothing else could describe degrades one row at a time" drives only the first. Same
expression, same fallback, one kind covered and one not -- which is the same argument
CLAUDE.md records for including ContainerCapabilities:94.

Nor is null an edge case here. InputProbe.container's own KDoc says MediaExtractor cannot
report a container at all, so it comes from FFprobe alone: any run where FFprobe did not
answer produces exactly this shape -- real codec, real dimensions, real duration, no
container. An empty value in its place would read as a rendering bug rather than as a
probe that got half its sources.

The other three rows are asserted alongside, which is what keeps this from being a copy of
the audio-only case. There, everything is unknown at once; here one field is missing from
a probe that is otherwise complete, and the rest have to be unaffected by it.

Mutation: `?: "Unknown"` -> `?: ""` at :668 only, run and restored. The AUDIO_ONLY twin at
:660 is a separate expression, and mutating that one would redden the existing test instead
-- which would prove nothing about this one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-05 19:36:30 -05:00
JMR-devandClaude Opus 5 cc215195ee Build a command for the audio the user turned off (#198)
audioArgs' Drop arm -- `AudioPlan.Drop -> listOf("-an")` -- was ci == 0. The suite's only
-an assertion lives in "gif generates a palette to avoid banding and drops audio", and that
one comes from the image path at FFmpegCommandBuilder.kt:79/:90, which emits -an directly
and never reaches audioArgs. Two sites, one string, one tested.

It is a live path rather than defensive code. AdvancedPicker renders all of
AudioCodec.entries including NONE, ContainerCapabilities.validate permits audio-off whenever
the input has video, and MKV routes the job to FFmpeg -- so "convert this and drop the
soundtrack" is something a user can do today and nothing had built the command for.

Both halves are asserted, and the second is not padding: -an alone still passes if the arm
falls through to the else and emits an AAC encoder beside the flag, which is a file that is
silent because the flag won while carrying an encoder nobody asked for.

Three mutations, all run and restored. The third is the one that justifies the second
assertion, since the first two break -an as a side effect and so cannot show it:

  Drop -> emptyList()                     red
  Drop -> the else arm's aac encoder      red (loses -an as well)
  Drop -> listOf("-an", "-c:a", "aac")    red -- -an intact, caught by assertFalse

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-05 19:36:28 -05:00
JMR-devandClaude Opus 5 92bcff8656 Make a failure that says nothing still say something (#193)
Three sites, all ci == 0 before this, and all the same rule:

  work/ConversionWorker.kt:316        cause.message ?: GENERIC_FAILURE_MESSAGE
  convert/ConversionViewModel.kt:631  e.message ?: SAVE_FAILED_MESSAGE
  join/JoinViewModel.kt:416           e.message ?: SAVE_FAILED_MESSAGE

Every existing test throws WITH a message, so the right-hand side had never been evaluated
anywhere in the suite. A Throwable carrying none is not exotic: RuntimeException(),
IOException() and most platform exceptions raised without an argument all have a null
message, and a native engine that dies is exactly where one comes from.

The worker case needed care, and the care is the reason it survived three waves.
ConversionStateMappingTest's "a failure with nothing said still says something" looks like
it covers that site and does not -- it drives the READ side, map(FAILED, Data.EMPTY), and
that side has a fallback of its own at ConversionViewModel.kt:147-149 which turns a blank
KEY_ERROR back into the same constant. So a test asserting on the resulting Failed state
stays green while the worker's fallback is broken. Measured rather than reasoned: with
:316 mutated to .orEmpty(), exactly ONE of 587 tests went red, and it was the new one.
Everything else, including the test that appears to cover it, stayed green.

So the worker case reads KEY_ERROR off the worker's own Result, before anything downstream
can repair it. The two save cases have no such second line -- both write _state.value
directly -- so the state is the right thing to assert there, and both also assert that
`pending` still travels: a fallback that dropped the handle would leave the file
unreachable from the very screen that just said the save failed.

Held in one class against the ticket's suggestion of three. They are one rule at three
layers, and the masking above has to be explained once rather than three times.
FailedSaveRetryTest already sets the precedent for both ViewModels in one file; this adds
one worker to that shape.

FORCE_SOFTWARE in the worker fixture so the failure comes straight out of runFFmpeg. AUTO
would enter runMedia3OrFallBack, whose catch runs the job a second time in software: the
same exception arrives, but by a path this is not about and which HardwareFallbackTest owns.

Mutations, all run and restored -- each site to .orEmpty(), never to a different constant,
which would only prove the test reads a constant:

  ConversionWorker:316      1 of 587 red (this file)
  ConversionViewModel:631   red
  JoinViewModel:416         red

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-05 19:36:27 -05:00
Jason Ross a847d3a81d Merge pull request #206 from JMR-dev/test/cancel-reaches-workmanager
Connect the Cancel button to WorkManager, which nothing did
2026-09-05 19:31:00 -05:00
JMR-devandClaude Opus 5 e4867ff956 Connect the Cancel button to WorkManager, which nothing did (#192)
Both ViewModels' cancel() is one line -- activeWorkId?.let(workManager::cancelWorkById) --
and JaCoCo reports every line of both as covered. The only test of either was
SettingsEditsTest's "cancelling with no active job does nothing rather than throwing",
whose own comment names the half it drives: "the null side". The other side had never been
entered, and JoinViewModel.cancel() had no test at all.

So nothing in 584 tests connected the Cancel button to WorkManager. The affordance tests
click TestTags.CANCEL and assert the action fires into a stub; ScreenWiringTest asserts the
action calls viewModel.cancel(). Both halves were pinned and the join between them was not.

No line-level filter could have found this. It takes a method-level read -- mi=11, ci=7,
mb=1, cb=1 on both -- a covered method with an arm nothing takes, which is the second of
the two filters #194 records and the gap that argued for adding it.

The fixture is why this stayed uncovered rather than why it is hard. The test WorkManager
runs on a SynchronousExecutor, so an ordinary request finishes inline: by the time a test
could call cancel(), convert()'s job was already terminal, leaving only the null arm
reachable. setInitialDelay is what TestScheduler honours, so the job sits in ENQUEUED and
the test never releases it. Production never sets a delay, so the request is built by hand
rather than through ConversionWorker.request -- but ENQUEUED at runAttemptCount 0 is a real
state every job passes through, Reattachment.choose ranks it QUEUED, and conversionStateFrom
maps it to Converting(input, 0). The delay changes how long the job stays in a real state,
not which state it is in.

Three tests, and the third is not padding: without it, cancelAllWork() in place of
cancelWorkById(activeWorkId) passes the other two. It asserts the shape rather than the
identity -- exactly one of two queued jobs is cancelled -- because which one the ViewModel
reattached to is the query's business, and Reattachment's ordering notes say queued jobs
are left tied deliberately.

WorkManager's own record is asserted before the screen. The screen alone would be weaker
than it looks: CANCELLED maps to Idle for a reattached job, and Idle is also where a
ViewModel that did nothing whatsoever would sit.

Mutations, both run and both restored:

  cancel() -> no-op                          all three red
  cancelWorkById(id) -> cancelAllWork()      only the third red

The second is what shows the third test does independent work rather than restating the
first two.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-02 17:58:03 -05:00
Jason Ross 4d5fba515a Merge pull request #205 from JMR-dev/docs/wave4-coverage-findings
Record the wave-4 coverage read, and correct the filter that missed the biggest gap
2026-09-02 16:41:49 -05:00
JMR-devandClaude Opus 5 223fe6deea Record what the wave-4 coverage read found, and correct the filter that missed the biggest gap
Five findings (F6-F10) and a methodology correction. The twelve test tickets the same
read produced are #192-#203, with #204 for four candidates whose cost was not obviously
worth paying; nothing here is work, by this document's standing rule.

The correction is the part worth carrying forward. Wave 3 filtered candidates on `mi > 0`
and CLAUDE.md recommended it. That filter fails in both directions. It over-reports on
Compose: JoinScreen.kt:222 reads mi=10 and also ci=38, and JoinStateAffordancesTest
already clicks that Save button and asserts save:joined.mp4 -- the missed instructions are
the synthesized $changed/$dirty recomposition-skip path, the same codegen this repo
already knew inflated the branch count, showing up in the instruction count too. Every
onClick lambda flagged that way turned out to be covered at method level.

It under-reports on the case that mattered more. ConversionViewModel.cancel() and
JoinViewModel.cancel() miss no line at all, so no line-level filter can see them -- yet
only the null arm of activeWorkId?.let(workManager::cancelWorkById) had ever been entered,
and nothing in 584 tests connected the Cancel button to WorkManager. That is #192, and it
needs `ci > 0 && mb > 0` at method level to surface. Use both filters; `ci == 0` alone is
JaCoCo's own missed-line definition and needs no judgement, which is why it is the first.

The five findings are what a test would not fix. F6: four more unreachable arms, each
traced to the upstream guard that makes it so, one of which (ConversionRouter:214-217)
carries a KDoc describing a hazard :117 already removed. F7: probeWithExtractor's catch is
unreachable for the same reason probeForConcat's is -- the measurement was on record for
one site and not the other, three lines apart in the same file. F8: three more dead
members and six unused defaults. F9: both getForegroundInfo overrides are dead because
getForegroundInfoAsync is only called for expedited work and nothing sets it -- which
sharpens #88's close rather than reopening it. F10: three arms that ARE reachable and
still cannot be made to bite, recorded because all three were picked up as candidates and
put down again.

Six of the ten findings are now "no action" or "not a test gap", and that shape is the
honest summary of what is left: arms nothing can reach, members nothing calls, and arms a
test can reach but not pin. A coverage number tells none of them apart.

One close is qualified rather than overturned. #86 and #133 ruled AndroidDeviceCodecs.probe()
out through ShadowMediaCodecList, on the grounds that MediaCodecInfoBuilder cannot set
isAlias or canonicalName. A pure seam does not have that constraint and #133 did not
evaluate one, so #194 is a different mechanism, not a third run of the same spike -- and
its argument is not coverage but that the runCatching fallback logs "assuming permissive"
while returning empty sets, which makes canEncode and canDecode answer no for everything.

Documentation only: no Kotlin, Gradle or shell file is touched.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-02 07:54:57 -05:00
Jason Ross 54ca2dda5a Merge pull request #191 from JMR-dev/docs/coverage-wave3-recovery
Re-measure after wave 3, and write down the two kinds of gap it had to separate
2026-09-02 00:06:33 -05:00
JMR-dev 5a5a680b4c Re-measure after wave 3, and write down the two kinds of gap it had to separate
92.8% line (2183/2352), 81.3% branch (1091/1342), 584 JVM tests in 87 classes, measured
2026-09-02 on the tree this branch creates rather than quoted from a PR body.

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

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

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

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

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Recovered onto main after hitting #160's trap for real. #189 was opened against
test/concat-engine-seam and, unlike #184-#188, never retargeted to main before merging --
so it merged into a branch that had already been merged and left behind. GitHub reported
`merged`, the PR shows MERGED, and none of it was on main: `git merge-base --is-ancestor`
is what said so, one line, immediately.

That check is the entire reason this was a five-minute recovery rather than a coverage
entry that silently stayed three points stale. The failure mode is exactly what CLAUDE.md
warns about; what it did not say, and now would, is that the auto-retarget it describes
belongs to GitHub's stacking feature, so a stack opened with plain `gh pr create --base`
has to be retargeted by hand for every single PR -- and missing one is invisible until you
check ancestry.

Numbers re-measured on this tree with --rerun-tasks rather than inherited from the branch
they were taken on: 584 tests, 0 failures, 2183/2352 line, 1091/1342 branch. Same figures,
earned again.
2026-09-01 23:46:51 -05:00
Jason Ross 7f23ea8fd7 Merge pull request #188 from JMR-dev/test/concat-engine-seam
C1 (#176): give ConcatWorker the seam ConversionWorker always had, and test what was behind it
2026-09-01 23:44:27 -05:00
Jason Ross 05422a6896 Merge pull request #187 from JMR-dev/test/mediaprobe-merge-seam
C2 (#177): cut MediaProbe's two-probe merge into a seam, and ask which probe wins
2026-09-01 23:44:21 -05:00
Jason Ross fa3a32e5c0 Merge pull request #186 from JMR-dev/test/adaptive-shell-wiring
B1 (#173): tell the rail from the bottom bar, and the Convert tab from the Join tab
2026-09-01 23:13:01 -05:00
Jason Ross e2c9981bbe Merge pull request #185 from JMR-dev/test/aac-audio-args
B3 (#175): pin the AAC arm every ordinary conversion takes
2026-09-01 23:12:54 -05:00
Jason Ross 7a5622c896 Merge pull request #184 from JMR-dev/test/notification-progress-text
B2 (#174): read what the progress notification actually says
2026-09-01 23:12:35 -05:00
Jason Ross d4ca6b7b0f Merge pull request #183 from JMR-dev/test/media3-muxer-guard
A5 (#171): fire the muxer guard that repairs "MP4 for everything", which had never fired
2026-09-01 23:02:03 -05:00
Jason Ross bbe40cf8f7 Merge pull request #182 from JMR-dev/test/hardware-fallback-and-cancellation
A2 + A3 (#168, #169): the hardware fallback, the cancellation that must not take it, and the name a job may not have
2026-09-01 22:36:13 -05:00
Jason Ross f81d76e730 Merge pull request #181 from JMR-dev/test/unprobeable-join-clip
A4 (#170): join the two halves of an unreadable join clip, and record why the catch arm stays device-only
2026-09-01 22:34:25 -05:00
Jason Ross 8849ae96eb Merge pull request #180 from JMR-dev/test/one-branch-outcomes
A6 (#172): six one-branch outcomes nothing produced, and one that cannot be produced
2026-09-01 22:26:27 -05:00
Jason Ross 25e14b26d9 Merge pull request #179 from JMR-dev/test/foreground-type-regimes
A1 (#167): pin all three foreground-service regimes, and the boundary between two of them
2026-09-01 22:13:13 -05:00
21 changed files with 2032 additions and 68 deletions
+47
View File
@@ -184,11 +184,29 @@ install for code that can never run — and on API 37 the full APK does not fit
- **coverage gaps** — the line never executes. Filtered to sites where JaCoCo reports `mi > 0`, a
concrete instruction no test runs, which is what separates a real gap from a partial branch on
a compound condition. That filter cut the candidate list roughly in half and was right to.
**Wave 4 found it wrong in both directions, though — use the two filters below instead.**
- **assertion gaps** — JaCoCo is green and nothing checks the answer. `MainActivity`'s rail and
bottom bar were both *executed* by `AppRootRestorationTest` and **transposing them passed the
entire suite**; so did swapping the two progress-notification strings, and swapping `Content`'s
two destinations. No coverage number would ever have found any of the three.
**Wave 4 (2026-09-02) corrected that first filter, and the correction is the reusable part.**
`mi > 0` fails in both directions. It *over-reports* on Compose: `JoinScreen.kt:222` reads
`mi=10` and also `ci=38`, and `JoinStateAffordancesTest` already clicks that Save button and
asserts `save:joined.mp4` — the missed instructions are the synthesized `$changed`/`$dirty`
recomposition-skip path, the same codegen this file already warns about for *branch* counts,
showing up in the instruction count too. And it *under-reports* on warm methods with cold arms:
`ConversionViewModel.cancel()` misses no line, yet `activeWorkId?.let(...)` had only ever been
entered on the null side in 584 tests. Use two filters together instead:
- **`ci == 0`** — the line never executed. This is JaCoCo's own missed-line definition, so it
totals exactly the reported missed-line count and needs no judgement.
- **`ci > 0 && mb > 0` at method level** — a covered method with an arm nothing takes. This is
the only one that finds the `cancel()` shape.
Of wave 4's 251 missed branches, just **18** sat on lines that do execute, so the branch gap and
the line gap are largely the same gap; the second filter is about which of them are reachable.
So **every ticket named the mutation that had to go red, and that was its acceptance criterion
rather than a coverage delta**. It caught **two vacuous tests written in the same session**,
before either shipped:
@@ -236,6 +254,35 @@ install for code that can never run — and on API 37 the full APK does not fit
And **re-measure before quoting**: this entry was once written quoting 81.4%, measured four hours
earlier, and was already three points stale by the time it was ready to merge.
**Wave 4's read (2026-09-02) moved no number at all, and that is its result.** It was a triage
rather than a test push: twelve tickets (**#192-#203**), four deferred candidates (**#204**), and
five findings (**F6-F10** in `docs/coverage-read-findings.md`). What it establishes is the shape
of what is left, which is different again from wave 3's:
- Of 169 never-executed lines, **81 are native or device edges and stay that way** —
`FFmpegEngine` 33, `Media3Engine` 24, `ConcatEngine` 14, `MainActivity.onCreate` 10 — their
zeroes being the `testDebugUnitTest`-only measurement boundary that #84, #85, #86 and #88 each
recorded before. A further **34 are device-bound only until a seam moves them**:
`AndroidDeviceCodecs` 20 (#194) and the 14 of `MediaProbe`'s 26 that are `readMediaInformation`
(#195). Do not read that second group as exempt — the two tickets exist because it is not.
- Most of the rest is **already closed with a reason on record**, or compiler-generated: default-arg
bridges, DI factory lambdas, synthetic `NoWhenBranchMatchedException` arms, coroutine completion.
- Six of the ten findings in that document are now "no action" or "not a test gap". By this point
the report's remaining red is mostly arms nothing can reach, members nothing calls, and arms a
test *can* reach but cannot pin — and a coverage number tells none of them apart.
**The biggest single gap it found was not a missed line.** `ConversionViewModel.cancel()` and
`JoinViewModel.cancel()` report every line covered; only the null arm of
`activeWorkId?.let(workManager::cancelWorkById)` had ever been entered, so nothing in 584 tests
connected the Cancel button to WorkManager (#192). That is what the second filter above is for.
It also re-opened a mechanism, not a close: #86 and #133 ruled `AndroidDeviceCodecs.probe()` out
**through `ShadowMediaCodecList`**, on the grounds that the builder cannot set `isAlias` or
`canonicalName`. A pure seam does not have that constraint, and #133 did not evaluate one. Read
#194 before re-arguing either way — and note the reason it is worth cutting is not coverage but
that the `runCatching` fallback logs "assuming permissive" while returning empty sets, which makes
`canEncode` and `canDecode` answer *no* for everything.
- **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.
@@ -21,6 +21,11 @@ import org.libremediaconverter.model.VideoCodec
* words, "cannot be tested for correctness". It is a hint, not a guarantee, which is
* why the router treats a failed hardware export as a signal to fall back rather
* than trusting this up front.
* - **An enumeration that fails answers no to everything**, which sends every job to
* FFmpeg. Empty sets are not a permissive default: `canEncode` looks a MIME type up in
* [hardwareEncodeMimes] and finds nothing there. That is the intended answer — FFmpeg
* can do whatever Media3 can, only slower — but it is the opposite of what this class
* said until #194, so it is written down rather than left to be re-derived.
*/
class AndroidDeviceCodecs private constructor(
private val hardwareEncodeMimes: Set<String>,
@@ -46,22 +51,62 @@ class AndroidDeviceCodecs private constructor(
fun get(): AndroidDeviceCodecs = cached ?: synchronized(this) { cached ?: probe().also { cached = it } }
private fun probe(): AndroidDeviceCodecs {
/**
* One entry of the platform's codec list, reduced to what the rules below read.
*
* The five booleans and the type list are the whole of what [capabilitiesFrom] needs, and
* none of them can be set on a `MediaCodecInfo` from a test: Robolectric ships
* `MediaCodecInfoBuilder`, but it has no `setIsAlias` and no `setCanonicalName`, which is
* exactly the objection #133 raised against reaching this code through
* `ShadowMediaCodecList`. That objection is about the shadow. It does not apply to a
* function that takes its own entry type, which is why this exists.
*/
internal data class CodecEntry(
val canonicalName: String,
val isAlias: Boolean,
val isEncoder: Boolean,
val isHardwareAccelerated: Boolean,
val isSoftwareOnly: Boolean,
val supportedTypes: List<String>,
)
/**
* The enumeration rules, over entries a caller chooses.
*
* [probe] is the only production caller and supplies the real codec list; a test supplies
* its own, which is the point — the two rules this class's KDoc calls out as easy to get
* wrong, the alias skip and the canonical-name dedup, are unreachable any other way.
*
* **`enumerate` returns a `Sequence`, deliberately.** The `runCatching` has to wrap the
* *iteration* rather than a list built before it, because a `MediaCodecInfo` whose
* properties throw does so partway through — and when that happens the codecs already read
* are kept. Taking a `List` here would move that throw outside the loop and silently turn a
* partial answer into an empty one. That behaviour predates this seam; a `List` parameter
* would have changed it as a side effect of a refactor.
*
* **An enumeration that fails answers restrictively, and that is deliberate.** The sets
* come back empty, and `"video/avc" in emptySet()` is `false`, so [canEncode] and
* [canDecode] both answer no and every job routes to FFmpeg. FFmpeg can do everything
* Media3 can, only slower, so refusing the hardware path is the safe reading of "we could
* not find out what this device supports". This used to log "assuming permissive", which
* described the opposite of what the code does.
*/
internal fun capabilitiesFrom(enumerate: () -> Sequence<CodecEntry>): AndroidDeviceCodecs {
val encoders = mutableSetOf<String>()
val decoders = mutableSetOf<String>()
val seen = mutableSetOf<String>()
runCatching {
MediaCodecList(MediaCodecList.REGULAR_CODECS).codecInfos.forEach { info ->
enumerate().forEach { entry ->
// Aliases point at the same underlying codec; counting both would
// double-count capabilities.
if (info.isAlias) return@forEach
if (!seen.add(info.canonicalName)) return@forEach
if (entry.isAlias) return@forEach
if (!seen.add(entry.canonicalName)) return@forEach
info.supportedTypes.forEach { mime ->
entry.supportedTypes.forEach { mime ->
if (!mime.startsWith("video/")) return@forEach
if (info.isEncoder) {
if (info.isHardwareAccelerated && !info.isSoftwareOnly) {
if (entry.isEncoder) {
if (entry.isHardwareAccelerated && !entry.isSoftwareOnly) {
encoders += mime
}
} else {
@@ -69,12 +114,32 @@ class AndroidDeviceCodecs private constructor(
}
}
}
}.onFailure { Log.w(TAG, "Codec enumeration failed; assuming permissive.", it) }
}.onFailure { Log.w(TAG, "Codec enumeration failed; routing everything to FFmpeg.", it) }
Log.i(TAG, "Hardware video encoders: $encoders")
return AndroidDeviceCodecs(encoders, decoders)
}
/**
* The thin edge: the real codec list, mapped onto [CodecEntry] one at a time.
*
* Lazily, so a property that throws does it inside [capabilitiesFrom]'s `runCatching` and
* on the entry that caused it — see that function's note on why the parameter is a
* `Sequence`.
*/
private fun probe(): AndroidDeviceCodecs = capabilitiesFrom {
MediaCodecList(MediaCodecList.REGULAR_CODECS).codecInfos.asSequence().map { info ->
CodecEntry(
canonicalName = info.canonicalName,
isAlias = info.isAlias,
isEncoder = info.isEncoder,
isHardwareAccelerated = info.isHardwareAccelerated,
isSoftwareOnly = info.isSoftwareOnly,
supportedTypes = info.supportedTypes.toList(),
)
}
}
/**
* `internal` rather than `private` so the cross-check test can ask what a [VideoCodec]
* means here and compare it with what [NAME_TO_MIME] says the same codec's names mean.
@@ -673,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 {
/**
@@ -221,12 +221,39 @@ object MediaProbe {
null
}
private fun readMediaInformation(path: String): FFprobeInfo? {
// ffmpeg-kit-next is compiled from Kotlin with private backing fields, so these have to go
// through the Java getters rather than property syntax.
val info: MediaInformation = FFprobeKit.getMediaInformation(path).getMediaInformation()
?: return null
/**
* The thin edge: spawn FFprobe, hand what it said to [ffprobeInfoFrom].
*
* Everything device-bound is on this line and the null check under it. What FFprobe *said* is a
* `MediaInformation`, which is an ordinary object over a `JSONObject` — so the reading of it is
* a decision a test can choose the inputs for, and it lives below rather than here.
*/
private fun readMediaInformation(path: String): FFprobeInfo? =
FFprobeKit.getMediaInformation(path).getMediaInformation()?.let(::ffprobeInfoFrom)
/**
* What FFprobe's answer means, as a function of the answer alone.
*
* `internal` for the same reason [Extracted] and [FFprobeInfo] are: a test cannot name it
* otherwise, and the JVM test source set is a friend of `main`.
*
* **JVM-safe, verified rather than assumed.** `javap` over the committed AAR's runtime jar:
* `MediaInformation(JSONObject, List<StreamInformation>, List<Chapter>)` and
* `StreamInformation(JSONObject)` are plain public constructors, and neither class's `<clinit>`
* touches the native library — so a test builds its own without `libffmpegkit` being present.
* That is the whole reason this split is worth making: `readMediaInformation` was 114 missed
* instructions and 24 missed branches, of which exactly one line needed a device.
*
* The subtle part is the **second argument to [containerFrom]**. `matroska,webm` is reported
* for both MKV and WebM — they share a demuxer — so the video codec is the only thing that
* separates them, and dropping it silently turns every VP9 WebM into an MKV. `containerFrom`
* has thirty-three covered branches of its own and none of them can notice that, because the
* mistake is at the call rather than in the callee.
*
* ffmpeg-kit-next is compiled from Kotlin with private backing fields, so these go through the
* Java getters rather than property syntax.
*/
internal fun ffprobeInfoFrom(info: MediaInformation): FFprobeInfo {
val streams = info.getStreams().orEmpty()
val video = streams.firstOrNull { it.getType() == "video" }
val audio = streams.firstOrNull { it.getType() == "audio" }
@@ -5,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()) }
@@ -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() },
)
}
@@ -0,0 +1,201 @@
package org.libremediaconverter.codec
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.model.VideoCodec
import org.robolectric.RobolectricTestRunner
/**
* The rules `AndroidDeviceCodecs.probe()` applies to the platform's codec list.
*
* ## Why this is not a third run of the #86/#133 spike
*
* #86 closed `probe()` as device-bound. #133 re-opened the question with
* `ShadowMediaCodecList` in hand and closed it again, for a reason that was right about what it
* was answering: `MediaCodecInfoBuilder` "has no `setIsAlias` and no `setCanonicalName`, so the
* alias skip and the canonical-name dedup — the two things the class's KDoc calls out as easy to
* get wrong — are not reachable through it."
*
* **That objection is about the shadow.** It does not apply to a function that takes its own entry
* type, which is what `capabilitiesFrom` now does. The half #133 named as unreachable is the half
* this file spends most of its cases on.
*
* ## What made the seam worth cutting, which is not coverage
*
* The `runCatching` fallback logged *"assuming permissive"* and returned empty sets — and empty
* sets are **restrictive**: `"video/avc" in emptySet()` is `false`, so `canEncode` and `canDecode`
* both answer no and every job routes to FFmpeg. The code was right and the message described the
* opposite of it. That is pinned below, so whichever reading a future change takes, it has to say
* so out loud.
*
* Robolectric only because `capabilitiesFrom` logs what it found; the rules themselves are pure.
*/
@RunWith(RobolectricTestRunner::class)
class CodecEnumerationTest {
/**
* The alias skip, in the one arrangement where it is observable — and finding that arrangement
* is the whole of this test.
*
* A first attempt listed the alias *after* the codec it aliases and passed with the skip
* deleted, because `canonicalName` is shared and the dedup below catches the second entry
* either way. The two rules overlap, so a fixture that does not separate them tests neither.
*
* What separates them is **order**. `MediaCodecInfo.getCanonicalName()` on an alias returns the
* underlying codec's name, so an alias arriving first claims that name in `seen` and has its
* own `supportedTypes` credited — and then the real codec is dropped by the dedup. Without the
* alias skip the device is described by whichever entry the platform happened to list first.
*
* That also says what the rule is worth. With a `Set` accumulator, an alias declaring the same
* types as its codec changes nothing whichever order they arrive in; the skip earns its place
* only when the two disagree, which is exactly when believing the wrong one matters.
*/
@Test
fun `an alias listed before the codec it aliases does not describe the device`() {
val codecs = capabilities(
entry("c2.qti.avc.encoder", encoder = true, types = listOf(HEVC), alias = true),
entry("c2.qti.avc.encoder", encoder = true, types = listOf(AVC)),
)
assertTrue("the real codec's types are the device's", codecs.canEncode(VideoCodec.H264))
assertFalse(
"an alias must not be credited with types the codec it aliases never claimed",
codecs.canEncode(VideoCodec.H265),
)
}
@Test
fun `two entries sharing a canonical name are read once`() {
val codecs = capabilities(
entry("c2.qti.avc.encoder", encoder = true, types = listOf(AVC)),
entry("c2.qti.avc.encoder", encoder = true, types = listOf(HEVC)),
)
assertEquals(setOf(AVC), codecs.hardwareEncoders())
}
/**
* Both halves of the hardware predicate, one arm at a time.
*
* A vendor may declare a codec hardware-accelerated *and* software-only; the class KDoc is
* explicit that the first flag "cannot be tested for correctness", so the second is what stops
* a mislabelled software encoder being treated as the fast path.
*/
@Test
fun `an encoder counts as hardware only when it is accelerated and not software-only`() {
assertEquals(
setOf(AVC),
capabilities(entry("hw", encoder = true, accelerated = true, types = listOf(AVC))).hardwareEncoders(),
)
assertEquals(
emptySet<String>(),
capabilities(entry("sw", encoder = true, accelerated = false, types = listOf(AVC))).hardwareEncoders(),
)
assertEquals(
"a codec claiming both must not be trusted as hardware",
emptySet<String>(),
capabilities(
entry("both", encoder = true, accelerated = true, softwareOnly = true, types = listOf(AVC)),
).hardwareEncoders(),
)
}
/**
* Decoders are collected regardless of the hardware flags, and that asymmetry is the design.
*
* `canDecode` asks whether the platform can read the input at all — a software decoder answers
* that as well as a hardware one. `canEncode` asks whether the *fast path* exists, which is a
* different question and why only encoders are filtered.
*/
@Test
fun `a software decoder still counts as something the platform can read`() {
val codecs = capabilities(
entry(
"c2.android.avc.decoder",
encoder = false,
accelerated = false,
softwareOnly = true,
types = listOf(AVC),
),
)
assertTrue(codecs.canDecode("h264"))
}
@Test
fun `audio types are ignored on both sides`() {
val codecs = capabilities(
entry("aac.encoder", encoder = true, accelerated = true, types = listOf("audio/mp4a-latm")),
entry("aac.decoder", encoder = false, types = listOf("audio/mp4a-latm")),
)
assertEquals(emptySet<String>(), codecs.hardwareEncoders())
// Not "the platform cannot decode AAC" -- `canDecode` is asked about *video* codec names,
// and an unknown name is answered permissively. The point is that nothing audio reached
// either set.
assertTrue("an unknown name stays permissive", codecs.canDecode("something-nobody-named"))
}
/**
* The failure fallback, pinned as the restrictive answer it actually is.
*
* #194 decided this rather than assuming it: the code stays, the message changes. If a later
* change wants the permissive reading its old log line described, this test is what makes that
* a decision instead of a drift.
*/
@Test
fun `an enumeration that fails sends every job to FFmpeg`() {
val codecs = AndroidDeviceCodecs.capabilitiesFrom { error("MediaCodecList exploded") }
assertFalse("a failed enumeration must not claim a hardware encoder", codecs.canEncode(VideoCodec.H264))
assertFalse(codecs.canDecode("h264"))
assertEquals(emptySet<String>(), codecs.hardwareEncoders())
}
/**
* A list that throws partway keeps what it already read.
*
* This predates the seam — `runCatching` has always wrapped the iteration rather than a list
* built before it — and it is asserted here because the seam is where it could quietly have
* been lost. Taking a `List` instead of a `Sequence` would move the throw outside the loop and
* turn this partial answer into an empty one, with no test to notice.
*/
@Test
fun `codecs read before a failing entry are kept`() {
val codecs = AndroidDeviceCodecs.capabilitiesFrom {
sequence {
yield(entry("good", encoder = true, accelerated = true, types = listOf(AVC)))
error("the sixth codec's properties threw")
}
}
assertEquals(setOf(AVC), codecs.hardwareEncoders())
}
private fun capabilities(vararg entries: AndroidDeviceCodecs.Companion.CodecEntry) =
AndroidDeviceCodecs.capabilitiesFrom { entries.asSequence() }
private fun entry(
canonicalName: String,
encoder: Boolean,
accelerated: Boolean = true,
softwareOnly: Boolean = false,
alias: Boolean = false,
types: List<String>,
) = AndroidDeviceCodecs.Companion.CodecEntry(
canonicalName = canonicalName,
isAlias = alias,
isEncoder = encoder,
isHardwareAccelerated = accelerated,
isSoftwareOnly = softwareOnly,
supportedTypes = types,
)
private companion object {
const val AVC = "video/avc"
const val HEVC = "video/hevc"
}
}
@@ -0,0 +1,180 @@
package org.libremediaconverter.convert
import android.app.Application
import androidx.media3.common.util.UnstableApi
import androidx.work.OneTimeWorkRequestBuilder
import androidx.work.WorkInfo
import androidx.work.WorkManager
import androidx.work.workDataOf
import kotlinx.coroutines.Dispatchers
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.join.JoinState
import org.libremediaconverter.join.JoinViewModel
import org.libremediaconverter.model.InputProbe
import org.libremediaconverter.work.ConcatWorker
import org.libremediaconverter.work.ConversionWorker
import org.libremediaconverter.work.JobTags
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment
import java.util.UUID
import java.util.concurrent.TimeUnit
/**
* That `cancel()` cancels the job, on both screens.
*
* ## Why this was missing, which is the interesting part
*
* Both `cancel()` methods are one line — `activeWorkId?.let(workManager::cancelWorkById)` — and
* **JaCoCo reports every line of both as covered**. `SettingsEditsTest`'s
* `cancelling with no active job does nothing rather than throwing` runs the method, and its own
* comment names which half it drives: "`activeWorkId?.let(...)` -- the null side". The other side
* had never been entered, and `JoinViewModel.cancel()` had no test at all.
*
* So no line-level coverage filter could see this. What surfaces it is a method-level read —
* `mi=11, ci=7, mb=1, cb=1` on both — a covered method with an arm nothing takes. That is the
* second of the two filters #194 records, and this is the gap that argued for it.
*
* The affordance tests are not this. `ConverterStateAffordancesTest` and `JoinStateAffordancesTest`
* click `TestTags.CANCEL` and assert the *action* fires into a stub; `ScreenWiringTest` asserts the
* action calls `viewModel.cancel()`. Both halves were pinned and the join between them was not, so
* nothing in 584 tests connected the button to WorkManager.
*
* ## Why the job is enqueued with a delay
*
* The test WorkManager runs on a `SynchronousExecutor`, so an ordinary request finishes inline —
* which is exactly why only the null half was ever covered: by the time a test could call
* `cancel()`, `convert()`'s job was already terminal. `setInitialDelay` is what `TestScheduler`
* honours, so the job sits in `ENQUEUED` until the test lets it go, and it never does.
*
* **Production never sets a delay**, so the request is built here rather than through
* `ConversionWorker.request`. The *state* is not synthetic: `ENQUEUED` at `runAttemptCount == 0` is
* what every job passes through before the scheduler picks it up, `Reattachment.choose` ranks it
* `QUEUED`, and `conversionStateFrom` maps it to `Converting(input, 0)`. The delay changes how long
* the job stays in a real state, not which state it is in.
*
* ## What is asserted, and in which order
*
* WorkManager's own record first, then the screen. The screen alone would be a weaker claim than it
* looks: `CANCELLED` maps to `Idle` for a reattached job, and `Idle` is also where a ViewModel that
* did nothing at all would sit.
*/
@UnstableApi
@RunWith(RobolectricTestRunner::class)
class CancelReachesWorkManagerTest {
private lateinit var app: Application
private lateinit var workManager: WorkManager
@Before
fun setUp() {
app = RuntimeEnvironment.getApplication()
ConversionDependencies.publisher = { RecordingPublisher(app) }
ConversionDependencies.probe = { _, _ -> InputProbe() }
installTestWorkManager(app, workDataOf())
workManager = WorkManager.getInstance(app)
}
@After
fun tearDown() {
ConversionDependencies.reset()
}
@Test
fun `cancelling a queued conversion cancels that job`() {
val id = enqueueQueuedConversion()
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
awaitState(viewModel.state, "Converting") { it is ConversionState.Converting }
viewModel.cancel()
assertEquals(
"Cancel must reach WorkManager, not just the screen",
WorkInfo.State.CANCELLED,
stateOf(id),
)
awaitState(viewModel.state, "Idle") { it is ConversionState.Idle }
}
@Test
fun `cancelling a queued join cancels that job`() {
val id = enqueueQueuedJoin()
val viewModel = JoinViewModel(app, Dispatchers.Unconfined)
awaitState(viewModel.state, "Joining") { it is JoinState.Joining }
viewModel.cancel()
assertEquals(
"Cancel must reach WorkManager, not just the screen",
WorkInfo.State.CANCELLED,
stateOf(id),
)
awaitState(viewModel.state, "Idle") { it is JoinState.Idle }
}
/**
* The negative that bounds both: cancelling must cancel the job the screen is showing, and only
* that one.
*
* Without this, `cancel()` could cancel everything in the queue — `cancelAllWork()` in place of
* `cancelWorkById(activeWorkId)` — and both tests above would still pass.
*/
@Test
fun `cancelling one conversion leaves another queued job alone`() {
val bystander = enqueueQueuedConversion(displayName = "beach.mp4")
val id = enqueueQueuedConversion(displayName = "holiday.mp4")
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
val converting = awaitState(viewModel.state, "Converting") { it is ConversionState.Converting }
val onScreen = (converting as ConversionState.Converting).input.displayName
viewModel.cancel()
// Which of the two the ViewModel reattached to is the query's business, not this test's --
// the comparator leaves queued jobs tied deliberately, per Reattachment's ordering notes.
// So assert the shape rather than the identity: exactly one is cancelled, and the other is
// untouched.
val cancelled = listOf(id, bystander).filter { stateOf(it) == WorkInfo.State.CANCELLED }
assertEquals(
"exactly one job may be cancelled, with $onScreen on screen",
1,
cancelled.size,
)
}
private fun stateOf(id: UUID): WorkInfo.State =
requireNotNull(workManager.getWorkInfoById(id).get()) { "no WorkInfo for $id" }.state
/**
* A conversion sitting in the queue, which is where every job starts.
*
* Built by hand rather than through `ConversionWorker.request` for the reason in the class
* KDoc; the display-name tag is included because `reattach()` reads it for the file card, and a
* job without one would exercise the `UNKNOWN_INPUT_NAME` fallback instead of this test's
* subject.
*/
private fun enqueueQueuedConversion(displayName: String = "holiday.mp4"): UUID {
val request = OneTimeWorkRequestBuilder<ConversionWorker>()
.addTag(JobTags.displayName(displayName))
.setInitialDelay(QUEUE_HOLD_HOURS, TimeUnit.HOURS)
.build()
workManager.enqueue(request).result.get()
return request.id
}
private fun enqueueQueuedJoin(inputCount: Int = 2): UUID {
val request = OneTimeWorkRequestBuilder<ConcatWorker>()
.addTag(JobTags.inputCount(inputCount))
.setInitialDelay(QUEUE_HOLD_HOURS, TimeUnit.HOURS)
.build()
workManager.enqueue(request).result.get()
return request.id
}
private companion object {
/** Long enough that `TestScheduler` never releases the job during a test run. */
const val QUEUE_HOLD_HOURS = 1L
}
}
@@ -0,0 +1,192 @@
package org.libremediaconverter.convert
import com.arthenica.ffmpegkit.MediaInformation
import com.arthenica.ffmpegkit.StreamInformation
import org.json.JSONObject
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertNull
import org.junit.Assert.assertTrue
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.model.Container
import org.robolectric.RobolectricTestRunner
/**
* What FFprobe's answer means, read as a function of the answer alone.
*
* `readMediaInformation` was 114 missed instructions and 24 missed branches — the second-biggest
* block on the wave-4 report — of which **exactly one line needed a device**:
*
* ```kotlin
* FFprobeKit.getMediaInformation(path).getMediaInformation()
* ```
*
* Everything after it reads an ordinary object. `javap` over the committed AAR's runtime jar:
* `MediaInformation(JSONObject, List<StreamInformation>, List<Chapter>)` and
* `StreamInformation(JSONObject)` are plain public constructors, and neither class's `<clinit>`
* loads the native library — so the fixtures below are built without `libffmpegkit` present.
*
* ## The one that matters
*
* `containerFrom(formatName, video?.getCodec())`. FFprobe reports `matroska,webm` for **both** MKV
* and WebM, because they share a demuxer, so the video codec is the only thing separating them.
* `containerFrom` has thirty-three covered branches of its own and not one of them can notice the
* argument being dropped — the mistake would be at the call, not in the callee, and every existing
* `containerFrom` test would stay green while every VP9 WebM quietly became an MKV.
*
* Robolectric only for `org.json`, which is a stub in a plain JVM test.
*/
@RunWith(RobolectricTestRunner::class)
class FFprobeMappingTest {
@Test
fun `the video codec decides between matroska and webm`() {
assertEquals(
Container.WEBM,
MediaProbe.ffprobeInfoFrom(info("matroska,webm", stream("video", "vp9"))).container,
)
assertEquals(
Container.MKV,
MediaProbe.ffprobeInfoFrom(info("matroska,webm", stream("video", "h264"))).container,
)
}
/**
* The same format name with no video stream at all, which is what makes the case above about
* the *argument* rather than about the format string.
*/
@Test
fun `a matroska container with no video track cannot be told from webm and is not guessed`() {
val read = MediaProbe.ffprobeInfoFrom(info("matroska,webm", stream("audio", "opus")))
assertEquals(Container.MKV, read.container)
assertNull(read.videoCodec)
}
@Test
fun `the first stream of each type wins`() {
val read = MediaProbe.ffprobeInfoFrom(
info(
"mov,mp4,m4a,3gp,3g2,mj2",
stream("video", "h264", width = 1920, height = 1080),
stream("video", "hevc", width = 640, height = 480),
stream("audio", "aac"),
stream("audio", "mp3"),
),
)
assertEquals("h264", read.videoCodec)
assertEquals("aac", read.audioCodec)
assertEquals(1920, read.width)
assertEquals(1080, read.height)
}
/**
* Dimensions come from the stream the codec came from, not from whichever stream has some.
*
* The fixture is deliberately awkward: the chosen video stream carries **no** dimensions and a
* later one does. That is a real shape — FFprobe omits `width`/`height` for a stream it could
* not measure — and it is the only arrangement that separates the two readings.
*
* A first version of this file asserted the dimensions inside the case above, where the chosen
* stream was also the first one carrying any. Replacing `video?.getWidth()` with
* `streams.firstNotNullOfOrNull { it.getWidth() }` gave the same answer there and **the
* mutation survived**. It reddens here.
*/
@Test
fun `a video stream with no dimensions reports none rather than borrowing another stream's`() {
val read = MediaProbe.ffprobeInfoFrom(
info(
"mov,mp4,m4a,3gp,3g2,mj2",
stream("video", "h264"),
stream("video", "hevc", width = 640, height = 480),
),
)
assertEquals("h264", read.videoCodec)
assertEquals(0, read.width)
assertEquals(0, read.height)
}
/**
* Stream order is the file's, not a promise. An audio-first container must read the same as a
* video-first one.
*/
@Test
fun `an audio track listed first does not become the video track`() {
val read = MediaProbe.ffprobeInfoFrom(
info("mov,mp4,m4a,3gp,3g2,mj2", stream("audio", "aac"), stream("video", "h264")),
)
assertEquals("h264", read.videoCodec)
assertEquals("aac", read.audioCodec)
}
@Test
fun `a duration in seconds becomes milliseconds`() {
assertEquals(12_345L, MediaProbe.ffprobeInfoFrom(info("mp4", duration = "12.345")).durationMs)
}
/**
* Both ways a duration can be absent, and neither may throw.
*
* FFprobe reports `"N/A"` for a stream it could not measure, and omits the key entirely for
* some containers. `toDoubleOrNull` is what keeps the second from being an exception on the
* file-pick path, where there is no user-visible failure to report it as.
*/
@Test
fun `a duration that is not a number is no duration rather than a crash`() {
assertEquals(0L, MediaProbe.ffprobeInfoFrom(info("mp4", duration = "N/A")).durationMs)
assertEquals(0L, MediaProbe.ffprobeInfoFrom(info("mp4", duration = null)).durationMs)
}
@Test
fun `a file with no streams reports nothing rather than defaults that look measured`() {
val read = MediaProbe.ffprobeInfoFrom(info("mp4"))
assertNull(read.videoCodec)
assertNull(read.audioCodec)
assertEquals(0, read.width)
assertEquals(0, read.height)
}
@Test
fun `an image format is reported as one`() {
assertTrue(MediaProbe.ffprobeInfoFrom(info("png_pipe", stream("video", "png"))).isImage)
assertFalse(MediaProbe.ffprobeInfoFrom(info("mp4", stream("video", "h264"))).isImage)
}
private fun stream(type: String, codec: String, width: Int? = null, height: Int? = null) = StreamInformation(
JSONObject().apply {
put(StreamInformation.KEY_TYPE, type)
put(StreamInformation.KEY_CODEC, codec)
width?.let { put(StreamInformation.KEY_WIDTH, it) }
height?.let { put(StreamInformation.KEY_HEIGHT, it) }
},
)
/**
* The format properties are **nested** under `"format"`, which is how FFprobe reports them and
* what `MediaInformation` reads: `getFormat()` resolves through `getStringFormatProperty`, not
* off the top-level object. A first version of this helper put the keys at the top level and
* every format-dependent case failed with a null container, which is worth recording here so
* the next fixture does not have to rediscover it.
*
* Streams are the other half and are *not* nested — they come from the constructor argument.
*/
private fun info(formatName: String, vararg streams: StreamInformation, duration: String? = "1.0") =
MediaInformation(
JSONObject().apply {
put(
MediaInformation.KEY_FORMAT_PROPERTIES,
JSONObject().apply {
put(MediaInformation.KEY_FORMAT, formatName)
duration?.let { put(MediaInformation.KEY_DURATION, it) }
},
)
},
streams.toList(),
emptyList(),
)
}
@@ -51,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
@@ -205,6 +205,34 @@ class FileCardTest {
assertNoRow("Length")
}
/**
* A video the app knows a great deal about and cannot name the container of.
*
* Not an edge case. `InputProbe.container`'s own KDoc says `MediaExtractor` cannot report a
* container at all -- it comes from FFprobe -- so any run where FFprobe did not answer produces
* exactly this: real codec, real dimensions, real duration, `container = null`.
*
* **The twin was already tested and this one was not**, which is the argument for adding it.
* `FileCard` renders `probe.container?.label ?: "Unknown"` twice, once in the `AUDIO_ONLY`
* branch (`ConverterScreen.kt:660`) and once in the `VIDEO` branch (`:668`), and
* `an audio-only file nothing else could describe degrades one row at a time` drives only the
* first. Same expression, same fallback, one kind covered. That asymmetry is the same one
* `CLAUDE.md` records for including `ContainerCapabilities:94`.
*
* The other rows are asserted alongside so this is not a copy of the audio-only case: there,
* everything is unknown at once; here, one field is missing from a probe that is otherwise
* complete, and the rest must be unaffected by it.
*/
@Test
fun `a video file whose container nothing identified says so and keeps its other rows`() {
setFileCard(input(probe = VIDEO_PROBE.copy(container = null)))
assertRow("Container", "Unknown")
assertRow("Video", "${VideoCodec.H264.label} · 1920×1080")
assertRow("Audio", AudioCodec.AAC.label)
assertRow("Length", "1:30")
}
/**
* The row is one node, not a label node beside a value node. A test matching on `"Container"`
* alone would pass against either shape.
@@ -0,0 +1,141 @@
package org.libremediaconverter.convert
import android.app.Activity
import android.content.Intent
import android.net.Uri
import androidx.activity.ComponentActivity
import androidx.compose.ui.test.assertIsDisplayed
import androidx.compose.ui.test.junit4.v2.createAndroidComposeRule
import androidx.compose.ui.test.onNodeWithTag
import androidx.compose.ui.test.performClick
import androidx.media3.common.util.UnstableApi
import androidx.work.Data
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Before
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.join.JoinScreen
import org.libremediaconverter.model.InputProbe
import org.libremediaconverter.ui.TestTags
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment
import org.robolectric.Shadows.shadowOf
import org.robolectric.shadows.ShadowActivity
/**
* The launcher layer above the `ScreenContent` seam — registered, and until now never resulted.
*
* ## The hazard this exists for
*
* `ConversionViewModel.onInputPicked(uri: Uri)` and `.save(destination: Uri)` are **both
* `(Uri) -> Unit`**, so swapping the two launcher callbacks at `ConverterScreen.kt:70` and `:83`
* compiles, renders, and passes the entire suite. Picking a file would attempt a save to it, and
* choosing a destination would load it as input.
*
* That is precisely the defect class `ScreenWiringTest` exists for, on the one pair it declines to
* cover: it drives `converterActions` directly and says the launcher-backed actions stay
* parameters. Correct for the `actions` seam, and it leaves the edge above that seam unpinned.
*
* Join's equivalents (`JoinScreen.kt:45`, `:55`) are `List<Uri>` and `Uri`, so they are **not**
* transposable and need no such test. The picker filter is a different matter and is covered below
* for both screens.
*
* ## The two mechanics, verified before the assertions were written
*
* Neither is used anywhere else in the suite, so both were spiked first:
*
* - **Reading what was launched** — `shadowOf(activity).nextStartedActivityForResult`, which returns
* the `Intent` with its `EXTRA_MIME_TYPES` intact.
* - **Delivering a result** — `shadowOf(activity).receiveResult(...)`, which reaches
* `ComponentActivity`'s `ActivityResultRegistry` and fires the `rememberLauncherForActivityResult`
* callback.
*
* `createAndroidComposeRule`, as `AdaptiveShellTest` uses and for the reason it gives: the screens
* compose real ViewModels through `viewModel()`, and the plain rule supplies no `ViewModelStoreOwner`.
*/
@UnstableApi
@RunWith(RobolectricTestRunner::class)
class LauncherWiringTest {
@get:Rule
val composeRule = createAndroidComposeRule<ComponentActivity>()
@Before
fun setUp() {
val app = RuntimeEnvironment.getApplication()
installTestWorkManager(app, Data.EMPTY)
// The real screen composes a real ViewModel; neither test here is about probing.
ConversionDependencies.probe = { _, _ -> InputProbe() }
}
@After
fun tearDown() = ConversionDependencies.reset()
/**
* The transposition guard. A picked file has to reach `onInputPicked`, which is observable as
* the screen arriving at `Ready` with the file card showing — `save()` from `Idle` returns at
* its own guard and leaves nothing behind.
*/
@Test
fun `a picked document is loaded as input rather than saved to`() {
composeRule.setContent { ConverterScreen() }
composeRule.onNodeWithTag(TestTags.Converter.CHOOSE_FILE).performClick()
deliver(Uri.parse("content://test/holiday.mkv"))
composeRule.onNodeWithTag(TestTags.Converter.FILE_CARD_NAME).assertIsDisplayed()
}
/**
* `ConverterScreen.kt:65-67` records why the all-types wildcard is load-bearing rather than lazy:
*
* > the picker is images and video only, offers no audio at all, and will not reliably surface
* > .mkv/.flac/.webm
*
* Narrowing it would make every audio conversion unreachable from the file picker, and nothing
* would have gone red. (The literal is spelled only in the assertion below: a KDoc cannot
* contain it, because the wildcard's second half closes the comment.)
*/
@Test
fun `the converter picker asks for every type, not just the ones a photo picker offers`() {
composeRule.setContent { ConverterScreen() }
composeRule.onNodeWithTag(TestTags.Converter.CHOOSE_FILE).performClick()
val intent = launched().intent
assertEquals(Intent.ACTION_OPEN_DOCUMENT, intent.action)
assertEquals(listOf("*/*"), intent.getStringArrayExtra(Intent.EXTRA_MIME_TYPES)?.toList())
}
@Test
fun `the join picker asks for video and accepts more than one file`() {
composeRule.setContent { JoinScreen() }
composeRule.onNodeWithTag(TestTags.Join.CHOOSE_FILES).performClick()
val intent = launched().intent
assertEquals(Intent.ACTION_OPEN_DOCUMENT, intent.action)
assertEquals(listOf("video/*"), intent.getStringArrayExtra(Intent.EXTRA_MIME_TYPES)?.toList())
// A join of one file is not a join; the contract is what asks for several.
assertEquals(true, intent.getBooleanExtra(Intent.EXTRA_ALLOW_MULTIPLE, false))
}
private fun launched(): ShadowActivity.IntentForResult {
composeRule.waitForIdle()
return requireNotNull(shadowOf(composeRule.activity).nextStartedActivityForResult) {
"nothing was launched for a result"
}
}
private fun deliver(uri: Uri) {
val started = launched()
shadowOf(composeRule.activity).receiveResult(
started.intent,
Activity.RESULT_OK,
Intent().setData(uri),
)
composeRule.waitForIdle()
}
}
@@ -0,0 +1,213 @@
package org.libremediaconverter.convert
import android.app.Application
import android.net.Uri
import androidx.media3.common.util.UnstableApi
import androidx.work.Data
import androidx.work.ListenableWorker
import androidx.work.testing.TestListenableWorkerBuilder
import androidx.work.workDataOf
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.runBlocking
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertNotNull
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.join.JoinState
import org.libremediaconverter.join.JoinViewModel
import org.libremediaconverter.model.ConversionRequest
import org.libremediaconverter.model.EnginePreference
import org.libremediaconverter.model.InputProbe
import org.libremediaconverter.model.OutputFormat
import org.libremediaconverter.work.ConcatWorker
import org.libremediaconverter.work.ConversionWorker
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment
import java.io.File
import java.util.UUID
/**
* A failure that says nothing still has to say something.
*
* Three sites, all `ci == 0` before this file, and all the same rule:
*
* ```
* work/ConversionWorker.kt:316 cause.message ?: GENERIC_FAILURE_MESSAGE
* convert/ConversionViewModel.kt:631 e.message ?: SAVE_FAILED_MESSAGE
* join/JoinViewModel.kt:416 e.message ?: SAVE_FAILED_MESSAGE
* ```
*
* Every existing test throws *with* a message, so the right-hand side had never been evaluated
* anywhere in the suite. A `Throwable` carrying none is not exotic — `RuntimeException()`,
* `IOException()` and most platform exceptions raised without an argument all have a null message.
*
* ## Held in one class, against the ticket's suggestion
*
* #193 proposed putting each case beside the behaviour it neighbours. They are together instead,
* because they are one rule at three layers and because the trap below has to be explained once
* rather than three times. `FailedSaveRetryTest` sets the precedent for both ViewModels in one
* file; this extends it by one worker.
*
* ## The trap, which is why the worker case asserts what it does
*
* `ConversionStateMappingTest`'s *"a failure with nothing said still says something"* looks like it
* already covers the worker site. It does not: it drives the **read** side, `map(FAILED, Data.EMPTY)`,
* and that side has a fallback of its own (`ConversionViewModel.kt:147-149`):
*
* ```kotlin
* update.outputData.getString(ConversionWorker.KEY_ERROR)
* ?.takeIf { it.isNotBlank() }
* ?: ConversionWorker.GENERIC_FAILURE_MESSAGE
* ```
*
* So mutating the worker's fallback to `.orEmpty()` writes `KEY_ERROR to ""`, and the ViewModel
* turns that straight back into the same constant. **A test asserting on the resulting `Failed`
* state stays green under the mutation**, which is most likely why the write-side fallback survived
* three waves of test work. The worker case therefore reads `KEY_ERROR` off the worker's own
* `Result`, before anything downstream can repair it.
*
* The two save cases have no such second line: both write `_state.value` directly, so the state is
* the right thing to assert there.
*/
@UnstableApi
@RunWith(RobolectricTestRunner::class)
class MessagelessFailureTest {
private lateinit var app: Application
private lateinit var publisher: RecordingPublisher
private lateinit var staged: File
@Before
fun setUp() {
app = RuntimeEnvironment.getApplication()
publisher = RecordingPublisher(app)
ConversionDependencies.publisher = { publisher }
ConversionDependencies.probe = { _, _ -> InputProbe() }
staged = publisher.createStagingFile("holiday.mp4").apply { writeBytes(ByteArray(4096)) }
}
@After
fun tearDown() {
ConversionDependencies.reset()
}
/**
* The engine gives up without saying why, which is what a native crash looks like from here.
*
* Asserted on the worker's own output `Data` rather than on a screen — see the class KDoc.
*/
@Test
fun `a conversion that fails without a message still reports one`() {
installTestWorkManager(app, Data.EMPTY)
ConversionDependencies.software = { MessagelessTranscoder }
val result = runBlocking { failingWorker().doWork() }
assertTrue("the job must fail rather than retry, got $result", result is ListenableWorker.Result.Failure)
assertEquals(
"a failure with no message must still put something on screen",
ConversionWorker.GENERIC_FAILURE_MESSAGE,
(result as ListenableWorker.Result.Failure).outputData.getString(ConversionWorker.KEY_ERROR),
)
}
@Test
fun `a save that fails without a message still reports one`() {
installTestWorkManager(app, conversionOutput())
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
viewModel.onInputPicked(Uri.parse("content://test/holiday.mkv"))
awaitState(viewModel.state, "Ready") { it is ConversionState.Ready }
viewModel.convert()
awaitState(viewModel.state, "Converted") { it is ConversionState.Converted }
publisher.publishFailure = RuntimeException()
viewModel.save(DESTINATION)
val failed = awaitState(viewModel.state, "Failed") { it is ConversionState.Failed } as ConversionState.Failed
assertEquals(SAVE_FAILED_MESSAGE, failed.message)
// The handle travels even on the wordless path. Without this, a fallback that also dropped
// `pending` would pass -- and the file would be unreachable from the screen that just said
// the save failed.
assertNotNull("a wordless failure must still offer the file again", failed.retry)
}
@Test
fun `a join save that fails without a message still reports one`() {
installTestWorkManager(app, joinOutput())
val viewModel = JoinViewModel(app, Dispatchers.Unconfined)
viewModel.onInputsPicked(listOf(Uri.parse("content://test/a.mp4"), Uri.parse("content://test/b.mp4")))
awaitState(viewModel.state, "Ready") { it is JoinState.Ready }
viewModel.join()
awaitState(viewModel.state, "Joined") { it is JoinState.Joined }
publisher.publishFailure = RuntimeException()
viewModel.save(DESTINATION)
val failed = awaitState(viewModel.state, "Failed") { it is JoinState.Failed } as JoinState.Failed
assertEquals(SAVE_FAILED_MESSAGE, failed.message)
assertNotNull("a wordless failure must still offer the file again", failed.retry)
}
/**
* `FORCE_SOFTWARE` so the failure comes straight out of `runFFmpeg`.
*
* `AUTO` would enter `runMedia3OrFallBack`, whose catch runs the job a second time in software
* — the same exception would arrive, but through a path this test is not about and which
* `HardwareFallbackTest` already owns.
*/
private fun failingWorker(): ConversionWorker {
val spec = OutputFormat.MP4_H265.spec
return TestListenableWorkerBuilder<ConversionWorker>(
context = app,
inputData = workDataOf(
ConversionWorker.KEY_INPUT_URI to "file:///tmp/holiday.mp4",
ConversionWorker.KEY_DISPLAY_NAME to "holiday.mp4",
ConversionWorker.KEY_CONTAINER to spec.container.name,
ConversionWorker.KEY_VIDEO_CODEC to spec.videoCodec.name,
ConversionWorker.KEY_AUDIO_CODEC to spec.audioCodec.name,
ConversionWorker.KEY_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name,
),
runAttemptCount = 0,
).setId(JOB_ID).build()
}
private fun conversionOutput() = workDataOf(
ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath,
ConversionWorker.KEY_SUGGESTED_NAME to SUGGESTED_NAME,
ConversionWorker.KEY_MIME_TYPE to JOB_MIME_TYPE,
)
private fun joinOutput() = workDataOf(
ConcatWorker.KEY_OUTPUT_PATH to staged.absolutePath,
ConcatWorker.KEY_SUGGESTED_NAME to SUGGESTED_NAME,
ConcatWorker.KEY_MIME_TYPE to JOB_MIME_TYPE,
)
private companion object {
val DESTINATION: Uri = Uri.parse("content://test/destination.mp4")
val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-00000000019a")
const val SUGGESTED_NAME = "holiday.mp4"
const val JOB_MIME_TYPE = "video/mp4"
}
}
/**
* An engine that gives up without saying why.
*
* `RuntimeException()` rather than a subclass with a blank message: `Throwable.message` is *null*
* here, which is the case the elvis exists for. A blank-but-present message takes the left-hand
* side and is a different path — `ConversionStateMappingTest` covers that one, on the read side.
*/
@UnstableApi
private object MessagelessTranscoder : SoftwareTranscoder {
override suspend fun run(
request: ConversionRequest,
inputPath: String,
output: File,
durationMs: Long,
onProgress: (Int) -> Unit,
): Unit = throw RuntimeException()
}
@@ -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")
}
}
@@ -190,6 +190,30 @@ class FFmpegCommandBuilderTest {
assertPair(cmd(OutputFormat.M4A_AAC), "-b:a", "192k")
}
/**
* Turning audio off, which the Advanced picker offers and nothing had ever built a command for.
*
* `audioArgs`' `Drop` arm was `ci == 0`. The suite's only `-an` assertion is in
* `gif generates a palette to avoid banding and drops audio`, and that one comes from the image
* path (`FFmpegCommandBuilder.kt:79`/`:90`), which emits `-an` directly and never reaches
* `audioArgs`. Two sites, one string, one tested.
*
* It is a live path rather than defensive code: `AdvancedPicker` renders all of
* `AudioCodec.entries` including `NONE`, `ContainerCapabilities.validate` permits audio-off
* whenever the input has video, and MKV routes the job to FFmpeg.
*
* Both halves are asserted. `-an` alone would still pass if the arm fell through to the `else`
* and emitted an AAC encoder beside it -- a file that is silent because the flag won, carrying
* an encoder nobody asked for.
*/
@Test
fun `turning audio off drops the track instead of encoding one`() {
val args = cmd(OutputSpec(Container.MKV, VideoCodec.H264, AudioCodec.NONE))
assertTrue("audio turned off must emit -an, got $args", args.contains("-an"))
assertFalse("a dropped track must not also carry an encoder, got $args", args.contains("-c:a"))
}
@Test
fun `audio only formats never carry a video encoder`() {
listOf(OutputFormat.MP3, OutputFormat.FLAC, OutputFormat.WAV, OutputFormat.OPUS)
@@ -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 })
}
@@ -0,0 +1,89 @@
package org.libremediaconverter.ui.theme
import androidx.compose.material3.ColorScheme
import androidx.compose.material3.MaterialTheme
import androidx.compose.ui.test.junit4.v2.createComposeRule
import org.junit.Assert.assertEquals
import org.junit.Assert.assertNotEquals
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.robolectric.RobolectricTestRunner
import org.robolectric.annotation.Config
/**
* The theme called the way the app calls it: with no arguments at all.
*
* [ThemeColorSchemeTest] resolves every branch of the `when` and always passes `darkTheme`
* explicitly, so the `$default` bridge is never entered and **`isSystemInDarkTheme()` is never
* called**. `MainActivity.kt:79` is its only default-argument caller and does not execute on the
* JVM, which left the app's actual call shape the one nothing exercised —
* `LibreMediaConverterTheme` reported `mi=21, mb=6, cb=12` at method level.
*
* ## Not #68
*
* #68 is about the two **unreachable** arms, `DarkColorScheme` and `LightColorScheme`, which cannot
* run because `dynamicColor` is always `true` and nothing can flip it. That is an open product
* decision. This is the reachable half — whether the default follows the system — and closing it
* does not close that.
*
* ## Why the assertion compares schemes rather than reading a number
*
* A luminance threshold would be a guess about the device palette. What is asserted instead is that
* the no-argument call resolves to **the same scheme** an explicit `darkTheme` of the matching
* value does, and a different one from its opposite. That holds whatever palette the platform
* hands back, and it is exactly the claim: the default reads the system rather than picking a side.
*
* Both schemes are resolved in one composition because `setContent` may be called once per test.
*/
@RunWith(RobolectricTestRunner::class)
class ThemeFollowsSystemTest {
@get:Rule
val composeRule = createComposeRule()
@Test
@Config(qualifiers = "+night")
fun `with no arguments the theme follows a system in dark mode`() {
val resolved = resolve()
assertEquals("the default must resolve what darkTheme = true does", resolved.dark, resolved.bare)
assertNotEquals(resolved.light, resolved.bare)
}
@Test
@Config(qualifiers = "+notnight")
fun `with no arguments the theme follows a system in light mode`() {
val resolved = resolve()
assertEquals("the default must resolve what darkTheme = false does", resolved.light, resolved.bare)
assertNotEquals(resolved.dark, resolved.bare)
}
/**
* The three colours are read together as one value, because any single one could coincide
* between the two schemes on some palette while the schemes themselves differ. Background is
* what dark mode is chiefly about; primary and surface are along to make a coincidence
* implausible rather than merely unlikely.
*/
private data class Fingerprint(val background: Long, val primary: Long, val surface: Long)
private fun ColorScheme.fingerprint() =
Fingerprint(background.value.toLong(), primary.value.toLong(), surface.value.toLong())
private class Resolved(val bare: Fingerprint, val dark: Fingerprint, val light: Fingerprint)
private fun resolve(): Resolved {
lateinit var bare: Fingerprint
lateinit var dark: Fingerprint
lateinit var light: Fingerprint
composeRule.setContent {
// No arguments — the call MainActivity makes, and the one nothing exercised.
LibreMediaConverterTheme { bare = MaterialTheme.colorScheme.fingerprint() }
LibreMediaConverterTheme(darkTheme = true) { dark = MaterialTheme.colorScheme.fingerprint() }
LibreMediaConverterTheme(darkTheme = false) { light = MaterialTheme.colorScheme.fingerprint() }
}
composeRule.waitForIdle()
return Resolved(bare, dark, light)
}
}
@@ -21,8 +21,10 @@ import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.convert.ConversionDependencies
import org.libremediaconverter.convert.HardwareTranscoder
import org.libremediaconverter.convert.SoftwareTranscoder
import org.libremediaconverter.convert.installTestWorkManager
import org.libremediaconverter.model.Container
import org.libremediaconverter.model.ConversionRequest
import org.libremediaconverter.model.DeviceCodecs
import org.libremediaconverter.model.EnginePreference
@@ -129,6 +131,40 @@ class ProgressNotificationTest {
)
}
/**
* The same plumbing on the engine most conversions actually use, which had none.
*
* `ConversionWorker.kt:208-210` is a second `onProgress` lambda at a second call site — the one
* handed to `engine.transcode` — and it reported `ci == 0`. Every test above drives the FFmpeg
* path; `HardwareFallbackTest` reaches `runMedia3OrFallBack` but its recording transcoder
* records the call and never invokes the callback it was given. So the two engines' progress
* wiring was one tested and one not, and the untested one is the default: `ConversionRouter`
* sends everything it can to Media3.
*
* `AUTO` with a real H.264 probe, because `FORCE_SOFTWARE` is precisely what keeps the other
* tests out of this branch. The probe and the permissive codec profile are what let the router
* choose Media3 at all — `InputProbe()` reports `UNPARSEABLE`, which routes straight to FFmpeg.
*
* Asserted on the *percentage*, not merely on an update having happened: `publishProgress`
* takes a display name and a percent, and replacing the percent with a constant compiles.
*/
@Test
fun `progress from the hardware engine reaches WorkManager the same way FFmpeg's does`() {
ConversionDependencies.probe = { _, _ -> H264_SOURCE }
val reporting = ReportingHardwareTranscoder { onProgress -> onProgress(PERCENT) }
ConversionDependencies.hardware = { reporting }
runBlocking { workerReporting(EnginePreference.AUTO) { }.doWork() }
assertEquals("the job must have gone to the hardware engine", 1, reporting.attempts)
val progressUpdates = updater.infos.drop(1)
assertEquals("one throttled progress update expected", 1, progressUpdates.size)
assertEquals(
PERCENT,
progressUpdates.single().notification.extras.getInt(Notification.EXTRA_PROGRESS),
)
}
/**
* A worker routed to the software engine, whose engine is [report] and a written output.
*
@@ -137,7 +173,10 @@ class ProgressNotificationTest {
* bridge, which is native. [report] is handed the worker's own progress callback, and runs with
* the worker as its receiver so a test can stop it mid-transcode.
*/
private fun workerReporting(report: ConversionWorker.((Int) -> Unit) -> Unit): ConversionWorker {
private fun workerReporting(
enginePreference: EnginePreference = EnginePreference.FORCE_SOFTWARE,
report: ConversionWorker.((Int) -> Unit) -> Unit,
): ConversionWorker {
val worker = TestListenableWorkerBuilder<ConversionWorker>(
context = app,
inputData = workDataOf(
@@ -147,7 +186,7 @@ class ProgressNotificationTest {
ConversionWorker.KEY_CONTAINER to SPEC.container.name,
ConversionWorker.KEY_VIDEO_CODEC to SPEC.videoCodec.name,
ConversionWorker.KEY_AUDIO_CODEC to SPEC.audioCodec.name,
ConversionWorker.KEY_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name,
ConversionWorker.KEY_ENGINE_PREFERENCE to enginePreference.name,
),
runAttemptCount = 0,
).setId(JOB_ID)
@@ -171,6 +210,17 @@ class ProgressNotificationTest {
const val TICKS = 50
val SPEC = OutputFormat.MP4_H265.spec
val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000021")
/**
* A probe the router can actually route. `InputProbe()` reports `UNPARSEABLE`, which
* `PERMISSIVE.canDecode` refuses, so every job would reach FFmpeg with no test saying why.
*/
val H264_SOURCE = InputProbe(
videoCodec = "h264",
audioCodec = "aac",
container = Container.MP4,
durationMs = 1_000,
)
}
}
@@ -211,3 +261,22 @@ private class ReportingTranscoder(private val report: ((Int) -> Unit) -> Unit) :
const val OUTPUT_BYTES = 512
}
}
/** A hardware engine that reports whatever [report] wants reported, then writes an output. */
@UnstableApi
private class ReportingHardwareTranscoder(private val report: ((Int) -> Unit) -> Unit) : HardwareTranscoder {
var attempts = 0
override suspend fun transcode(input: Uri, output: File, request: ConversionRequest, onProgress: (Int) -> Unit) {
attempts++
report(onProgress)
output.writeBytes(ByteArray(OUTPUT_BYTES))
}
override fun close() = Unit
private companion object {
const val OUTPUT_BYTES = 16
}
}
+233 -17
View File
@@ -1,22 +1,27 @@
# Coverage-read findings
**Status:** five findings, none fixed, none urgent. F5 was added on 2026-08-27, found while decomposing #132 into children — it had been listed there as a test gap, and is not one. Every entry here is a *code* observation —
something a test would document rather than repair. The test gaps found in the same read are
tickets #132 and #133, not entries here; see [Not covered here](#not-covered-here).
**Scope:** what a JaCoCo read on 2026-08-26 turned up that writing a test would not fix. This is
a survey, not a work order. Acting on any entry is a separate decision and would be its own commit.
**Last verified:** `main` at `dc8b7c3`, 2026-08-26. Coverage re-measured that day with
`./gradlew :app:jacocoTestReport`: **84.9% line (1971/2321), 63.8% branch (900/1410)**, against
**456 JVM tests in 68 classes**. `CLAUDE.md` quotes 454 in 67 from four hours earlier; the
percentages are unchanged, so no figure there is stale.
**Status:** ten findings, none fixed, none urgent. F1-F4 came from the 2026-08-26 read; F5 was added
on 2026-08-27 while decomposing #132; **F6-F10 were added on 2026-09-02 from the wave-4 read**. Every
entry here is a *code* observation — something a test would document rather than repair. The test
gaps found in the same reads are tickets, not entries here; see [Not covered here](#not-covered-here).
**Scope:** what a JaCoCo read turned up that writing a test would not fix. This is a survey, not a
work order. Acting on any entry is a separate decision and would be its own commit.
**Last verified:** `main` at `54ca2dd`, 2026-09-02. Coverage measured that day with
`./gradlew :app:jacocoTestReport`: **92.8% line (2183/2352), 81.3% branch (1091/1342)**, against
**584 JVM tests in 87 classes**, matching what `CLAUDE.md` quotes.
The wave-4 read that produced F6-F10 also produced twelve test tickets, **#192-#203**, plus **#204**
for four candidates whose cost was not obviously worth paying. The split between them is the same one
this document has always drawn: a ticket is where a test goes, an entry here is where a test would not
help.
## Why this document is separate from `defect-audit.md`
`defect-audit.md` is the record of the 2026-08-22 defect sweep: sixteen entries, each a thing that
is *wrong at runtime*. Nothing here is wrong at runtime today. These are arms that cannot be
reached, accessors nobody calls, and one KDoc that contradicts the code beside it — the category
`defect-audit.md` calls **latent**, plus one that is not a defect at all and is recorded so the
next coverage read does not re-file it.
reached, accessors nobody calls, and two KDocs that contradict the code beside them — the category
`defect-audit.md` calls **latent**, plus several that are not defects at all and are recorded so the
next coverage read does not re-file them.
They are here rather than in that document because folding them in would inflate a sixteen-entry
audit whose status metadata has already gone stale once, and because they share a provenance:
@@ -24,7 +29,7 @@ every one fell out of reading a coverage report, and every one is the kind of th
report is *good* at surfacing and a test is bad at fixing. F5 is the clearest case — it was filed
as a test gap first, and only stopped being one when someone went looking for its callers.
Entry ids are `F1`–`F5` so they cannot be confused with `defect-audit.md`'s `D1`–`D16`.
Entry ids are `F1`–`F10` so they cannot be confused with `defect-audit.md`'s `D1`–`D16`.
## How to read the confidence labels
@@ -262,6 +267,179 @@ no way to make it happen now.
---
## F6 — Four more arms that cannot be reached, and one KDoc among them that is false
**Severity: low · Confirmed by inspection · F4's family, found in the wave-4 read**
```
app/src/main/java/org/libremediaconverter/model/ConversionRouter.kt:178-179
app/src/main/java/org/libremediaconverter/model/ConversionRouter.kt:221
app/src/main/java/org/libremediaconverter/model/ContainerCapabilities.kt:297
app/src/main/java/org/libremediaconverter/model/ContainerCapabilities.kt:324, :340
```
Four sites that a coverage report flags and that no test can reach. Each is recorded with the
upstream guard that makes it unreachable, because that guard is what would have to change first.
- **`ConversionRouter:178-179`** — the missed branch is `orEmpty()`'s absent-key arm on
`MEDIA3_MUXABLE_VIDEO[plan.container]`. `MEDIA3_CONTAINERS` is `setOf(MP4)` and `route()` returns at
`:104` for anything else, so `media3CanMux` only ever sees MP4, which both maps key. Same function
as F4's second pair, one line below it.
- **`ConversionRouter:221`** — `DeviceCodecs.PERMISSIVE.canDecode` returning **false** for
`InputProbe.UNPARSEABLE`. `PERMISSIVE` has no production caller at all (tests only), and the
router's one `canDecode` call at `:128` is already preceded by `:117` returning FFMPEG for
`UNPARSEABLE`. **Its KDoc at `:214-217` is false as written:**
> That exception matters: a device double that claims it can decode an unparseable file would let
> the router send a doomed job to Media3.
It would not — `:117` already caught it. This is F2's shape: a comment that describes a hazard the
code upstream has removed. Correcting it is a one-line change and should not be bundled with
anything.
- **`ContainerCapabilities:297`** — `if (container == GIF || container == IMAGE_SEQUENCE) return null`
in `repair`. `repair`'s only caller is `suggestions` (`:281`); `validate` returns at `:121` for
`isImageOutput` (which is exactly GIF ∥ IMAGE_SEQUENCE) before `suggestions` is reached, and
`firstContainerHolding` filters on `CARRIES_VIDEO`, which is empty for both.
- **`ContainerCapabilities:324` and `:340`** — the `else ->` arms themselves are exercised; what is
missed is the elvis tail, `firstOrNull() ?: VideoCodec.NONE` / `?: AudioCodec.NONE`. Reaching it
needs a container with no encodable codec on that axis. Audio-only containers return early at
`:307`, and the only containers with an empty audio set are GIF and IMAGE_SEQUENCE, excluded at
`:297` above.
**Recorded so the next read does not re-file them.** F4's rule applies unchanged: a second line of
defence that can be provoked is not a second line of defence, and widening a private function to make
one reachable buys a test that asserts a fallback fires when called in a way production cannot call
it.
---
## F7 — `probeWithExtractor`'s catch is unreachable for the same measured reason `probeForConcat`'s is
**Severity: n/a · No action · completes a measurement already on record**
```
app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt:180-182
```
```kotlin
} catch (e: Exception) {
Log.i(TAG, "Platform extractor could not read $uri.", e)
null
}
```
`CLAUDE.md` records the measurement for the *other* extractor site: Robolectric's `MediaExtractor`
never throws from `setDataSource`, checked across an unregistered `content://` authority, a missing
`file://`, a file of garbage bytes and an `http://` URL — all four returned with `trackCount = 0`.
`probeWithExtractor` calls the same overload, three lines apart in the same file, and the measurement
covers it identically. It was simply not written down for this site, so a future read would re-derive
it. It stays device-only, alongside `probeForConcat`'s.
**Two neighbouring line counts are artifacts of this, not separate gaps.** `MediaProbe:184` and
`:331` each report 27 missed instructions and are the `finally` block's synthetic exception-path copy
— JaCoCo duplicates a `finally` per exit path, and the exceptional one is unreachable for the reason
above. Do not read them as a third and fourth site.
---
## F8 — Three more dead members, and six unused defaults
**Severity: low · Confirmed by inspection · F3's family**
```
app/src/main/java/org/libremediaconverter/model/CopyPlanner.kt:28 ConversionPlan.hasVideo
app/src/main/java/org/libremediaconverter/codec/AndroidDeviceCodecs.kt:39 hardwareEncoders()
app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt:30 Result.output
app/src/main/java/org/libremediaconverter/convert/Transcoders.kt:28, :29, :40, :61
app/src/main/java/org/libremediaconverter/work/Reattachment.kt:28, :30
```
- **`ConversionPlan.hasVideo`** — zero callers in `main`, `test` or `androidTest`. Every `hasVideo`
hit in the tree is `InputProbe.hasVideo`, `OutputSpec.hasVideo` or `Container.extensionFor(hasVideo)`,
which are different properties on different types. A test asserting
`plan.hasVideo == (plan.video != VideoPlan.Drop)` is vacuous by construction.
- **`AndroidDeviceCodecs.hardwareEncoders()`** — its only caller is `RealMediaBenchmark`, in
`androidTest`. Production reads capabilities through `DeviceCodecs`, never the raw set.
- **`ConcatEngine.Result.output`** — `ConcatWorker` reads `result.strategy` and uses the `staged`
file it passed in, never `.output`.
- **`Transcoders.kt`'s default arguments** — `request` and `onProgress` on
`HardwareTranscoder.transcode` (`:28`, `:29`), `onProgress` on `SoftwareTranscoder.run` (`:40`),
and `format` on `ConcatJoiner.join` (`:61`). All three production call sites
(`ConversionWorker.kt:208`, `:234`, `ConcatWorker.kt:79`) pass every argument, so the synthesised
`$default` bridges and `$DefaultImpls` copies are never entered. The
`request: ConversionRequest = ConversionRequest(OutputFormat.MP4_H265.spec)` default is the one
worth a second look: nothing anywhere omits it, so an interface silently promises H.265 to a
caller that does not exist.
- **`JobSnapshot`'s `outputModifiedAt` and `tags` defaults** — `JobSnapshots.kt:32-42` passes all
seven fields, so the synthesised `$default` constructor (20 missed instructions at
`Reattachment.kt:14`) is never entered.
**Not a test gap, for F3's reason.** Delete them, or keep them and know they are unused; either is a
decision, and a test restating the compiler is not.
---
## F9 — Both workers' `getForegroundInfo` overrides are dead, and this is why
**Severity: n/a · No action · sharpens #88 rather than reopening it**
```
app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt:342-346
app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt:132-136
```
**#88 already closed on these**, after reading both and finding no decision worth a seam — the
correct call, and it stands. What #88 did not name is the reason they are cold in the first place,
which is stronger than "the JVM cannot reach them":
WorkManager calls `getForegroundInfoAsync()` **only for expedited work**. `ConversionWorker`'s own
KDoc says expedited is deliberately not used, and `grep -rn 'setExpedited\|OutOfQuotaPolicy' app/src`
returns nothing. So both overrides are dead in production today, not merely untested — a test would
assert the shape of something nothing invokes.
They are still correct to keep: `ForegroundInfo` is required by the `CoroutineWorker` contract and
`setForeground` is called explicitly elsewhere. **What would reopen this** is the same trigger #88
named — a `getForegroundInfo` that starts branching — plus one more: the day anything calls
`setExpedited`.
---
## F10 — Three arms that are reachable, uncovered, and cannot be made to bite
**Severity: n/a · No action · the shape a coverage number cannot distinguish**
```
app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt:550, :553
app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt:349, :352, :278
```
F4 and F6 hold arms that cannot be *reached*. These can — and a test written against them would still
pass under the mutation that ought to redden it, which is the harder case to spot and the more
expensive one to discover halfway through writing the test.
- **`observer?.cancel()`'s non-null arm** (`ConversionViewModel:550`, `JoinViewModel:349`). Reachable
by calling `convert()` twice. But `ScreenOwnership`'s token is what actually blocks the superseded
write — the ViewModel's own KDoc at `reset()` says the cancel is "a request honoured at the next
suspension point" and "the claim is what actually stops that write". Delete `observer?.cancel()`
and the suite stays green, correctly.
- **`if (info == null) return@collect`** (`ConversionViewModel:553`, `JoinViewModel:352`). Reachable
through `pruneWork()`. But when the null arrives the state is already terminal, so removing the
guard crashes the collector and **leaves the state unchanged** — a state assertion is green under
the mutation. The only observable is an escaped coroutine exception, which the ViewModel's own KDoc
documents as unreliable on the JVM: kotlinx-coroutines-test's process-wide collector hands it to
whichever `runTest` starts next.
- **`JoinViewModel:278`'s `Ambiguous` arm.** Looks like the twin of `ReattachGuardsTest`'s "a result
two jobs both claim", and is not. An `Ambiguous` requires a shared `outputPath`, so it can only be a
*finished* job — which maps to `Joined`, a state that reads nothing from `inputs`. **The Convert-side
twin does bite**, because `displayNameOf(tags)` reaches the file card; the asymmetry is the point.
**Recorded because each of these was picked up as a candidate and put down again.** The wave-4 read
lost time to all three before the mutation test was run in the head rather than the editor, which is
the cheaper order.
---
## Summary
| ID | Finding | Severity | Evidence | Action |
@@ -271,12 +449,23 @@ no way to make it happen now.
| F3 | `ConversionRequest.videoCodec` / `.audioCodec` have no callers | low | confirmed by inspection | delete, or keep for symmetry — **not** a test gap |
| F4 | Two private guards reachable only by direct call | n/a | confirmed by inspection | **no action** — named exemption, per #88 |
| F5 | `ConversionNotifications.areEnabled()` is never called | low | confirmed by inspection; grep returns the declaration only | **decide**: act on it or delete it — **not** a test gap |
| F6 | Four more unreachable arms; `ConversionRouter:214-217`'s KDoc is false | low | confirmed by inspection; each traced to its upstream guard | **no action**, except the one-line KDoc fix |
| F7 | `probeWithExtractor`'s catch is unreachable, as `probeForConcat`'s is | n/a | measured across four URI shapes (recorded in `CLAUDE.md`) | **no action** — device-only, now written down for both sites |
| F8 | Three more dead members and six unused defaults | low | confirmed by inspection; grep per member | delete or keep knowingly — **not** a test gap |
| F9 | Both `getForegroundInfo` overrides are dead: expedited work is never used | n/a | confirmed by inspection; `grep setExpedited` returns nothing | **no action** — sharpens #88's close |
| F10 | Three reachable arms where no mutation bites | n/a | confirmed by inspection; each mutation traced to its masking guard | **no action** — recorded to stop the next read re-picking them |
Order, if these are acted on: **F1 and F5 first, separately.** They are the two with a possible
user-visible answer — a format the app can produce and does not offer, and a warning the app
documents and does not give — and either answer changes what the tidying should look like. F2 and F3
are tidying and belong in one commit with each other, not with F1 or F5. F4 is finished by being
written down.
documents and does not give — and either answer changes what the tidying should look like. F2, F3 and
F8 are tidying and belong in one commit with each other, not with F1 or F5. F6's KDoc correction is a
third kind: one line, no decision, and it should not wait on the tidying. F4, F7, F9 and F10 are
finished by being written down.
**Six of the ten are now "no action" or "not a test gap", and that is the useful shape.** By wave 4
the report's remaining red is mostly this: arms nothing can reach, members nothing calls, and arms a
test can reach but not pin. A coverage number cannot tell any of them from a real gap, which is why
this document exists and why it grows faster than the percentage moves.
**F1 and F5 share a shape worth naming:** both are places where a comment describes behaviour the
code does not have, and in both the tempting fix (delete the dead arm, test the dead method) would
@@ -287,7 +476,15 @@ freeze the wrong answer in place. The decision comes first.
**The test gaps from the same read.** Seven JVM-side gaps (**#132**) and three seam questions
(**#133**) came out of this coverage read and are tracked there, because they are work rather than
observations. This document holds only what a test would not fix. #133 also records why
`AndroidDeviceCodecs.probe()` was considered and left out, so that spike is not run a third time.
`AndroidDeviceCodecs.probe()` was considered and left out **through `ShadowMediaCodecList`**, so that
spike is not run a third time.
**Updated 2026-09-02:** #194 proposes reaching the same code through a *pure seam* instead, which is a
different mechanism and one #133 did not evaluate — the builder objection it turns on (no
`setIsAlias`, no `setCanonicalName`) does not apply to a function taking its own entry type. #133's
close stands for the shadow; it is not a close on the seam. #194 also carries the reason the seam is
worth cutting at all, which is not coverage: the `runCatching` fallback logs "assuming permissive" and
returns empty sets, which makes `canEncode` and `canDecode` answer *no* for everything.
**`ConversionForegroundType.current()`**, which looked like the sharpest gap in the read and is not.
Its API 33 and 34 arms are cold on the JVM, but issue **#88** already established that the class is
@@ -321,6 +518,25 @@ the real ones — **34 of 383** and **20 of 143** missed — and the screens are
better-covered files in the repo, which is what #52, #57 and #61 were for. **Do not chase the
branch number here.** If a future read wants a screen metric, use lines.
**Updated 2026-09-02: the same codegen inflates the *instruction* count, which wave 3's filter did
not allow for.** Wave 3 selected candidates on `mi > 0` — at least one missed instruction — which was
right to prefer over a bare branch count and is still wrong on these files. `JoinScreen.kt:222` reads
`mi=10` and looks uncovered; it also reads `ci=38`, and `JoinStateAffordancesTest` already clicks that
Save button and asserts `save:joined.mp4`. Every `onClick` lambda body flagged this way turned out to
be covered at method level, the missed instructions being the recomposition-skip path again.
Use `ci == 0` — the line never executed, which is JaCoCo's own missed-line definition — and pair it
with a method-level `ci > 0 && mb > 0` pass for covered methods with cold arms. Neither filter alone
is enough: `ConversionViewModel.cancel()` misses no line at all, yet its non-null arm had never been
entered in 584 tests (#192). `CLAUDE.md`'s coverage entry carries the same correction.
**Also codegen, also not gaps**, recorded once so they are not re-derived: the synthetic
`NoWhenBranchMatchedException` closing an exhaustive `when` (`ConverterScreen:399`, `:686`,
`JoinScreen:278`, `MainActivity:160`); the inner `is Idle -> Unit` arms at `ConverterScreen:253-254`
and `JoinScreen:158-159`, which are structurally unreachable because the outer `when` already routed
`Idle`; and the closing brace of a `launch` block whose `collect` never terminates
(`ConversionViewModel:578`, `JoinViewModel:371`).
**Anything requiring a device.** `MediaProbe`'s FFprobe half (`MediaProbe.kt:151, 156-158, 173-188`)
and `FFmpegEngine` in full report 0% on the JVM and are covered by `androidTest`. JaCoCo measures
`testDebugUnitTest` only; their zeroes are a boundary, as #84, #85, #86 and #88 each recorded