Commit Graph
363 Commits
Author SHA1 Message Date
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
JMR-devandClaude Opus 5 aa7e1d8b01 C1 (#176): give ConcatWorker the seam ConversionWorker always had, and test what was behind it
ConcatWorker constructed ConcatEngine in place while ConversionWorker reached its engines
through ConversionDependencies. That asymmetry is the whole reason one worker had a tested
failure path and the other had none: everything past setForeground was untested on *every*
source set, JVM and device alike.

The repo had already measured the cost and written it down. PerJobStagingTest's KDoc
records that **reverting ConcatWorker to a constant staging name left all 257 tests
green**, because nothing could reach the line that names the file. RefusedJobTest says it
from the other side -- "the next thing past the count guard is ConcatEngine, which is
native". That mutation is red now.

The seam is `ConversionDependencies.concat: (Context) -> ConcatJoiner`, beside .hardware
and .software. ConcatEngine implements the interface; its Result type stays nested in the
implementation, because moving it would touch every call site to buy nothing -- what a
test needs is the ability to not run FFmpeg, and that is the method, not the type.

Five tests, and the mutations that hold them:

  the engine's own reason reaches the user   replace e.message with the generic string
  a failure with no message still says one   drop the ?: GENERIC_FAILURE_MESSAGE fallback
  a failed join deletes its partial          drop staged.delete() from the catch
  no input array at all is refused           swap in TOO_FEW_INPUTS_MESSAGE
  a join reports its own staged file         revert to the constant staging name

The delete test was vacuous on its first draft and the mutation caught it: it scanned the
staging directory for a "join-" prefix that StagingNames.forJob does not produce -- it
names files <jobId>.<ext> -- so the assertion was trivially true. Rewritten to assert
against the handle the joiner was actually given.

Two small fixes ride along, both the repo's own conventions rather than new opinions.
"No input files." becomes NO_INPUTS_MESSAGE, per #158: a message the user can see is
named once, so a test asserts the string the worker writes rather than a copy that can
drift. And the JVM now covers that arm, which ran before staging and before any native
code and had no business being a device test.

579 -> 584 JVM tests, 0 failures.
ConcatWorker: 14 -> 5 missed lines, 2 -> 0 missed branches.
Line 2173/2348 -> 2183/2352; branch 1091/1342 unchanged in the numerator.

The line denominator moved 2348 -> 2352: that is the ConcatJoiner interface, not new
untested code. Said plainly because CLAUDE.md's coverage entry has a history of explaining
its own numbers wrongly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-01 22:06:52 -05:00
JMR-devandClaude Opus 5 794cef7b34 C2 (#177): cut MediaProbe's two-probe merge into a seam, and ask which probe wins
probe() runs MediaExtractor and FFprobe independently and merges the two, and every rule
in that merge is a decision nothing held. The reason is structural rather than an
oversight: RemuxTest drives the whole thing on a device against committed fixtures, but
only ever with one probe answering and the other agreeing or also failing. Nothing on any
source set can arrange for a real extractor and a real FFprobe to *disagree*, so every
elvis in the merge was taken in one direction and never the other.

The seam is `internal fun merge(Extracted?, FFprobeInfo?): InputProbe`, pulled out of
probe() whole -- probe() now reads the two probes, merges, and keeps the log. FFprobeInfo
becomes internal alongside it; Extracted already was, with a KDoc giving this exact reason,
and FFprobeInfo simply never got the same treatment. Half a signature being private is
what made the function unnameable from a test.

Eleven tests, and the mutations that hold them:

  image beats a real video codec      demote the isImage arm below the video arm
  the extractor wins on codecs        flip the elvis to FFprobe-first
  duration is the larger reading      replace maxOf with extractor-first
  dimensions prefer the extractor     flip the width elvis
  no recognised stream is unreadable  narrow the guard to `extracted == null && info == null`

All five red, then restored. One mutation I tried first was *semantically equivalent* --
moving the image arm above the both-null arm changes nothing for any reachable input -- so
it stayed green and is recorded here rather than counted: a green mutation is only evidence
when the mutation is a real change.

The last row is the arm the ticket was filed for: parsed, and carrying no stream either
probe recognised, which is what a container holding only subtitles looks like. Its input
was already being constructed elsewhere in the suite -- MediaProbeTrackWalkTest calls
extractedFrom(emptyList()) and gets exactly it -- and had never been handed to the merge.

568 -> 579 JVM tests, 0 failures.
MediaProbe: 35 -> 24 missed lines, 72 -> 40 missed branches.
Line 2103/2348 -> 2173/2348; branch 1029/1340 -> 1091/1342.

The branch denominator moved by two, and it is the seam that moved it -- worth stating
separately from the numerator, because CLAUDE.md's coverage entry has a documented history
of explaining its own numbers wrongly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-01 21:59:00 -05:00
JMR-devandClaude Opus 5 eded47d666 B1 (#173): tell the rail from the bottom bar, and the Convert tab from the Join tab
Two assertion gaps, not coverage gaps, which is why they lasted. AppRootRestorationTest
already drives AppRoot at Compact and Expanded, so JaCoCo is green on useRail -- but it
asserts only that the selected tab survives recreation, through a stub `content`
composable. Nothing anywhere queried for a rail or a bar, and nothing composed the real
screens. Measured before this file existed:

  - transposing the NavigationRail and NavigationBar bodies passed the entire suite
  - transposing Content's two arms passed it too

A tablet showing phone chrome, or the Convert tab opening the Join screen, and 546 tests
with nothing to say about either. AppRoot's own KDoc is why that matters more than it
looks: from targetSdk 37 the app is resized and rotated whether or not it is ready, so the
width class is not a preference.

WindowWidthSizeClass.Medium appears in no test in either source set today. useRail is
`!= Compact`, so Medium takes the rail; narrowing it to `== Expanded` is one character and
breaks every tablet and unfolded foldable. That mutation is red now, and it is red only
because of the Medium test -- the Compact and Expanded ones both survive it.

Two things this needed:

**createAndroidComposeRule rather than createComposeRule.** Rendering AppRoot with its
default content reaches ConverterScreen's `viewModel = viewModel()`, which needs a
ViewModelStoreOwner. It works because both ViewModels are
`@JvmOverloads constructor(app: Application, ...)` so AndroidViewModelFactory can build
them, and because ui-test-manifest's debugImplementation entry already puts a
ComponentActivity in the merged manifest the unit tests build against -- which
app/build.gradle.kts says in terms. Checked with a throwaway spike before the ticket was
filed, rather than discovered here.

**Two tags, applied inside main.** The only production change: TestTags.Shell, set on the
rail and the bar. There is no other way to tell the two apart -- both render the same two
destinations with the same labels and the same selection state, so any assertion writable
without them is satisfied by either layout. In TestTags and applied by the shell rather
than handed down by the test, for the reason that file's KDoc gives: a tag the test
supplies proves only that the test set it. TagTableUniquenessTest covers the new group.

564 -> 568 JVM tests, 0 failures.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-01 21:54:50 -05:00
JMR-devandClaude Opus 5 b41341a1cb B3 (#175): pin the AAC arm every ordinary conversion takes
audioArgs has six arms. Five are named codecs with tests; AAC arrives through the `else`,
so nothing named it -- neither "aac" nor "192k" appeared anywhere in
FFmpegCommandBuilderTest. It is the audio MP4 and M4A get, which is to say the audio the
picker offers first and most conversions produce.

Both halves are asserted, and the bitrate is the half worth arguing for: an -b:a that
quietly changed would fail nothing, look wrong in no command line, and surface only as
files that sound different from the ones the app produced last month. Both mutations
confirmed red -- 192k -> 128k and aac -> libfdk_aac.

Asserted through MP4_H264 and M4A_AAC rather than one of them, so an AAC arm added above
the `else` later has to keep answering the same way for both.

**Deliberately not added here: an ENCODABLE_AUDIO-vs-audioArgs agreement test**, the
obvious companion to VideoCodecMimeAgreementTest. It would freeze the answer to F1, which
is open: ContainerCapabilities.kt:84 says "nothing here emits a Vorbis encoder" and
FFmpegCommandBuilder.kt:188 does. docs/coverage-read-findings.md says in terms that the
tempting fix there locks in the wrong answer and that the decision comes first. This is
the AAC arm only.

563 -> 564 JVM tests, 0 failures. No production code changed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-01 21:50:31 -05:00
JMR-devandClaude Opus 5 0f842243b5 B2 (#174): read what the progress notification actually says
An assertion gap rather than a coverage one, which is the reason it survived. JaCoCo is
green on build()'s `if (indeterminate)` because ProgressNotificationTest drives it through
a real worker -- but that test reads the notification id and EXTRA_PROGRESS and nothing
else. Nothing had ever read the text. Swapping the two branches passed the whole suite;
so did replacing the caller's title with a constant. Both are red now.

What it costs to get wrong is small and permanent: a conversion four minutes in still
saying "Preparing", or one that has not started reporting yet claiming 0%. Neither is a
crash, and nothing else here would have found it.

Nothing in the suite had constructed ConversionNotifications directly, and the reason
turned out to be mechanical rather than an oversight: build() reaches
WorkManager.getInstance for the Cancel action's PendingIntent, so the notification cannot
be built without one. installTestWorkManager in setUp is the whole fixture, and the KDoc
records the coupling so the next person does not rediscover it.

areEnabled() in the same file is deliberately still untested. It has no caller anywhere in
app/src/main, so a test would assert that a function nobody calls returns what the platform
told it -- and would imply the app handles the disabled-notification case, which it does
not. That is F5 in docs/coverage-read-findings.md, and it asks for a decision rather than a
test.

561 -> 563 JVM tests, 0 failures. No production code changed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-01 21:48:36 -05:00
JMR-devandClaude Opus 5 2fbc957119 A5 (#171): fire the muxer guard that repairs "MP4 for everything", which had never fired
Media3Muxers' KDoc names the defect this guards -- "the router claimed five containers
while the engine silently wrote MP4 for all of them" -- and the repair itself was
untested: Media3Engine$buildTransformer$3, the requireNotNull message lambda, was four
lines and four branches at 0%. Nothing had ever driven a plan whose container Media3
cannot mux, and factoryFor answers null for fourteen of them.

Weakening it does not crash. The wrong output is a playable file with the wrong
container, which is why a test rather than a bug report is what would catch it.

Same harness and the same two disciplines as Media3EngineEmptyCompositionTest, which is
the sibling this joins: assert the plan really is the one the test needs before driving
the engine, and rule out CancellationException so an unresumed continuation cannot read
as a pass. Three premises are asserted here rather than assumed -- that the plan is still
WebM by the time the engine sees it, that Media3 really has no muxer for WebM, and that
neither track was dropped, since the empty-composition refusal fires earlier and is a
different test's subject.

The assertion is on the exception type *and* its message, and the ticket predicted why:
replacing requireNotNull with `?: DefaultMuxer.Factory()` does not make the export
succeed, it lets it run on and fail some other way. Measured -- that mutation fails the
type assertion, so the guard is genuinely what this test is holding, and the message
assertion stands behind it.

560 -> 561 JVM tests, 0 failures. No production code changed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-01 21:45:43 -05:00
JMR-devandClaude Opus 5 a645442acc A2 + A3 (#168, #169): the hardware fallback, the cancellation that must not take it, and the name a job may not have
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>
2026-09-01 21:42:27 -05:00
JMR-devandClaude Opus 5 5761faced6 A4 (#170): join the two halves of an unreadable join clip, and record why the catch arm stays device-only
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>
2026-09-01 21:37:45 -05:00
JMR-devandClaude Opus 5 c2c0bfa848 A6 (#172): six one-branch outcomes nothing produced, and one that cannot be produced
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>
2026-09-01 21:31:21 -05:00
JMR-devandClaude Opus 5 016030f3e4 A1 (#167): pin all three foreground-service regimes, and the boundary between two of them
`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>
2026-09-01 21:19:06 -05:00
Jason Ross d354f6470c Merge pull request #166 from JMR-dev/docs/coverage-wave2
Re-measure coverage after wave 2, and write down how a stacked PR merges
2026-08-29 11:33:20 -05:00
JMR-devandClaude Opus 5 dbedfb4708 Re-measure coverage after wave 2, and write down how a stacked PR merges
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>
2026-08-29 11:16:20 -05:00
Jason Ross c6e9a480e1 Merge pull request #165 from JMR-dev/test/screen-wiring
W3: the screen wiring, and a narrower hazard than the ticket claimed
2026-08-29 11:13:34 -05:00
Jason Ross d8110d7a62 Merge pull request #164 from JMR-dev/test/viewmodel-setters
W4: the seven settings edits, and three tests that did not bite until they did
2026-08-29 10:57:14 -05:00
JMR-devandClaude Opus 5 6f3966cc69 W3 (#156): the screen wiring, and a narrower hazard than the ticket claimed
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>
2026-08-29 10:56:44 -05:00
Jason Ross 7595177e81 Merge pull request #163 from JMR-dev/test/join-state-mapping
W2: the join state mapping, and a crash the seam exposed
2026-08-29 10:49:03 -05:00
JMR-devandClaude Opus 5 a507736d3d W4 (#157): the seven settings edits, and three tests that did not bite until they did
`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>
2026-08-29 10:46:48 -05:00
JMR-dev 1ff5c4463c Merge remote-tracking branch 'origin/main' into test/join-state-mapping 2026-08-29 10:39:55 -05:00
Jason Ross 70337b2c12 Merge pull request #162 from JMR-dev/test/conversion-state-mapping
W1: cut the conversion state mapping into a seam, and choose all six arms
2026-08-29 10:38:52 -05:00
JMR-devandClaude Opus 5 fea480b000 W2 (#155): the join state mapping, and a crash the seam exposed
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>
2026-08-29 10:37:13 -05:00
JMR-devandClaude Opus 5 ddfb1dd78e W1 (#154): cut the conversion state mapping into a seam, and choose all six arms
`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>
2026-08-29 10:23:52 -05:00
Jason Ross 348eaa2f22 Merge pull request #161 from JMR-dev/test/dedupe-user-messages
W5: one sentence per user-facing condition, not two
2026-08-29 10:14:08 -05:00
JMR-devandClaude Opus 5 104d02de03 W5 (#158): one sentence per user-facing condition, not two
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>
2026-08-29 10:05:27 -05:00
Jason Ross 90814222b7 Merge pull request #160 from JMR-dev/fix/restore-stack-merges
Restore the five stacked PRs that merged into their bases instead of main
2026-08-29 09:09:46 -05:00
JMR-devandClaude Opus 5 2d4898ad44 Re-measure the coverage entry against the tree this branch creates
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>
2026-08-27 09:01:40 -05:00
JMR-dev 83ac7eff2c Merge commit '79cca0e' into fix/restore-stack-merges 2026-08-27 08:59:45 -05:00
Jason Ross b2c11bbfe0 Merge pull request #151 from JMR-dev/test/outputpublisher-seams
S2 + S3: the two OutputPublisher seams, and where the second one actually goes
2026-08-27 08:54:30 -05:00
JMR-devandClaude Opus 5 3a5210ec5d S2 + S3 (#142, #143): the two OutputPublisher seams, and where the second one goes
#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>
2026-08-27 07:22:38 -05:00
Jason Ross 5f3eda9c40 Merge pull request #149 from JMR-dev/test/outputpublisher-partial-branches
C6: OutputPublisher's guarded branches, three of them guarding a delete
2026-08-27 07:22:35 -05:00
JMR-dev 79cca0eb47 Merge branch 'test/refused-jobs' into test/mediaprobe-track-seam 2026-08-27 07:19:42 -05:00