runMedia3OrFallBack was eleven lines at 0% and isCancellation had never been called by any
JVM test -- ci=0, not merely a missed branch. The seam to reach it has existed the whole
time: ConversionDependencies.hardware, which no unit test had ever set. What kept the path
cold is that every worker test uses EnginePreference.FORCE_SOFTWARE, which never enters
the function, and the probe defaults to UNPARSEABLE, which PERMISSIVE.canDecode refuses --
so even AUTO would have routed straight to FFmpeg for a reason no assertion mentioned.
Both are now stated in setUp rather than inherited.
Four behaviours, each with the mutation that proves it:
hardware failure falls back to software delete the fallback call
... on a *clean* staging file delete staged.delete() before it
cancellation is rethrown, not fallen back delete `if (isCancellation(e)) throw e`
engine.close() runs either way empty the finally block
the display-name fallback (#169) change "input" to anything else
All five confirmed red, then restored.
The cancellation one is the reason this ticket was first in the group. runMedia3OrFallBack
catches Throwable, so without that re-throw a user cancelling a hardware transcode has the
app quietly start a *second* conversion in software -- the one thing cancelling is for.
ForcedFailureTest covers the failure half on a device and does not cover this half at all.
The clean-staging assertion is made where it is observable rather than by reading the
file: the software fake records whether the output existed when it was entered, so a
missing delete shows up as FFmpeg finding a half-written hardware output at the path it is
about to write.
556 -> 560 JVM tests, 0 failures. No production code changed.
ConversionWorker: 22 -> 9 missed lines, 14 -> 7 missed branches.
Line 2090/2348 -> 2103/2348; branch 1022/1340 -> 1029/1340.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The ticket asked for two things. One of them is not reachable from the JVM, and saying
so is most of the value here.
**probeForConcat's catch arm cannot be provoked on this runtime.** Robolectric's
MediaExtractor never throws from setDataSource -- measured across four input shapes: an
unregistered content:// authority, a missing file://, a file of garbage bytes, and an
http:// URL. All four returned normally with trackCount = 0. So a failed read arrives as
an empty track list rather than as an exception and reaches the same
ConcatInput(null, null, 0, 0, 0) by the other road. The catch stays covered only by
ConcatEngineTest on a device. The test file says this rather than implying the arm is
handled.
**What is reachable, and was genuinely missing, is the span.** Both halves were already
covered and neither reached the other: MediaProbeTrackWalkTest pins what concatInputFrom
makes of a track list, ConcatPlannerTest's `an unknown codec is not treated as a match`
pins what the planner does with a hand-built ConcatInput(video = null). The planner's
safety rests on the probe really producing that shape, and the hand-built fixture would
go on passing if it stopped.
Measured rather than claimed: mutating concatInputFrom's initial `video` to a non-null
placeholder leaves ConcatPlannerTest green and turns this red. Dropping the planner's
video null guard turns both red -- so that half was already held, and this file does not
claim credit for it.
The coupling itself is worth writing down: ConcatPlanner guards video against a null codec
and audio not at all, and that asymmetry is correct rather than an oversight --
MediaProbe.shortName returns a non-null String, so a null audioCodec means the track is
absent and two clips with no audio really do match, while a null videoCodec means absent
*or* unreadable. The audio check is safe because the video guard fires first on a clip
nothing could read. Nothing held that.
555 -> 556 JVM tests, 0 failures. No production code changed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Each of these is a site where JaCoCo reported mi > 0 -- a concrete instruction no test
runs -- rather than a partial branch on a compound condition, which is how the group was
filtered in the first place. Six closed, one moved to the exclusions.
AndroidDeviceCodecs:35 the UNPARSEABLE sentinel, refused where an unknown name is not
ContainerCapabilities:94 accepts(container, VideoCodec.NONE, mode) -- the audio twin has
had a test since #136; the asymmetry is the argument
ContainerCapabilities:323 repairVideo's keep-the-requested-codec arm
ContainerCapabilities:348 firstContainerHolding's fallback container
OutputPublisher:230 the resolver call that throws -- the third case the KDoc names
and the one the shape list was missing
JobSnapshots:31 a job that recorded no output path at all
Every one was mutated and confirmed red, then restored. Two are worth stating because
they did not go red first time or would not have:
**The firstContainerHolding test was vacuous on its first draft.** It asserted the
refusal still offered *something*, and deleting the fallback left it green: the source
container is a candidate in its own right, so the list stays non-empty and only its
contents change. Rewritten around AVI, which has no mapping for H.265, and asserting the
codec survives -- without the fallback the app silently offers H.264 instead, which is
the actual loss. This is the failure mode CLAUDE.md records from the mutation review, met
head on rather than in the abstract.
**OutputPublisher:230 needed one line.** `a destination whose size cannot be determined is
never deleted` already walked three RowShapes; QUERY_THROWS was the fourth case its own
KDoc names -- "a resolver call that throws" -- and the only one that reaches `?: false`
through runCatching rather than through a cursor answer.
ContainerCapabilities:282's `.filter { it != exclude }` is **not** closed here and is not
a gap: nothing can make it drop anything. On the shared container `repair` always changes
at least one codec, because a codec it left alone is one validate would not have refused;
every other candidate differs by container; and the single call site passing a non-default
exclude (validateVideo:186) excludes a spec carrying VideoCodec.COPY while every repaired
candidate carries NONE. F4-shaped -- recorded rather than covered, and 550 tests agree.
550 -> 555 JVM tests, 0 failures. Five new tests rather than six: OutputPublisher:230
is one line inside a test that already existed. No production code changed.
Line 2087/2348 -> 2090/2348; branch 1011/1340 -> 1022/1340.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ConversionForegroundType.current()` has three arms and the JVM suite executed one.
`robolectric.properties` pins everything to `sdk=36`, and `@Config` appears nowhere in
`app/src/test`, so 3 lines and 3 of 4 branches were cold.
The instrumented test is not a substitute, and the reason is specific rather than
general. `ConversionWorkerTest.foregroundTypeMatchesTheRunningApiLevel` asserts against
whichever API the leg is, so it covers one arm per leg and never the other two -- and the
legs that would cover 33 and 34 are the ones #122 wedges. From
docs/coverage-read-findings.md, an API 33 run reported `received: 60` with
`failed: unknown`: the regime was exercised and that leg could not have said so if it had
broken. This runs all three deterministically in the same ./gradlew invocation.
Four classes, not three. 35 shares its answer with 36 and looks redundant; it is the
whole point. Relaxing `>= VANILLA_ICE_CREAM` to `>` is invisible at every level except
exactly 35 -- measured, not assumed: that mutation failed ForegroundTypeApi35Test alone,
while swapping DATA_SYNC and MEDIA_PROCESSING failed 34, 35 and 36. Without the 35 class
the first mutation survives the suite.
546 -> 550 JVM tests, 0 failures. No production code changed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
COVERAGE. 87.1% line / 69.1% branch, 502 tests -> 88.9% line (2087/2348), 75.4%
branch (1011/1340), 546 tests in 76 classes, as #153's five children land.
The branch figure moved for two reasons and the entry now says so, because only
one of them is new tests. The numerator rose 974 -> 1011; the denominator *fell*
1410 -> 1340. Both are the seam work: pulling a `when` out of a lambda inside a
`collect` deletes the coroutine state machine's synthesized branches around it,
and leaves a plain function whose branches a test can choose.
`ConversionViewModel$observe$1$1` went from carrying the whole mapping to six
branches, while the extracted `ConversionViewModelKt` covers 41 of 42 and
`JoinViewModelKt` 38 of 39.
That is worth stating rather than quoting the percentage alone. A number that
rises because the denominator shrank is a different claim from one that rises
because more branches are tested, and this entry has a documented history of
explaining its own movements wrongly.
STACKED PRS. A new Conventions entry, from two traps measured on 2026-08-27
while landing #144-#151.
`gh pr merge` refuses a stacked PR outright -- "must be merged using the
asynchronous merge REST API" -- and so does the plain `/merge` endpoint. The one
that works is `PUT .../pulls/N/merge-async`, which returns a uuid to poll.
The second is worse because nothing looks wrong. GitHub retargets a stacked PR's
base to main when the one below it merges, but asynchronously. Merging five about
thirty seconds apart outran it, so each merged into its own already-merged base
branch. Every call returned `status: merged`, every PR read MERGED, `gh pr list
--state open` was empty, and none of the content was on main.
What caught it was a coverage re-measure two points below what the same tree had
produced an hour earlier -- a fresh `git pull` changed nothing, which is what
made it a question rather than a stale checkout. `git merge-base --is-ancestor`
answers it in one line. #160 is what the recovery cost.
Also recorded: the auto-retarget belongs to the stacking feature. A PR opened
with a plain `--base some-branch` does not retarget when that branch merges, and
has to be moved by hand -- which is what #163 needed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The stateful outer composables hand `ConverterScreenContent` and
`JoinScreenContent` a list of `viewModel::` references. No test in the suite had
ever seen that list: the content tests build their own `ConverterActions`, so
they drive the stateless inner and never touch the wiring.
THE TICKET'S PREMISE WAS HALF WRONG, AND CHECKING BEAT ASSUMING. #156 was filed
claiming a transposition of any two of seventeen bindings would survive the
suite. Measured instead of trusted:
onVideoCodec <-> onAudioCodec -> REJECTED: "Inapplicable candidate(s):
fun setAudioCodec(codec: AudioCodec)"
onCancel <-> onReset -> COMPILES
Every typed binding -- container, both codecs, preset, suggestion, quality,
engine preference -- takes a distinct parameter type, so the compiler is already
the test. Writing assertions against those transpositions would have been
theatre, and this file says so rather than quietly including them.
WHAT IS ACTUALLY AT RISK is the `() -> Unit` bindings, which are interchangeable
to the compiler: two on the converter screen (onCancel, onReset) and *three* on
the join screen (onJoin, onCancel, onReset). A Cancel button that discards the
finished file, a Start-over that leaves it on screen, or a Join button that
cancels -- each is one wrong word and each ships.
I got that wrong in the first check too: an early run reported the onCancel/
onReset swap as rejected, from a grep-and-exit-code test that misread a stale
build. Re-running it properly printed BUILD SUCCESSFUL with the swap in place.
THE SEAM. `converterActions(viewModel, onPickInput, onConvert, onSave)` and
`joinActions(viewModel, onPickInputs, onSave)`. The launcher-backed actions stay
parameters -- they need an ActivityResultLauncher, which is the part that
genuinely needs a composition, and keeping them out means the rest needs none.
Told apart by effect rather than by a recording double: `reset()` sets the state
to Idle, `cancel()` with no active job leaves it alone (`activeWorkId?.let`,
which SettingsEditsTest pins).
Mutations -- every transposition caught, each by two tests:
converter onCancel <-> onReset | 2 tests
join onJoin <-> onCancel | 2 tests
join onReset <-> onCancel | 2 tests
a typed binding dropped to {} | 1 test
a launcher action rerouted | 1 test
The two-test symmetry is deliberate: one direction alone passes against a wiring
with BOTH actions bound to the same method, which is what a copy-pasted line
produces.
Three guard assertions earned their place during writing -- the picks land
through an injected dispatcher, and without `ParkedPickDispatcher.runAll()` all
three state-based tests sat on Idle and would have asserted nothing. They failed
loudly instead of passing quietly.
`@UnstableApi` on both builders, per CLAUDE.md; lint caught their absence, as it
did in W1.
525 -> 537 tests, 88.0% -> 88.9% line, 70.5% -> 75.4% branch. Gate green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`setPreset` was covered; the six beside it and `cancel()` had no coverage at all.
That asymmetry is the tell -- they are reachable from the JVM suite by exactly
the route `setPreset` already takes, and nothing had asked.
WHAT IS ASSERTED. Not "the setter sets something". Each of these copies into a
nested `OutputSpec`, so the failure worth catching is a setter that writes the
right value into the wrong field, or that rebuilds the spec and quietly discards
the other two. Every test asserts the field it changed AND that the rest survived.
THREE OF THEM DID NOT BITE, AND THE REASON IS WORTH KEEPING. The first run of the
mutations came back with two green:
setContainer rebuilding from OutputFormat.MP4_H265.spec -> GREEN
setQuality also resetting enginePreference to AUTO -> GREEN
Both for one mistake of mine: I asserted "the rest survived" against values that
were still at their defaults. `ConversionSettings` starts at `MP4_H265.spec`,
`QualityTier.FAST` and `EnginePreference.AUTO` -- so a mutation that RESET a
neighbouring field to its default was indistinguishable from one that left it
alone. The tests were checking a value, not a behaviour.
Fixed by moving each neighbour off its default before the call under test. A
third test had the same latent hazard -- it asserted `quality == FAST` -- and was
corrected with the others rather than left to fail later.
That is precisely the shape CLAUDE.md warns about ("five of them passing the
whole suite over a completely unguarded code path"), and it is the second time in
this wave the mutation pass has earned its place: green was not evidence.
Eight mutations, all red after the fix:
setContainer rebuilds from a preset | 1 test
setQuality resets the engine preference | 1 test
setEnginePreference resets the quality | 2 tests
applySuggestion resets quality | 1 test
setVideoCodec writes nothing | 1 test
setAudioCodec writes nothing | 2 tests
setEnginePreference writes nothing | 2 tests
cancel() dereferences a null activeWorkId | 1 test
516 -> 525 tests, 87.7% -> 88.0% line, 70.4% -> 70.5% branch. Gate green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The join-side twin of W1, deliberately the same shape -- one refactor done twice,
and letting the two diverge would cost more than the duplication. Five arms had
never been chosen by any test, for the same reason: a real ConcatWorker only ever
reaches a terminal state with well-formed output.
WHAT THE SEAM TURNED UP. This line was in the SUCCEEDED arm:
info.outputData.getString(ConcatWorker.KEY_STRATEGY)
?.let(ConcatStrategy::valueOf) ?: ConcatStrategy.REENCODE
`valueOf` throws IllegalArgumentException on a name this build does not define,
and this runs inside a `viewModelScope` collect with no handler -- so it is not a
Failed state, it takes the process down.
Not theoretical. WorkManager keeps finished work about a week, so a downgrade or
rollback hands this build a job enqueued by another one -- the premise
`WorkerEnumFallbackTest` and `JobTags` are both written on. `ConcatWorker` writes
`result.strategy.name`, so a build that added a third strategy would leave this
one crashing on its own completed joins.
The codebase had already made this exact fix one file over, and said why:
// Looked up rather than `valueOf` -- see the same three reads in
// ConversionWorker. This one is above the try as well, so a format name this
// build does not define used to throw past the catch
The matching read on the ViewModel side had not been changed with it. It is now
`ConcatStrategy.entries.firstOrNull { it.name == name } ?: REENCODE`.
PROVEN RATHER THAN ASSERTED. Restoring `valueOf` and running the new test:
RED: an unknown strategy name is read as a re-encode rather than thrown
java.lang.IllegalArgumentException: No enum constant
org.libremediaconverter.model.ConcatStrategy.SMART_CONCAT_V2
REENCODE is the conservative default rather than an arbitrary one: it is the
answer for inputs that do not match, so a job whose strategy cannot be read is
described as the more cautious of the two rather than claimed as a lossless
stream copy. The mutation to STREAM_COPY reddens two tests.
Eight mutations, all red:
unknown strategy -> STREAM_COPY | 2 tests
runAttemptCount ignored, both directions | 2 tests
success with no path -> empty Joined | 1 test
BLOCKED unfolded from RUNNING | 1 test
blank error no longer falls back | 1 test
cancellation ignores the caller's state | 1 test
516 -> 530 tests, 87.7% -> 88.0% line, 70.4% -> 71.3% branch. Gate green:
assembleDebug, testDebugUnitTest, compileDebugAndroidTestKotlin, ktlintCheck,
detekt, lintDebug.
Stacked on W1 (#162), which this mirrors and should not land before.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ConversionViewModel.observe` maps a `WorkInfo` onto a `ConversionState`. That is
the app's main UI state machine, and no test had ever chosen which arm it took.
NOT COLD CODE, WHICH IS THE POINT. `ConversionViewModel$observe$1$1` already
reported 28 covered lines and 24 covered branches: every test that drives a real
worker runs this. But a real worker only ever reaches a terminal state with
well-formed output, so `SUCCEEDED`-with-a-path and `FAILED`-with-a-message were
the only arms any test had produced. The other six ran never -- the progress
read, both sides of the retry check, a success naming no file, a failure with
nothing to say, `CANCELLED`, and `BLOCKED`.
A grep makes that look untrue: all six `WorkInfo.State` constants appear in the
JVM suite. They are in `ReattachmentTest`, driven into `Reattachment.choose` --
a *different* function encoding the same enqueued-means-retry rule. So the rule
had a test in one of its two homes, and the copy the user's screen reads had
none.
THE SEAM. `workManager` comes from `WorkManager.getInstance` in the constructor
and `observe` is private, so nothing could hand this a chosen `WorkInfo`. The
`when` is now `conversionStateFrom`, a pure function over a `ConversionUpdate`
carrying only the fields it reads -- the same shape as `JobSnapshot` beside
`Reattachment.choose`, and its KDoc gives the same reason. `outputData` stays a
`Data`, which this suite already builds with `workDataOf` everywhere; unpacking
it into five nullable strings would move the same reads without helping.
TWO THINGS DELIBERATELY LEFT OUTSIDE IT:
- The ownership check stays at the call site. Its comment says it guards the
file ownership the SUCCEEDED arm takes, not merely the assignment, so moving
it inside would change what it protects.
- The mapping takes no responsibility for the staged file. It returns the
state; the caller reads the file off the result. That is strictly better
than the original, where `pendingStaged = staged` happened inside one arm:
"the state and `pendingStaged` refer to the same file or to no file" is now
the shape of the code rather than a rule two branches have to keep.
Mutations, each killing exactly the test it should:
| mutation | red test |
|---------------------------------------------|-----------------------------|
| progress read ignored | reports the progress |
| runAttemptCount ignored -> always Waiting | never run is simply starting|
| runAttemptCount ignored -> never Waiting | already run is waiting |
| success with no path -> empty Converted | named no file is a failure |
| blank name/type no longer falls back | blank falls back like missing|
| blank error no longer falls back | blank message falls back |
| cancellation ignores the caller's state | lands where caller said |
| BLOCKED remapped | blocked looks like starting |
`ENQUEUED` needs both mutations and both tests: either one alone passes against a
mapping that ignores `runAttemptCount` entirely.
The extracted functions carry `@UnstableApi` rather than swallowing the marker
with `@OptIn`, per CLAUDE.md -- lint's UnsafeOptInUsageError caught their absence.
An early `@Suppress("ReturnCount")` turned out to be unnecessary and was removed
rather than left: detekt is clean without it, and the file now carries none.
502 -> 516 tests, 87.1% -> 87.7% line, 69.1% -> 70.4% branch. Gate green:
assembleDebug, testDebugUnitTest, compileDebugAndroidTestKotlin, ktlintCheck,
detekt, lintDebug.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four messages were written out in two places each, in a codebase that already
had the convention for this and states it in `OutputPublisher.kt`:
Kept next to [STAGED_FILE_GONE_MESSAGE] for the same reason it is: both
ViewModels need it and staging is what it is about.
The ticket named three. A wider scan -- `"[A-Z][^"]{8,90}[.!]"` rather than the
{15,70} that produced the original list -- found a fourth, `"Joining failed."`,
which is the exact join-side twin of `"Conversion failed."` and had been missed
because it is fifteen characters long.
"Pick at least two files to join." -> ConcatWorker.TOO_FEW_INPUTS_MESSAGE
"Joining failed." -> ConcatWorker.GENERIC_FAILURE_MESSAGE
"Conversion failed." -> ConversionWorker.GENERIC_FAILURE_MESSAGE
"Could not save the file." -> SAVE_FAILED_MESSAGE, beside
STAGED_FILE_GONE_MESSAGE
Each constant sits with the layer that owns the condition, which is what the two
existing constants do. The arity rule is the worker's -- `request(...)` takes a
`List<Uri>` and checks nothing about its length -- so `TOO_FEW_INPUTS_MESSAGE`
lives there and the ViewModel reads it, not the other way round.
WHY THE TWO `Log.e` LITERALS STAY. `"Conversion failed."` and `"Joining failed."`
each also appear in a log line beside the failure they describe. Those keep their
own copies: a log has a different audience and carries the exception with it, and
coupling it to the user-facing wording would mean rewording the screen to change
a log. Stated in the KDoc so the next scan does not read them as a miss.
THE TEST IS A CROSS-LAYER ONE, DELIBERATELY. #158's done-when is explicit that "a
test asserting the constant equals its own value is worth nothing". Sharing a
constant makes the two sites agree by construction; what it cannot show is that
both layers still *reach* it. So `SharedFailureMessagesTest` drives each for real
-- the ViewModel through `onInputsPicked`, the worker through `doWork` -- and
asserts the two answers are the same string, taken from two running layers rather
than from one declaration.
That the sharing was worth doing at all is visible in what was pinned before:
`RefusedJobTest` (#139) pinned the worker's copy of the arity message and nothing
pinned the ViewModel's, so the screen's wording could drift with no test saying
anything.
Mutations:
| mutation | result |
|---|---|
| ViewModel keeps its own drifted literal | red |
| ViewModel's arity guard removed entirely | red |
Gate green: assembleDebug, testDebugUnitTest, compileDebugAndroidTestKotlin,
ktlintCheck, detekt, lintDebug.
Not done here: `"Saved ${s.displayName}."` appears in both screens. It is left
alone, and the reason is a real distinction rather than an oversight -- the four
above are cases where one layer's message is another layer's *fallback*, so drift
means the user sees different words for one condition. Two screens each wording
their own success text is ordinary UI, and drift there is cosmetic.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
84.9% line / 63.8% branch, 454 tests -> 87.1% line (2025/2324), 69.1% branch
(974/1410), 502 tests in 71 classes, as #132 and #133's ten children land.
The entry already instructs re-measuring before quoting, and that is why this is
here rather than in the batch: quoting these numbers before the work merged would
have described a tree that did not exist. It nearly went wrong the other way too
-- the first measurement for this commit was taken against a main that was three
merges stale and read 85.1%.
Also says something the bare numbers do not. Branch moved 5.3 points against
line's 2.2, and that asymmetry is the expected shape of this kind of work rather
than a curiosity: those children targeted decision code -- enum fallbacks,
refusal arms, cursor shapes, a `when` over container rules -- where one test
chooses a branch the suite had never taken. Line coverage barely notices that.
Branch coverage is the whole point.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#142 -- openOutputStream refuses two ways and only one was reachable. A
provider that has gone away throws from inside the call, which
`a destination the provider will not open...` already drives. A provider
that is present and declines returns null, and nothing could produce that
on demand. openDestination is the seam; the test asserts the failure names
the destination, which is what separates the `?: error(...)` from an NPE
inside `use`.
#143 -- the sweep's re-read. **The seam the ticket proposed does not reach
it.** Overriding the listing fires before the entries are snapshotted, so
StagingSweep.collectable is handed the new timestamp, the file is never
proposed for deletion, and the guard is never exercised. Measured: with an
entriesIn seam, deleting the guard outright left the test green.
The race is a file that *was* collectable when the snapshot was taken and
is not by the time the delete comes round, so the seam has to sit at the
snapshot. `snapshot(listing)` does, and deleting the guard now reddens the
test.
Three mutations after the move, three red:
null stream returns silently null-return test
null stream via !! instead null-return test
sweep deletes unconditionally race test
OutputPublisher.kt now has no never-executed lines at all. Two partial
branches are left and both are named exemptions rather than gaps:
L216's `getOrNull() ?: false` and L304's `getOrDefault(absoluteFile)` are
the failure arms of a runCatching whose body cannot be made to throw
through any public entry point -- the same shape as the `size >= 0`
exemption recorded in the previous commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#142 -- openOutputStream refuses two ways and only one was reachable. A
provider that has gone away throws from inside the call, which
`a destination the provider will not open...` already drives. A provider
that is present and declines returns null, and nothing could produce that
on demand. openDestination is the seam; the test asserts the failure names
the destination, which is what separates the `?: error(...)` from an NPE
inside `use`.
#143 -- the sweep's re-read. **The seam the ticket proposed does not reach
it.** Overriding the listing fires before the entries are snapshotted, so
StagingSweep.collectable is handed the new timestamp, the file is never
proposed for deletion, and the guard is never exercised. Measured: with an
entriesIn seam, deleting the guard outright left the test green.
The race is a file that *was* collectable when the snapshot was taken and
is not by the time the delete comes round, so the seam has to sit at the
snapshot. `snapshot(listing)` does, and deleting the guard now reddens the
test.
Three mutations after the move, three red:
null stream returns silently null-return test
null stream via !! instead null-return test
sweep deletes unconditionally race test
OutputPublisher.kt now has no never-executed lines at all. Two partial
branches are left and both are named exemptions rather than gaps:
L216's `getOrNull() ?: false` and L304's `getOrDefault(absoluteFile)` are
the failure arms of a runCatching whose body cannot be made to throw
through any public entry point -- the same shape as the `size >= 0`
exemption recorded in the previous commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`the sweep tolerates a staging path that is not a directory` failed once on run
33069641674, against 468 tests that pass on this machine including under
`--rerun-tasks`:
java.io.FileNotFoundException at OutputPublisherStagingTest.kt:112
468 tests completed, 1 failed
Line 112 was `writeBytes` immediately after `deleteRecursively()`.
`FileOutputStream` answers `FileNotFoundException` for an existing directory, so
something had recreated the path inside that window. That something is
`LibreMediaConverterApp.onCreate`, which ends with
appScope.launch { OutputPublisher(...).sweepStaging() }
on `Dispatchers.IO`, and `sweepStaging` reads `stagingDir`, whose getter calls
`mkdirs()`. Robolectric builds the application for every test that asks for one,
so that background `mkdirs()` is in flight across the whole suite on a thread the
paused main looper does not control and no test awaits.
Retrying closes the window rather than narrowing it, because the race is not
symmetric: `mkdirs()` fails on an existing regular file, so the invariant only has
to survive being *established*. Once a write lands, nothing in the suite can turn
this path back into a directory -- which is also why the new assertion that the
sweep left a file behind is worth making.
The `check()` matters as much as the loop. The next failure here should say
"something recreated conversions/ as a directory", not `FileNotFoundException at
line 112` -- that is the difference between a flake someone reads and a flake
someone re-runs.
The wider problem is #159 and is deliberately not fixed here: `AppStartSweepTest`,
`JobSnapshotsTest` and `SpaceArithmeticTest` all name the same path, and the real
answer is an injectable scope rather than a retry loop in every staging test.
#159's done-when is that this loop can be deleted.
Mutation: `listFiles() ?: return` -> `listFiles()!!` reddens exactly this test.
Gate green: assembleDebug, testDebugUnitTest, compileDebugAndroidTestKotlin,
ktlintCheck, detekt, lintDebug.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
destinationIsKnownEmpty's three short-circuits -- no SIZE column, no row,
a null cell -- each had to answer false and none was tested. Its KDoc is
unambiguous about why: "this decides whether a delete is allowed and 'I
could not tell' must never authorise one." The existing tests only ever
drove a provider that answers properly, where the answer is zero and the
delete is correct. Getting the uncertain cases backwards costs the user a
file they already had, on a save that failed.
Also discardStaged's null parentFile, and sweepStaging's null listing --
which is not the case the existing `tolerates a staging directory that
does not exist yet` covers, because stagingDir's own mkdirs() recreates a
missing directory and it then lists as empty. Only a path that cannot be
a directory makes listFiles() answer null.
Five mutations, three bite:
drop !row.isNull(size) short-circuit test red
drop row.moveToFirst() five tests red
parentFile!! instead of ?: return false parentless test red
size >= 0 -> size >= -1 GREEN, does not bite
parentless treated as staged GREEN -- bad mutation, see below
The first green one is recorded in the test as a named exemption.
Measured: getColumnIndex returns -1 for an absent column and isNull(-1)
throws CursorIndexOutOfBoundsException, which the surrounding runCatching
already turns into `?: false`. Same answer, reached by the exception path,
so no behavioural test can pin that conjunct. It stays anyway -- control
flow through an exception is worse than a comparison, and another Cursor
implementation need not throw.
The second was my mistake rather than a finding: substituting stagingDir
for the null parent reaches `return false` by a different route, so it
proves nothing. parentFile!! is the honest mutation and it goes red.
:197, :235 and :258 are now covered. What is left in this file is exactly
what the ticket scoped out: :173-174 (#142) and :267 (#143).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing had ever handed InputQuery a cursor row. UnknownInputSizeTest
drives the no-provider case thoroughly -- query returns null, measure()
answers -- so firstRow's body, displayNameOrNull and sizeOrNull had never
executed at all.
Nine tests over FakeSafProvider's RowShape states. What they pin is not
"reads a cursor" but the rule the class exists for: a size nobody could
determine must arrive as null, never 0. Four separate ways a provider
fails to give one -- a null cell, a missing column, a negative value, an
empty cursor -- plus a provider that throws outright, which is the guard
firstRow's KDoc is written for.
Mutations run, all four bite:
drop `takeIf { it >= 0 }` from sizeOrNull -> negative-size test red
drop `!isNull(it)` from sizeOrNull -> null-size test red
drop the runCatching in firstRow -> throwing-provider test red
drop `!isNull(it)` from displayNameOrNull -> GREEN, does not bite
That last one is recorded in the test's KDoc as a named exemption rather
than papered over. Measured: MatrixCursor.getString on a null cell returns
null while getLong returns 0. So the guard is load-bearing on the size path
-- it is what stops a null becoming a real number -- and unfalsifiable on
the name path, where getString already yields null. It stays regardless:
Cursor.getString's contract makes throwing on null implementation-defined,
and a real provider may do what MatrixCursor does not.
InputQuery.kt now has no never-executed lines. Suite 456 -> 465 tests,
branch coverage 63.8% -> 65.6%.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two of #132's items are cursor-shaped -- InputQuery's row reads (#137) and
OutputPublisher.destinationIsKnownEmpty's short-circuits (#140) -- and the
provider that could drive them lived inside OutputPublisherPublishTest and
could only answer correctly. Its row was always (file.name, file.length()).
Moved FakeSafProvider, FakePlainProvider and the registration helper to
FakeProviders.kt, same package, following StagingCleanupSupport.kt and
ParkedPickDispatcher.kt. UnreliableOutputStream stays behind: it serves one
test, which is the line WorkerStubs.kt draws.
Added RowShape, seven ways a provider can answer a metadata query. Column
granularity is deliberate -- OutputPublisher reads only SIZE, InputQuery
reads both and reaches different answers depending on which is bad -- and so
is keeping null, missing, negative and no-row distinct rather than folding
them into one "bad" case. That distinction is the whole reason InputQuery
exists: hasSpaceFor(0) is only "is there 128 MB free", so a size nobody
could determine must not arrive as 0.
No production change. OutputPublisherPublishTest, OutputPublisherStagingTest
and UnknownInputSizeTest pass unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A gap audit over the eight branches merged together looked for lines still
never executed and asked, for each, whether something already accounts for it.
Everything mapped except one: `ConcatWorker.kt:42`, the refusal of a join with
fewer than two inputs.
Its neighbour maps. `ConcatWorker.kt:40` -- the missing-URI-array arm, two lines
above -- is covered on the device by
`UnopenableUriTest.aJoinWithNoInputArrayFailsWithAMessage`. That is invisible to
JaCoCo, which measures `testDebugUnitTest` only, so the report shows both arms
cold and cannot distinguish the one that is e2e-covered from the one nothing
touches. Only reading the androidTest source separates them.
`grep` says nothing in either source set mentions "Pick at least two files to
join." Two tests here now do:
- `a join of a single file is refused with a message rather than joined` pins
the verdict and the message together, via `Failure.equals`, for the reason the
file's header already gives.
- `a join of two files is not refused for its count` is the control that puts
the assertion on the boundary rather than on the string. It refuses the
*space* rather than letting the job run: the next thing past the count guard
is `ConcatEngine`, which is native, and `NamingPublisher`'s KDoc already
records that no JVM test gets past it. A failure carrying the space message is
proof execution reached line 57, which is proof it cleared line 42, at no
engine cost.
Reachability is the header's argument plus one of its own: `request(...)` takes
a `List<Uri>` and checks nothing about its length, so a one-item join is a
well-formed call rather than a corrupted queue entry.
Mutations, each killing exactly the test it should:
| mutation | red |
|---|---|
| guard deleted outright | `a join of a single file is refused...` |
| `uris.size < 2` -> `< 3` | `a join of two files is not refused for its count` |
Restored, both green. Gate green: assembleDebug, testDebugUnitTest,
compileDebugAndroidTestKotlin, ktlintCheck, detekt, lintDebug.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#84 closed by classifying probeWithExtractor and probeForConcat as
device-bound and explicitly not a gap. That was right about FFprobe and
right about the measurement boundary, and wrong that these are only
orchestration. The track walk is a branch matrix, and androidTest reaches
it only through whatever the committed fixtures happen to contain -- so
none of its rules is *chosen* by any test there.
#133 offered two ways to reach it: drive ShadowMediaExtractor, or cut the
loop into a pure function. Taking the second, which is the pattern
CLAUDE.md names and work/FailureOutcome.kt documents. extractedFrom and
concatInputFrom take List<MediaFormat>; what is left needing a device --
setDataSource, getTrackFormat, release -- is one three-line extension
function, which is the thin edge androidTest should be covering.
The two are deliberately not merged despite the overlap. One reads
duration and not frame rate; the other reads frame rate and not duration.
A merged version would compute both for every caller, and ConcatPlanner
treats an unknown frame rate as "cannot prove a match" -- so a field the
join flow does not need must not start arriving as a number.
Eleven tests over cases no fixture provides: two video tracks, two audio
tracks, audio outlasting video, a track with no KEY_DURATION, audio
declared before video, a subtitle track, and no tracks at all.
Six mutations, six red:
last video track wins first-video test
last audio track wins first-audio test
duration = last rather than max longest-track test
drop the containsKey guard six tests (getLong throws on a missing key)
guess a frame rate of 30 no-frame-rate test
join takes the last video track join frame-rate test
MediaProbe's missed branches drop 91 -> 70; what is left is the FFprobe
half and the two catch arms, which are native and device-bound exactly as
#84 said.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both exits were cold, and both are reachable for the same reason: a job
does not have to come from the picker. WorkManager keeps work for about a
week, so a downgrade or rollback hands this build a job enqueued by
another one, and request(...) is callable directly.
:62 -- a missing KEY_INPUT_URI -- was untested everywhere, JVM and device.
The nearest e2e test, ForcedFailureTest.aMissingInputFailsRatherThanCrashing,
passes a URI pointing at a file that does not exist, which reaches the
engine and fails much later with a different message.
:124-126 -- the Validation.Invalid refusal -- had no test at all, though
its comment names both arrival paths it exists for.
Five tests, in a new file because both are about the *message*. A refusal
that fails with empty output Data renders the UI's generic "Conversion
failed." with nothing else to say, which is the defect shape
DeniedForegroundStartTest records from the device pass; asserting the
verdict alone would pass against exactly that.
Two of the five are there to stop the others passing for the wrong reason:
`a refused spec never reaches an engine` says it failed *before*
converting rather than during, and `a valid spec is not refused` is the
control -- without it every assertion here would still pass against a
worker that refused everything.
Three mutations, three red:
change the no-input message no-input message test
drop the validation refusal both refusal tests
validate but keep converting both refusal tests
L61-62 and L123-126 are now fully covered, branches included.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ConversionWorker has WorkerCancellationTest and DeniedForegroundStartTest.
Its twin had the retry case only -- `a join whose foreground start is
denied` already existed -- so two of ConcatWorker's three failure exits
were cold: the CancellationException arm, and FOREGROUND_DENIED.
Four tests, added to the files that own each rule rather than to a new
ConcatWorker file, which is how this suite is organised: a file per rule,
tested across both workers.
The cancellation seam is worth a look in review. The conversion twin
cancels inside the engine, which is honest there because
ConversionDependencies has a seam for it. ConcatWorker calls ConcatEngine
directly and has none -- it is native and nothing here gets past it -- so
the cancellation is injected at the only other point inside the try,
setForeground. That is a real shape rather than a contrivance: a job
cancelled while WorkManager is promoting it is exactly when that window is
open, and the catch arm cannot tell where in the try it came from.
FailedFuture moved to WorkerStubs.kt on the way. Two tests now inject two
different failures through it, and Kotlin will not take two file-private
top-level classes of one name in one package.
Four mutations, four red, each isolated:
cancellation arm -> Result.failure propagation test only
drop delete on cancellation cancellation-partial test only
FOREGROUND_DENIED -> Result.retry past-the-bound test only
drop delete on the Throwable path give-up-partial test only
ConcatWorker's :92, :95-96 and :105-106 are covered; missed branches 4 -> 3.
What is left is what the ticket scoped out: the two input guards (e2e), the
ConcatEngine success path (native), and getForegroundInfo (#88's named
exemption).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two halves of ContainerCapabilities.validate were written together
and only one of them was ever checked. Six audio outcomes had no test --
every one a string the user reads -- while the video twin of each was
already covered.
Seven tests, deliberately shaped like their twins rather than as a fresh
idea about what to assert:
unidentifiable source audio on a COPY twin of `an unidentifiable
source codec cannot be copied`
container cannot hold the copied source twin of `a codec the container
cannot hold is refused...`
container cannot carry it on encode twin of `H265 in AVI is refused`
this app cannot encode it twin of `copying is offered as
the fix when...`
accepts(_, AudioCodec.NONE, _) -> true twin of the VideoCodec.NONE arm
accepts(_, AudioCodec.COPY, _) throws twin of `resolving COPY before
asking the matrix is required`
The seventh is not the audio axis: validateVideo's copy-into-a-container-
that-cannot-hold-it refusal was the one video outcome with no test, and it
is the same shape and the same file.
Each asserts the message verbatim and re-validates every suggestion the
refusal offers. Validation.Invalid promises its suggestions are themselves
valid and names this class as the proof; the existing property test walks
the presets, and no preset reaches suggestions() through validateAudio.
Seven mutations run, seven red, each isolated to exactly one test:
CARRIES_AUDIO check -> false encode-path test only
drop the COPY error arm resolve-first test only
AudioCodec.NONE -> false no-audio-track test only
drop ENCODABLE_AUDIO check unencodable test only
drop audio copy container check audio-copy test only
drop video copy container check video-copy test only
drop unidentified-audio guard unidentifiable test only
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WorkerEnumFallbackTest already existed for this defect class -- a name
this build does not define, read above the try, throwing out of doWork
entirely: FAILED with reschedule=false, empty output Data so the screen
said "Conversion failed." with nothing else, and the staged file never
deleted. It covered 2 of the 5 above-the-try reads. readSpec's three
were the ones left, and all three were cold.
The baseline is the part worth reviewing. readSpec returns the *entire*
fallback spec the moment any one axis fails to resolve, so a test
starting from MP4_H265 -- which is itself the fallback -- cannot tell a
worker that read the spec correctly from one that gave up on it. These
start from MKV/H.264, which differs on container and video codec at
once, and assert the spec that actually reached the transcoder rather
than only that a Result came back.
Mutations run, four for three tests:
KEY_CONTAINER `?: return fallback` -> `?: error(...)` -> container test red
KEY_VIDEO_CODEC same -> video test red
KEY_AUDIO_CODEC same -> audio test red
fallback = MP4_H264 instead of MP4_H265 -> all three red
The first three confirm the tests are isolated to their own axis; the
fourth confirms they pin *which* spec ran, which is what "a Result at
all" would have missed.
readSpec is now fully covered, branches included.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found while decomposing #132 into children. It was item 6 there, and it
looked like the cheapest item on the list: three cold lines, a KDoc with
real user-visible stakes, and a permission Robolectric can flip in one
line.
grep -rn 'areEnabled' app/src returns the declaration and nothing else.
Both workers construct ConversionNotifications and only ever call
build(). So the behaviour the KDoc describes -- warning when progress
will be invisible -- does not happen, and a test would assert that a
function nobody calls returns what the platform told it. Green, vacuous,
and worse than nothing, because it would imply the disabled-notification
case is handled.
Recorded rather than tested, and the summary now names what F1 and F5
have in common: a comment describing behaviour the code lacks, where the
tempting fix freezes the wrong answer.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The ConversionForegroundType note asserted that #122's wedge no longer
kills the API 33 leg, on the evidence of a single run. The PR carrying
this document then wedged that exact leg: 23m08s, "wedged: yes --
gradle was killed after 1200s and never returned", failed: unknown.
Corrected to what the runs actually show: intermittent, not resolved --
five of the last six completed legs passed in ~7 minutes. And the
distinction the wedge row exists to draw is now stated, because it is
what keeps #88's reasoning intact: received: 60 means all sixty tests
still reported, so the API 33 regime was exercised; it is the failed
count that reads "unknown", so the leg could not have reported a break.
Also names what that changes -- a @Config(sdk = 33/34) JVM test is
worth three lines as insurance against a leg that cannot be trusted to
go red, which is a different and much smaller claim than the uncovered
behaviour this first looked like.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#132 holds the seven JVM test gaps from the same read, #133 the three
seam questions. The doc drew the line between them in prose already;
this makes it followable.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JMR-devandClaude Opus 5