Compare commits

..
Author SHA1 Message Date
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
12 changed files with 1129 additions and 67 deletions
+44 -3
View File
@@ -130,9 +130,9 @@ install for code that can never run — and on API 37 the full APK does not fit
- The `model` package is excluded from `ReturnCount` and `CyclomaticComplexMethod` only. It is the
decision layer, where one branch is one documented user-visible outcome and the metric counts
answers rather than complexity. Every other rule still applies there.
- **Coverage is reported, not gated** — **87.1% of lines (2025/2324), 69.1% of branches
(974/1410)**, measured 2026-08-27 with `./gradlew :app:jacocoTestReport`, against 502 JVM tests
in 71 classes.
- **Coverage is reported, not gated** — **88.9% of lines (2087/2348), 75.4% of branches
(1011/1340)**, measured 2026-08-29 with `./gradlew :app:jacocoTestReport`, against 546 JVM tests
in 76 classes.
**Every figure this file carried before 2026-08-24 was an artifact, roughly half the real one.**
Robolectric loads classes through its own sandbox classloader with no source location, JaCoCo
@@ -159,6 +159,21 @@ install for code that can never run — and on API 37 the full APK does not fit
chooses a branch the suite had never taken. Line coverage barely notices; branch coverage is the
whole point.
Then #153's five children on 2026-08-29 — 87.1% -> 88.9% line, 69.1% -> **75.4%** branch, 502 ->
546 tests.
**That last branch figure moved for two reasons, and 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 what is left is a plain function whose branches a test can choose:
`ConversionViewModel$observe$1$1` went from carrying the whole mapping to 6 branches, while the
extracted `ConversionViewModelKt` covers 41 of 42 and `JoinViewModelKt` 38 of 39. So a seam is
worth more than the tests it enables — it also stops the measurement counting scaffolding.
Be careful quoting a branch move on its own for that reason. A percentage that rises because the
denominator shrank is not the same claim as one that rises because more branches are tested, and
this entry has a history of explaining its own numbers wrongly.
And **re-measure before quoting**: this entry was once written quoting 81.4%, measured four hours
earlier, and was already three points stale by the time it was ready to merge.
- **Testable code is not done until it is tested.** If a piece is unit testable, it gets unit
@@ -182,6 +197,32 @@ install for code that can never run — and on API 37 the full APK does not fit
- `kotlin.code.style=official`. Gradle stays Kotlin DSL.
- **A stacked PR does not merge with `gh pr merge`, and `MERGED` is not proof it reached `main`.**
Two separate traps, both measured on 2026-08-27 while landing #144-#151.
`gh pr merge` uses the GraphQL mutation, which refuses a stacked PR outright: *"This pull request
is part of a stack and must be merged using the asynchronous merge REST API."* So does
`PUT .../pulls/{n}/merge`. The one that works is
`gh api -X PUT repos/OWNER/REPO/pulls/N/merge-async -f merge_method=merge`, which returns a uuid
to poll at `.../merge-async/{uuid}` until `status` is `merged` or `failed`.
**The second trap is worse, because nothing looks wrong.** GitHub retargets a stacked PR's base
to `main` when the PR below it merges, but *asynchronously*. Merge a stack faster than that
settles — five PRs about thirty seconds apart, in the case that found this — and each one merges
into its own base branch, which has itself already been merged and left behind. Every call
returns `status: merged` and every one is true. `gh pr list --state open` comes back empty, every
PR shows `MERGED`, and **none of the content is on `main`**.
What caught it was a coverage re-measure reading two points lower than the same tree had measured
an hour earlier; a fresh `git pull` changed nothing, which is what turned it into a question.
`git merge-base --is-ancestor <merge-sha> origin/main` answers it in one line. Do that after
merging a stack, or merge one at a time and re-read `baseRefName` between. #160 is what the
recovery cost.
The auto-retarget belongs to the stacking feature specifically. A PR opened with a plain
`gh pr create --base some-branch` does **not** retarget when that branch merges — it is left
pointing at a dead base and has to be moved by hand.
- **File one-off issues with `tools/github/file-issue.sh`, not `gh issue create`.** `gh issue
create` does not touch the project board, so the issue exists, carries its labels, and is
invisible in the Kanban — indistinguishable from never having been filed. Measured 2026-08-24:
@@ -94,24 +94,61 @@ fun ConverterScreen(modifier: Modifier = Modifier, viewModel: ConversionViewMode
state = state,
settings = settings,
validation = validation,
actions = ConverterActions(
actions = converterActions(
viewModel = viewModel,
onPickInput = { pickInput.launch(arrayOf("*/*")) },
onPreset = viewModel::setPreset,
onContainer = viewModel::setContainer,
onVideoCodec = viewModel::setVideoCodec,
onAudioCodec = viewModel::setAudioCodec,
onSuggestion = viewModel::applySuggestion,
onQuality = viewModel::setQuality,
onEnginePreference = viewModel::setEnginePreference,
onConvert = { requestNotifications.launch(Manifest.permission.POST_NOTIFICATIONS) },
onCancel = viewModel::cancel,
onSave = { suggestedName -> chooseDestination.launch(suggestedName) },
onReset = viewModel::reset,
),
modifier = modifier,
)
}
/**
* Which of the ViewModel's methods each affordance on the screen calls.
*
* ## Why this is a function rather than an argument list
*
* It was an argument list, inside [ConverterScreen], which no test reached: `ConverterScreenContent`
* builds its own [ConverterActions], so every test in the suite drove the stateless inner and none
* of them ever saw the wiring.
*
* Most of the list is safe without a test, and saying so is more useful than pretending otherwise:
* `onContainer`, `onVideoCodec`, `onAudioCodec`, `onPreset`, `onSuggestion`, `onQuality` and
* `onEnginePreference` each take a distinct type, so binding one to another's setter does not
* compile. Verified rather than assumed — swapping `onVideoCodec` and `onAudioCodec` fails with
* *"Inapplicable candidate(s): fun setAudioCodec(codec: AudioCodec)"*.
*
* **[ConverterActions.onCancel] and [ConverterActions.onReset] are the exception.** Both are
* `() -> Unit`, so swapping them compiles silently — also verified — and ships a Cancel button that
* throws the conversion away and a Start-over button that leaves it on screen. That pair is what
* `ConverterWiringTest` exists for.
*
* The three launcher-backed actions stay parameters: they need an `ActivityResultLauncher`, which
* is the part that genuinely needs the composition, and keeping them out means the rest can be
* checked without one.
*/
@UnstableApi
internal fun converterActions(
viewModel: ConversionViewModel,
onPickInput: () -> Unit,
onConvert: () -> Unit,
onSave: (suggestedName: String) -> Unit,
): ConverterActions = ConverterActions(
onPickInput = onPickInput,
onPreset = viewModel::setPreset,
onContainer = viewModel::setContainer,
onVideoCodec = viewModel::setVideoCodec,
onAudioCodec = viewModel::setAudioCodec,
onSuggestion = viewModel::applySuggestion,
onQuality = viewModel::setQuality,
onEnginePreference = viewModel::setEnginePreference,
onConvert = onConvert,
onCancel = viewModel::cancel,
onSave = onSave,
onReset = viewModel::reset,
)
/**
* Everything [ConverterScreenContent] can ask for, in one value.
*
@@ -56,17 +56,38 @@ fun JoinScreen(modifier: Modifier = Modifier, viewModel: JoinViewModel = viewMod
JoinScreenContent(
state = state,
actions = JoinActions(
actions = joinActions(
viewModel = viewModel,
onPickInputs = { pickInputs.launch(arrayOf("video/*")) },
onJoin = viewModel::join,
onCancel = viewModel::cancel,
onSave = { suggestedName -> chooseDestination.launch(suggestedName) },
onReset = viewModel::reset,
),
modifier = modifier,
)
}
/**
* Which of the ViewModel's methods each affordance on the join screen calls.
*
* The join-side twin of `converterActions`, and the transposition risk here is worse: **three**
* `() -> Unit` bindings rather than two. `onJoin`, `onCancel` and `onReset` are mutually
* interchangeable as far as the compiler is concerned, so a Join button that cancels, or a Cancel
* button that starts the job, is a swap nothing but a test would catch.
*
* See `converterActions` for why the launcher-backed actions stay parameters.
*/
@UnstableApi
internal fun joinActions(
viewModel: JoinViewModel,
onPickInputs: () -> Unit,
onSave: (suggestedName: String) -> Unit,
): JoinActions = JoinActions(
onPickInputs = onPickInputs,
onJoin = viewModel::join,
onCancel = viewModel::cancel,
onSave = onSave,
onReset = viewModel::reset,
)
/**
* Everything [JoinScreenContent] can ask for, in one value.
*
@@ -5,6 +5,7 @@ import android.net.Uri
import androidx.lifecycle.AndroidViewModel
import androidx.lifecycle.viewModelScope
import androidx.media3.common.util.UnstableApi
import androidx.work.Data
import androidx.work.WorkInfo
import androidx.work.WorkManager
import kotlinx.coroutines.CoroutineDispatcher
@@ -30,6 +31,108 @@ import org.libremediaconverter.work.jobSnapshots
import java.io.File
import java.util.UUID
/**
* One update about a running join, as WorkManager last reported it.
*
* The join-side twin of `ConversionUpdate`, and deliberately the same shape: this pair of seams is
* one refactor done twice, and letting them diverge would make the two flows harder to compare than
* the duplication costs. There is no `progressPercent` here because `ConcatWorker` publishes none —
* a join is indeterminate.
*/
internal data class JoinUpdate(val state: WorkInfo.State, val runAttemptCount: Int, val outputData: Data)
/**
* What the join screen should show, given what WorkManager last said about the job.
*
* The join-side twin of `conversionStateFrom`, extracted for the same reason and with the same two
* exclusions: the ownership check stays at the call site, and this takes no responsibility for the
* staged file. See that function's KDoc for the argument in full.
*
* Five arms had never been chosen by any test before this was cut out, because a real `ConcatWorker`
* only ever produces a terminal state with well-formed output.
*
* @param cancelled where a cancellation lands, which differs for a reattached job — see [observe].
*/
@UnstableApi
internal fun joinStateFrom(update: JoinUpdate, inputs: List<InputFile>, cancelled: JoinState): JoinState =
when (update.state) {
// BLOCKED is a job waiting on a prerequisite, which the user has nothing to do about and
// nothing useful to be told about. It reads as "starting", like a fresh ENQUEUED.
WorkInfo.State.RUNNING, WorkInfo.State.BLOCKED -> JoinState.Joining(inputs)
// ENQUEUED after a run means a retry is pending -- the same rule, and the same reasoning, as
// the convert side. See `conversionStateFrom`.
WorkInfo.State.ENQUEUED ->
if (update.runAttemptCount > 0) {
JoinState.Waiting(inputs)
} else {
JoinState.Joining(inputs)
}
WorkInfo.State.SUCCEEDED -> joinedFrom(update.outputData)
// A worker that dies before it can report anything leaves no output data at all, and an
// exception's message can be an empty string. Both would read as a failure with nothing said,
// so blank falls back like missing does.
WorkInfo.State.FAILED -> JoinState.Failed(
update.outputData.getString(ConcatWorker.KEY_ERROR)
?.takeIf { it.isNotBlank() }
?: ConcatWorker.GENERIC_FAILURE_MESSAGE,
)
WorkInfo.State.CANCELLED -> cancelled
}
/**
* The `SUCCEEDED` arm. A join that reported success and named no file has nothing to offer.
*/
@UnstableApi
private fun joinedFrom(outputData: Data): JoinState {
val path = outputData.getString(ConcatWorker.KEY_OUTPUT_PATH)
?: return JoinState.Failed(JOINED_WITHOUT_A_FILE_MESSAGE)
return JoinState.Joined(
staged = File(path),
strategy = strategyFrom(outputData.getString(ConcatWorker.KEY_STRATEGY)),
// A join enqueued before the worker reported these carries neither, and the fallback is
// the format such a job really used -- ConcatWorker.request has always defaulted to it,
// and the join screen has never offered a choice.
suggestedName = outputData.getString(ConcatWorker.KEY_SUGGESTED_NAME)
?.takeIf { it.isNotBlank() }
?: ConcatWorker.outputNameFor(ConcatWorker.DEFAULT_FORMAT),
mimeType = outputData.getString(ConcatWorker.KEY_MIME_TYPE)
?.takeIf { it.isNotBlank() }
?: ConcatWorker.DEFAULT_FORMAT.mimeType,
)
}
/**
* The strategy a finished join reported, or [ConcatStrategy.REENCODE] when it named none.
*
* **Looked up rather than `valueOf`, and that is a fix rather than a style choice.** `valueOf`
* throws `IllegalArgumentException` on a name this build does not define, and this runs inside a
* `viewModelScope` collect with no handler -- so the throw does not become a `Failed` state, it
* takes the process down.
*
* Reachable for the reason `WorkerEnumFallbackTest` and `JobTags` are both written on: WorkManager
* keeps finished work for about a week, so a downgrade or a rollback hands this build a job
* enqueued by another one. `ConcatWorker` writes `result.strategy.name` into the output `Data`, so
* a build that added a third strategy would leave this one crashing on its own completed joins.
*
* `ConcatWorker.kt` already made exactly this change for `KEY_FORMAT`, and says why in as many
* words: *"Looked up rather than `valueOf` … a format name this build does not define used to throw
* past the catch."* The same read on this side had not been changed with it.
*
* REENCODE is the safe 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 conservative of the
* two rather than being claimed as a lossless stream copy.
*/
private fun strategyFrom(name: String?): ConcatStrategy =
ConcatStrategy.entries.firstOrNull { it.name == name } ?: ConcatStrategy.REENCODE
/** A join that reported success and named no file. There is nothing to offer the user to save. */
internal const val JOINED_WITHOUT_A_FILE_MESSAGE: String =
"Joining reported success but produced no file."
sealed interface JoinState {
data object Idle : JoinState
data class Ready(val inputs: List<InputFile>) : JoinState
@@ -251,56 +354,19 @@ class JoinViewModel @JvmOverloads constructor(
// takes ownership of the staged file, and a superseded observation must not do
// that either.
if (!ownership.stillHeldBy(token)) return@collect
_state.value = when (info.state) {
WorkInfo.State.RUNNING, WorkInfo.State.BLOCKED -> JoinState.Joining(inputs)
WorkInfo.State.ENQUEUED ->
if (info.runAttemptCount > 0) {
JoinState.Waiting(inputs)
} else {
JoinState.Joining(inputs)
}
WorkInfo.State.SUCCEEDED -> {
val path = info.outputData.getString(ConcatWorker.KEY_OUTPUT_PATH)
val strategy = info.outputData.getString(ConcatWorker.KEY_STRATEGY)
?.let(ConcatStrategy::valueOf) ?: ConcatStrategy.REENCODE
if (path == null) {
JoinState.Failed("Joining reported success but produced no file.")
} else {
val staged = File(path)
// Take responsibility for the file at the same moment the state
// starts referring to it, so the two cannot disagree.
pendingStaged = staged
JoinState.Joined(
staged = staged,
strategy = strategy,
// A join enqueued before the worker reported these carries
// neither, and the fallback is the format such a job really
// used -- ConcatWorker.request has always defaulted to it, and
// the join screen has never offered a choice.
suggestedName = info.outputData
.getString(ConcatWorker.KEY_SUGGESTED_NAME)
?.takeIf { it.isNotBlank() }
?: ConcatWorker.outputNameFor(ConcatWorker.DEFAULT_FORMAT),
mimeType = info.outputData
.getString(ConcatWorker.KEY_MIME_TYPE)
?.takeIf { it.isNotBlank() }
?: ConcatWorker.DEFAULT_FORMAT.mimeType,
)
}
}
// A worker that dies before it can report anything leaves no output data at
// all, and an exception's message can be an empty string. Both would read as
// a failure with nothing said, so blank falls back like missing does.
WorkInfo.State.FAILED -> JoinState.Failed(
info.outputData.getString(ConcatWorker.KEY_ERROR)
?.takeIf { it.isNotBlank() }
?: ConcatWorker.GENERIC_FAILURE_MESSAGE,
)
WorkInfo.State.CANCELLED -> cancelled
}
val next = joinStateFrom(
JoinUpdate(
state = info.state,
runAttemptCount = info.runAttemptCount,
outputData = info.outputData,
),
inputs = inputs,
cancelled = cancelled,
)
// Read off the result rather than assigned inside the mapping -- see the same
// three lines in ConversionViewModel for why that is the better half of the swap.
if (next is JoinState.Joined) pendingStaged = next.staged
_state.value = next
}
}
}
@@ -7,6 +7,7 @@ import org.junit.Assert.assertNull
import org.junit.Assert.assertTrue
import org.junit.Test
import org.libremediaconverter.model.CodecNames
import org.libremediaconverter.model.InputProbe
import org.libremediaconverter.model.VideoCodec
/**
@@ -133,6 +134,38 @@ class CodecVocabularyTest {
* landed, a device with no HEVC decoder answered true for `x265` and Media3 was handed a job it
* could not do; now the router sends it to FFmpeg without spending the attempt.
*/
/**
* The sentinel is not just another unknown name, and the difference is the whole guard.
*
* `canDecode` ends `?: true` -- a name neither table knows keeps the permissive answer, because
* the app would rather try than refuse a file it might handle. `InputProbe.UNPARSEABLE` has to
* be the exception: the platform has *already* failed to parse the input, so there is nothing
* for a decoder to be permissive about, and waving it through spends a Media3 attempt on a job
* that cannot start.
*
* The `cinepak` line is what makes the sentinel line mean something. Without it, deleting the
* early return leaves this test green -- both names would fall through to the same `?: true`.
* The pair is the assertion.
*
* `DeviceCodecs.PERMISSIVE` carries the same rule and `ConversionRouterTest` pins its routing
* consequence. This is the implementation that runs on a device.
*/
@Test
fun `the unparseable sentinel is refused even where an unknown name is waved through`() {
val everything = AndroidDeviceCodecs.forTesting(
encoders = emptySet(),
decoders = setOf("video/avc", "video/hevc"),
)
assertFalse(
"the platform could not parse this input, so there is nothing to decode with",
everything.canDecode(InputProbe.UNPARSEABLE),
)
assertTrue(
"a merely unknown name still keeps the permissive answer",
everything.canDecode("cinepak"),
)
}
@Test
fun `a device without the decoder now says so for the aliases it used to wave through`() {
val hevcOnly = AndroidDeviceCodecs.forTesting(encoders = emptySet(), decoders = setOf("video/hevc"))
@@ -232,6 +232,12 @@ class OutputPublisherPublishTest {
RowShape.NO_SIZE_COLUMN to "a cursor with no SIZE column",
RowShape.NULL_SIZE to "a cursor whose SIZE cell is null",
RowShape.NO_ROWS to "a cursor holding no rows",
// The third case the KDoc names -- "a resolver call that throws" -- and the one the
// list was missing. It reaches `?: false` through `runCatching` rather than through a
// cursor answer, so it is the only one of the four that proves the catch is load
// bearing: a provider that revokes its grant between the picker and the write must not
// have its document deleted on the way out.
RowShape.QUERY_THROWS to "a provider that throws out of query",
).forEach { (shape, description) ->
FakeSafProvider.deleteRequests.clear()
FakeSafProvider.backingFile(documentUri).writeBytes(ByteArray(0))
@@ -0,0 +1,241 @@
package org.libremediaconverter.convert
import android.app.Application
import android.net.Uri
import androidx.media3.common.util.UnstableApi
import androidx.work.Data
import androidx.work.workDataOf
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertNotEquals
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.join.JoinState
import org.libremediaconverter.join.JoinViewModel
import org.libremediaconverter.join.joinActions
import org.libremediaconverter.model.AudioCodec
import org.libremediaconverter.model.Container
import org.libremediaconverter.model.EnginePreference
import org.libremediaconverter.model.OutputFormat
import org.libremediaconverter.model.QualityTier
import org.libremediaconverter.model.VideoCodec
import org.libremediaconverter.work.ConcatWorker
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment
/**
* That each affordance is wired to the ViewModel method it is named after.
*
* ## What this covers that no other test can
*
* `ConverterScreenContentTest`, `ConverterStateAffordancesTest` and `JoinScreenContentTest` all
* drive the **stateless** content composables, which build their own `ConverterActions`. So the
* wiring — the list of `viewModel::` references the stateful outer hands down — was seen by nothing
* in the suite.
*
* ## The hazard is narrower than "seventeen bindings", and this says so
*
* #156 was filed claiming a transposition of any two bindings would survive the suite. That is not
* true, and it was worth checking rather than testing on the assumption:
*
* | swap | result |
* |---|---|
* | `onVideoCodec` ↔ `onAudioCodec` | **rejected by the compiler** |
* | `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 for
* those would be theatre.
*
* **The `() -> Unit` bindings are the real gap**, because they are interchangeable to the compiler:
* two on the converter screen (`onCancel`, `onReset`) and three on the join screen (`onJoin`,
* `onCancel`, `onReset`). A Cancel that discards the finished file, or a Join that cancels, is a
* one-character mistake that ships.
*
* ## How they are told apart
*
* By effect, not by a recording double. `reset()` sets the state to `Idle`; `cancel()` with no
* active job leaves it alone (`ConversionViewModel.cancel` is `activeWorkId?.let(...)`, and
* `SettingsEditsTest` pins that). Driving each from a non-`Idle` state is therefore enough to say
* which one ran.
*/
@UnstableApi
@RunWith(RobolectricTestRunner::class)
class ScreenWiringTest {
private lateinit var app: Application
@Before
fun setUp() {
app = RuntimeEnvironment.getApplication()
ConversionDependencies.publisher = { RecordingPublisher(app) }
ConversionDependencies.probe = { _, _ -> org.libremediaconverter.model.InputProbe() }
}
@After
fun tearDown() {
ConversionDependencies.reset()
}
// --- the converter screen ----------------------------------------------
@Test
fun `Start over resets, and Cancel does not`() {
// The transposition that compiles. If onReset were bound to cancel, this stays on Ready.
installTestWorkManager(app, Data.EMPTY)
val pick = ParkedPickDispatcher()
val viewModel = ConversionViewModel(app, pickDispatcher = pick)
val actions = converterActions(viewModel, onPickInput = {}, onConvert = {}, onSave = {})
viewModel.onInputPicked(INPUT_URI)
pick.runAll()
assertNotEquals(
"the fixture needs a non-Idle state or neither action is observable",
ConversionState.Idle,
viewModel.state.value,
)
actions.onReset()
assertEquals(ConversionState.Idle, viewModel.state.value)
}
@Test
fun `Cancel leaves the picked file on screen`() {
// The other half. Without it, a wiring with BOTH actions bound to reset passes the test
// above -- and that is exactly what a copy-paste of the wrong line produces.
installTestWorkManager(app, Data.EMPTY)
val pick = ParkedPickDispatcher()
val viewModel = ConversionViewModel(app, pickDispatcher = pick)
val actions = converterActions(viewModel, onPickInput = {}, onConvert = {}, onSave = {})
viewModel.onInputPicked(INPUT_URI)
pick.runAll()
val before = viewModel.state.value
actions.onCancel()
assertEquals(
"Cancel must not throw away the pick the way Start over does",
before,
viewModel.state.value,
)
}
@Test
fun `each settings affordance reaches the setting it is named after`() {
// The typed bindings. The compiler already rejects a transposition among these, so this is
// not that assertion -- it is the cheaper one that each is bound to *something*, and that a
// binding dropped to `{}` during an edit would be caught.
installTestWorkManager(app, Data.EMPTY)
val pick = ParkedPickDispatcher()
val viewModel = ConversionViewModel(app, pickDispatcher = pick)
val actions = converterActions(viewModel, onPickInput = {}, onConvert = {}, onSave = {})
actions.onPreset(OutputFormat.WEBM_VP9)
assertEquals(OutputFormat.WEBM_VP9.spec, viewModel.settings.value.spec)
actions.onContainer(Container.MKV)
assertEquals(Container.MKV, viewModel.settings.value.spec.container)
actions.onVideoCodec(VideoCodec.H264)
assertEquals(VideoCodec.H264, viewModel.settings.value.spec.videoCodec)
actions.onAudioCodec(AudioCodec.FLAC)
assertEquals(AudioCodec.FLAC, viewModel.settings.value.spec.audioCodec)
actions.onQuality(QualityTier.BEST)
assertEquals(QualityTier.BEST, viewModel.settings.value.quality)
actions.onEnginePreference(EnginePreference.FORCE_SOFTWARE)
assertEquals(EnginePreference.FORCE_SOFTWARE, viewModel.settings.value.enginePreference)
actions.onSuggestion(OutputFormat.MP4_H264.spec)
assertEquals(OutputFormat.MP4_H264.spec, viewModel.settings.value.spec)
}
@Test
fun `the launcher-backed actions are the ones the screen supplies`() {
// Not wired to the ViewModel at all, deliberately -- they need an ActivityResultLauncher.
// Asserted so that a later edit routing one of them at the ViewModel is noticed.
installTestWorkManager(app, Data.EMPTY)
val pick = ParkedPickDispatcher()
val viewModel = ConversionViewModel(app, pickDispatcher = pick)
val called = mutableListOf<String>()
val actions = converterActions(
viewModel,
onPickInput = { called += "pick" },
onConvert = { called += "convert" },
onSave = { called += "save:$it" },
)
actions.onPickInput()
actions.onConvert()
actions.onSave("holiday.mp4")
assertEquals(listOf("pick", "convert", "save:holiday.mp4"), called)
}
// --- the join screen, where three are interchangeable -------------------
@Test
fun `Start over resets the join, and Cancel does not`() {
installTestWorkManager(app, workDataOf(ConcatWorker.KEY_OUTPUT_PATH to "/dev/null"))
val pick = ParkedPickDispatcher()
val viewModel = JoinViewModel(app, pickDispatcher = pick)
val actions = joinActions(viewModel, onPickInputs = {}, onSave = {})
viewModel.onInputsPicked(TWO_INPUTS)
pick.runAll()
assertTrue(
"the fixture needs a non-Idle state: ${viewModel.state.value}",
viewModel.state.value !is JoinState.Idle,
)
actions.onReset()
assertEquals(JoinState.Idle, viewModel.state.value)
}
@Test
fun `Cancel leaves the picked files on screen`() {
installTestWorkManager(app, workDataOf(ConcatWorker.KEY_OUTPUT_PATH to "/dev/null"))
val pick = ParkedPickDispatcher()
val viewModel = JoinViewModel(app, pickDispatcher = pick)
val actions = joinActions(viewModel, onPickInputs = {}, onSave = {})
viewModel.onInputsPicked(TWO_INPUTS)
pick.runAll()
val before = viewModel.state.value
actions.onCancel()
assertEquals(before, viewModel.state.value)
}
@Test
fun `Join starts the job rather than cancelling or resetting it`() {
// The third of the join screen's interchangeable trio, and the one whose transposition is
// worst: a Join button bound to cancel does nothing at all, which reads as a dead button.
installTestWorkManager(app, workDataOf(ConcatWorker.KEY_OUTPUT_PATH to "/dev/null"))
val pick = ParkedPickDispatcher()
val viewModel = JoinViewModel(app, pickDispatcher = pick)
val actions = joinActions(viewModel, onPickInputs = {}, onSave = {})
viewModel.onInputsPicked(TWO_INPUTS)
pick.runAll()
actions.onJoin()
assertTrue(
"Join must leave Ready for a running state, not sit still and not go Idle: " +
"${viewModel.state.value}",
viewModel.state.value is JoinState.Joining || viewModel.state.value is JoinState.Joined,
)
}
private companion object {
val INPUT_URI: Uri = Uri.parse("content://test/holiday.mov")
val TWO_INPUTS = listOf(
Uri.parse("content://test/a.mp4"),
Uri.parse("content://test/b.mp4"),
)
}
}
@@ -0,0 +1,197 @@
package org.libremediaconverter.convert
import android.app.Application
import androidx.media3.common.util.UnstableApi
import androidx.work.Data
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertNotEquals
import org.junit.Assert.assertNull
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.model.AudioCodec
import org.libremediaconverter.model.Container
import org.libremediaconverter.model.EnginePreference
import org.libremediaconverter.model.OutputFormat
import org.libremediaconverter.model.QualityTier
import org.libremediaconverter.model.VideoCodec
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment
/**
* The seven one-line edits the settings sheet makes, and what each one leaves alone.
*
* ## Why these needed a file of their own
*
* `setPreset` was covered. The six beside it — `setContainer`, `setVideoCodec`, `setAudioCodec`,
* `applySuggestion`, `setQuality`, `setEnginePreference` — and `cancel()` had **no coverage at
* all**, which is the tell: they are reachable from the JVM suite by exactly the route `setPreset`
* already takes, and nothing had asked.
*
* ## What is actually being 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 silently discards the other two**. So every test here asserts the field it changed
* *and* that the rest of the spec survived — a `setContainer` implemented as
* `it.copy(spec = OutputFormat.MP4_H265.spec.copy(container = container))` would pass a test that
* only checked the container.
*
* `ConverterScreenContentTest` cannot cover this: it builds `ConverterActions` itself and never
* touches the ViewModel. That the *screen* calls these is #156's, and neither implies the other.
*/
@UnstableApi
@RunWith(RobolectricTestRunner::class)
class SettingsEditsTest {
private lateinit var app: Application
private lateinit var viewModel: ConversionViewModel
@Before
fun setUp() {
app = RuntimeEnvironment.getApplication()
installTestWorkManager(app, Data.EMPTY)
viewModel = ConversionViewModel(app)
}
@After
fun tearDown() {
ConversionDependencies.reset()
}
@Test
fun `choosing a preset replaces the whole spec`() {
viewModel.setPreset(OutputFormat.WEBM_VP9)
assertEquals(OutputFormat.WEBM_VP9.spec, viewModel.settings.value.spec)
}
@Test
fun `changing the container leaves both codecs alone`() {
// Moved off the default spec first, and that is load-bearing rather than tidiness. The
// default IS `OutputFormat.MP4_H265.spec`, so a `setContainer` that rebuilt the spec from
// that preset instead of from the current one produced an identical answer and the
// mutation went green. Editing the codecs away from the default first is what makes
// "the other two survived" an assertion rather than a coincidence.
viewModel.setPreset(OutputFormat.WEBM_VP9)
val before = viewModel.settings.value.spec
viewModel.setContainer(Container.MKV)
val after = viewModel.settings.value.spec
assertEquals(Container.MKV, after.container)
assertEquals("the video codec is not the container's to change", before.videoCodec, after.videoCodec)
assertEquals("the audio codec is not the container's to change", before.audioCodec, after.audioCodec)
}
@Test
fun `changing the video codec leaves the container and the audio codec alone`() {
// The transposition this guards against is real: setVideoCodec and setAudioCodec take
// different enum types, but a copy(...) naming the wrong field compiles wherever the types
// happen to line up, and the picker would silently set the other one.
val before = viewModel.settings.value.spec
viewModel.setVideoCodec(VideoCodec.VP9)
val after = viewModel.settings.value.spec
assertEquals(VideoCodec.VP9, after.videoCodec)
assertEquals(before.container, after.container)
assertEquals(before.audioCodec, after.audioCodec)
}
@Test
fun `changing the audio codec leaves the container and the video codec alone`() {
val before = viewModel.settings.value.spec
viewModel.setAudioCodec(AudioCodec.OPUS)
val after = viewModel.settings.value.spec
assertEquals(AudioCodec.OPUS, after.audioCodec)
assertEquals(before.container, after.container)
assertEquals(before.videoCodec, after.videoCodec)
}
@Test
fun `applying a suggestion replaces the spec without disturbing quality or engine`() {
// A suggestion comes from ContainerCapabilities when the current spec is invalid, so it is
// a whole spec by construction. What it must not do is reset the two settings beside it.
viewModel.setQuality(QualityTier.BEST)
viewModel.setEnginePreference(EnginePreference.FORCE_SOFTWARE)
viewModel.applySuggestion(OutputFormat.MKV_H264.spec)
val settings = viewModel.settings.value
assertEquals(OutputFormat.MKV_H264.spec, settings.spec)
assertEquals(QualityTier.BEST, settings.quality)
assertEquals(EnginePreference.FORCE_SOFTWARE, settings.enginePreference)
}
@Test
fun `changing the quality leaves the spec and the engine preference alone`() {
// Both neighbours are moved off their defaults first. Asserting against AUTO -- which is
// what `ConversionSettings` starts with -- let a `setQuality` that also reset the engine
// preference to AUTO pass, because the reset and the survival looked identical.
viewModel.setPreset(OutputFormat.WEBM_VP9)
viewModel.setEnginePreference(EnginePreference.FORCE_SOFTWARE)
val before = viewModel.settings.value.spec
viewModel.setQuality(QualityTier.BEST)
val settings = viewModel.settings.value
assertEquals(QualityTier.BEST, settings.quality)
assertEquals(before, settings.spec)
assertEquals(
"quality is not the engine preference's to change",
EnginePreference.FORCE_SOFTWARE,
settings.enginePreference,
)
}
@Test
fun `changing the engine preference leaves the spec and the quality alone`() {
// Off the defaults for the same reason as the test above: QualityTier.FAST is the starting
// value, so asserting it here would have been satisfied by a reset as readily as by a
// survival.
viewModel.setPreset(OutputFormat.WEBM_VP9)
viewModel.setQuality(QualityTier.BEST)
val before = viewModel.settings.value.spec
viewModel.setEnginePreference(EnginePreference.FORCE_SOFTWARE)
val settings = viewModel.settings.value
assertEquals(EnginePreference.FORCE_SOFTWARE, settings.enginePreference)
assertEquals(before, settings.spec)
assertEquals(
"the engine preference is not the quality's to change",
QualityTier.BEST,
settings.quality,
)
}
@Test
fun `editing past every preset leaves no matching preset`() {
// `matchingPreset` is what the settings sheet reads to decide whether to show a preset as
// selected or to say "Custom". Editing one field of a preset must drop it out of the list
// rather than leaving the old one highlighted.
viewModel.setPreset(OutputFormat.MP4_H265)
assertEquals(OutputFormat.MP4_H265, viewModel.settings.value.matchingPreset)
viewModel.setAudioCodec(AudioCodec.FLAC)
assertNull(
"an edited spec is no longer any preset, and the sheet says Custom",
viewModel.settings.value.matchingPreset,
)
assertNotEquals(OutputFormat.MP4_H265.spec, viewModel.settings.value.spec)
}
@Test
fun `cancelling with no active job does nothing rather than throwing`() {
// `activeWorkId?.let(...)` -- the null side. A user can reach Cancel through a state that
// has already finished, and taking the app down for it would be worse than doing nothing.
viewModel.cancel()
assertEquals(ConversionState.Idle, viewModel.state.value)
}
}
@@ -0,0 +1,200 @@
package org.libremediaconverter.join
import android.net.Uri
import androidx.media3.common.util.UnstableApi
import androidx.work.Data
import androidx.work.WorkInfo
import androidx.work.workDataOf
import org.junit.Assert.assertEquals
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.convert.InputFile
import org.libremediaconverter.model.ConcatStrategy
import org.libremediaconverter.work.ConcatWorker
import org.robolectric.RobolectricTestRunner
/**
* Every answer [joinStateFrom] can give, chosen rather than stumbled into.
*
* The join-side twin of `ConversionStateMappingTest`, and the argument is the same one: the mapping
* ran on every test that drove a real `ConcatWorker`, but a real worker only ever reaches a terminal
* state with well-formed output, so five arms had never been *chosen* by anything.
*
* ## The one that is not just coverage
*
* `an unknown strategy name is read as a re-encode rather than thrown` covers a real defect this
* seam exposed. The line it replaces was:
*
* ```kotlin
* .getString(ConcatWorker.KEY_STRATEGY)?.let(ConcatStrategy::valueOf) ?: ConcatStrategy.REENCODE
* ```
*
* `valueOf` throws on a name this build does not define, and this runs inside a `viewModelScope`
* collect with no handler — so it does not become a `Failed` state, it takes the process down.
* `ConcatWorker.kt` had already made this exact change for `KEY_FORMAT` and written down why; the
* matching read on this side had not been changed with it.
*/
@UnstableApi
@RunWith(RobolectricTestRunner::class)
class JoinStateMappingTest {
@Test
fun `a running join is joining`() {
assertEquals(JoinState.Joining(INPUTS), map(WorkInfo.State.RUNNING))
}
@Test
fun `a blocked join looks like one that is starting`() {
// Folded into the RUNNING arm deliberately: a job waiting on a prerequisite is nothing the
// user can act on, and a separate word for it would be noise.
assertEquals(JoinState.Joining(INPUTS), map(WorkInfo.State.BLOCKED))
}
@Test
fun `an enqueued join that has already run is waiting to retry`() {
assertEquals(JoinState.Waiting(INPUTS), map(WorkInfo.State.ENQUEUED, runAttemptCount = 1))
}
@Test
fun `an enqueued join that has never run is simply starting`() {
// The other side. Without it, a mapping that ignored runAttemptCount passes the test above.
assertEquals(JoinState.Joining(INPUTS), map(WorkInfo.State.ENQUEUED, runAttemptCount = 0))
}
@Test
fun `a success that named no file is a failure, not an empty success`() {
assertEquals(
JoinState.Failed(JOINED_WITHOUT_A_FILE_MESSAGE),
map(WorkInfo.State.SUCCEEDED, data = Data.EMPTY),
)
}
@Test
fun `a success carries the strategy the worker actually used`() {
// Not cosmetic: the join screen tells the user whether their files were stream-copied or
// re-encoded, which is the difference between lossless and lossy.
val joined = map(
WorkInfo.State.SUCCEEDED,
data = workDataOf(
ConcatWorker.KEY_OUTPUT_PATH to "/cache/conversions/joined.mp4",
ConcatWorker.KEY_STRATEGY to ConcatStrategy.STREAM_COPY.name,
),
) as JoinState.Joined
assertEquals(ConcatStrategy.STREAM_COPY, joined.strategy)
}
@Test
fun `an unknown strategy name is read as a re-encode rather than thrown`() {
// The defect. A build that added a third strategy leaves finished joins in the queue naming
// it, and WorkManager keeps those about a week -- the premise WorkerEnumFallbackTest and
// JobTags are both written on. With `valueOf` this throws IllegalArgumentException inside a
// viewModelScope collect that has no handler, so it is not a Failed state, it is a crash.
//
// REENCODE rather than STREAM_COPY because it is the conservative answer: describing an
// unknown join as lossless would be a claim the app cannot support.
val joined = map(
WorkInfo.State.SUCCEEDED,
data = workDataOf(
ConcatWorker.KEY_OUTPUT_PATH to "/cache/conversions/joined.mp4",
ConcatWorker.KEY_STRATEGY to "SMART_CONCAT_V2",
),
) as JoinState.Joined
assertEquals(ConcatStrategy.REENCODE, joined.strategy)
}
@Test
fun `a success with no strategy at all falls back the same way`() {
val joined = map(
WorkInfo.State.SUCCEEDED,
data = workDataOf(ConcatWorker.KEY_OUTPUT_PATH to "/cache/conversions/joined.mp4"),
) as JoinState.Joined
assertEquals(ConcatStrategy.REENCODE, joined.strategy)
}
@Test
fun `a success from older work falls back to the format such a job really used`() {
val joined = map(
WorkInfo.State.SUCCEEDED,
data = workDataOf(ConcatWorker.KEY_OUTPUT_PATH to "/cache/conversions/joined.mp4"),
) as JoinState.Joined
assertEquals(ConcatWorker.outputNameFor(ConcatWorker.DEFAULT_FORMAT), joined.suggestedName)
assertEquals(ConcatWorker.DEFAULT_FORMAT.mimeType, joined.mimeType)
}
@Test
fun `a blank name or type falls back the same way a missing one does`() {
val joined = map(
WorkInfo.State.SUCCEEDED,
data = workDataOf(
ConcatWorker.KEY_OUTPUT_PATH to "/cache/conversions/joined.mp4",
ConcatWorker.KEY_SUGGESTED_NAME to "",
ConcatWorker.KEY_MIME_TYPE to " ",
),
) as JoinState.Joined
assertEquals(ConcatWorker.outputNameFor(ConcatWorker.DEFAULT_FORMAT), joined.suggestedName)
assertEquals(ConcatWorker.DEFAULT_FORMAT.mimeType, joined.mimeType)
}
@Test
fun `a failure carries the reason the worker gave`() {
assertEquals(
JoinState.Failed("Not enough free space to join these files."),
map(
WorkInfo.State.FAILED,
data = workDataOf(
ConcatWorker.KEY_ERROR to "Not enough free space to join these files.",
),
),
)
}
@Test
fun `a failure with nothing said still says something`() {
assertEquals(
JoinState.Failed(ConcatWorker.GENERIC_FAILURE_MESSAGE),
map(WorkInfo.State.FAILED, data = Data.EMPTY),
)
}
@Test
fun `a failure whose message is blank falls back like a missing one`() {
assertEquals(
JoinState.Failed(ConcatWorker.GENERIC_FAILURE_MESSAGE),
map(WorkInfo.State.FAILED, data = workDataOf(ConcatWorker.KEY_ERROR to " ")),
)
}
@Test
fun `a cancellation lands wherever the caller said it should`() {
// A join started here goes back to Ready with the picked files; one picked up by reattach
// goes to Idle, because those URIs belong to a process that no longer exists.
assertEquals(
JoinState.Ready(INPUTS),
map(WorkInfo.State.CANCELLED, cancelled = JoinState.Ready(INPUTS)),
)
assertEquals(JoinState.Idle, map(WorkInfo.State.CANCELLED, cancelled = JoinState.Idle))
}
private fun map(
state: WorkInfo.State,
runAttemptCount: Int = 0,
data: Data = Data.EMPTY,
cancelled: JoinState = JoinState.Ready(INPUTS),
): JoinState = joinStateFrom(
JoinUpdate(state = state, runAttemptCount = runAttemptCount, outputData = data),
inputs = INPUTS,
cancelled = cancelled,
)
private companion object {
val INPUTS = listOf(
InputFile(Uri.parse("content://test/a.mp4"), "a.mp4", 1024L),
InputFile(Uri.parse("content://test/b.mp4"), "b.mp4", 2048L),
)
}
}
@@ -451,6 +451,99 @@ class ContainerCapabilitiesTest {
}
}
/**
* The video twin of `no audio track is accepted by every container in both modes`.
*
* Dead in production today, and deliberately so: every caller guards `NONE` before asking the
* matrix, so nothing reaches this arm through the app. **The asymmetry is the argument, not the
* reachability** -- its audio counterpart at the top of the same `when` has had a dedicated
* test since #136, and one of a matched pair being covered is how a later reader concludes the
* other was considered and exempted. It was not; it was simply missed.
*
* Not the same shape as the two `COPY -> error(...)` arms, which `docs/coverage-read-findings.md`
* records as a named exemption (F4). Those are guards that must not be provokable. This is a
* documented answer -- "no video track fits anywhere" -- and an answer is a thing to pin.
*/
@Test
fun `no video track is accepted by every container in both modes`() {
Container.entries.forEach { container ->
listOf(CodecMode.COPY, CodecMode.ENCODE).forEach { mode ->
assertTrue(
"$container should accept no video track ($mode)",
ContainerCapabilities.accepts(container, VideoCodec.NONE, mode),
)
}
}
}
/**
* A suggestion that keeps the codec the user asked for, rather than falling back to the
* container's first encodable one.
*
* `repairVideo`'s third arm -- "the request is not a copy, and this container can encode it" --
* is the one that preserves intent, and it was the only arm of the four nothing reached. The
* property test above executes `repairVideo` on every case it walks and lands elsewhere each
* time: an explicit COPY that works, a source the container can carry untouched, or no video
* track at all.
*
* The route is indirect because it is the only one the app has. VP9 into WebM is a perfectly
* good video request; what makes it invalid is the *audio* -- WebM carries Opus and Vorbis, not
* AAC. So `validateAudio` refuses, `suggestions` looks for a container that can hold what was
* asked for, and MP4 can encode VP9. The suggestion has to come back carrying VP9: swapping to
* the container's first encodable codec would discard the choice the user made.
*/
@Test
fun `a repaired suggestion keeps the video codec the user chose`() {
val invalid = ContainerCapabilities.validate(
OutputSpec(Container.WEBM, VideoCodec.VP9, AudioCodec.AAC),
h264Source,
)
assertTrue("WebM cannot hold AAC, so this spec is invalid", invalid is Validation.Invalid)
val suggestions = (invalid as Validation.Invalid).suggestions
assertTrue(
"expected a suggestion that still encodes VP9, got $suggestions",
suggestions.any { it.videoCodec == VideoCodec.VP9 },
)
assertEverySuggestionValid(invalid, h264Source)
}
/**
* The fallback in `firstContainerHolding`: when the input's own container cannot hold the
* codec the user asked for, any container that can will do.
*
* The preferred half -- "the container the input already uses" -- is what every other case
* reaches, because they all start from a file whose own container carries the codec in
* question. The elvis after it had never run.
*
* AVI is the input that makes it run: AVI predates H.265 and has no mapping for it, so asking
* an AVI for H.265 is refused, and the container the input already uses cannot be part of the
* answer. Without the fallback the only candidates left are AVI itself and the container
* holding the *source* codec -- also AVI -- so the refusal still offers something, but what it
* offers is H.264: the app quietly declines the codec the user asked for instead of moving them
* to a container that supports it.
*
* That is why this asserts the codec survives rather than that the list is non-empty. A
* non-empty assertion passes with the fallback deleted -- measured, not assumed.
*/
@Test
fun `an input whose container cannot hold the requested codec is moved, not downgraded`() {
val aviSource = InputProbe(videoCodec = "h264", audioCodec = "aac", container = Container.AVI)
val invalid = ContainerCapabilities.validate(
OutputSpec(Container.AVI, VideoCodec.H265, AudioCodec.AAC),
aviSource,
)
assertTrue("AVI has no mapping for H.265", invalid is Validation.Invalid)
val suggestions = (invalid as Validation.Invalid).suggestions
assertTrue(
"expected a container that can actually hold H.265, got $suggestions",
suggestions.any { it.videoCodec == VideoCodec.H265 },
)
assertEverySuggestionValid(invalid, aviSource)
}
@Test
fun `resolving audio COPY before asking the matrix is required`() {
// The audio twin of `resolving COPY before asking the matrix is required`, and the reason is
@@ -0,0 +1,88 @@
package org.libremediaconverter.work
import android.content.pm.ServiceInfo
import org.junit.Assert.assertEquals
import org.junit.Test
import org.junit.runner.RunWith
import org.robolectric.RobolectricTestRunner
import org.robolectric.annotation.Config
/**
* [ConversionForegroundType.current] answers differently on each of the three API regimes, and
* until this file only one of them was ever executed.
*
* `app/src/test/resources/robolectric.properties` pins the whole JVM suite to `sdk=36`, so every
* Robolectric test that reaches a `ForegroundInfo` takes the `mediaProcessing` arm and no other.
* The 33 and 34 arms were cold: 3 lines and 3 of 4 branches, measured on `main` at `d354f64`.
*
* **The instrumented test is not a substitute, and the reason is specific.**
* `ConversionWorkerTest.foregroundTypeMatchesTheRunningApiLevel` asserts against whichever API the
* leg happens to be — one arm per leg, never the other two — and the legs that would cover 33 and
* 34 are the ones issue #122 wedges. `docs/coverage-read-findings.md` records an API 33 run that
* reported `received: 60` and `failed: unknown`: the regime *was* exercised, and that leg could
* not have said so if it had broken. Four `@Config` classes here pin all three arms
* deterministically, in the same `./gradlew` invocation as everything else.
*
* `minSdk` is 33, so none of these is dead code — each is a device someone is running the app on.
*
* **SDK 35 is in the list for the boundary, not for the answer.** It shares its answer with 36,
* which would make it look redundant. It is not: relaxing `>= VANILLA_ICE_CREAM` to `>` is invisible
* at every level except exactly 35, so without this class that mutation survives the suite.
*/
@RunWith(RobolectricTestRunner::class)
@Config(sdk = [33])
class ForegroundTypeApi33Test {
/**
* Zero rather than a named constant because there is no constant to name: API 33 does not
* require a type, and `mediaProcessing` does not exist here to pass. `ForegroundInfo` reads 0
* as "no type at all", which is what this regime wants.
*/
@Test
fun `api 33 asks for no foreground service type`() {
assertEquals(0, ConversionForegroundType.current())
}
}
/**
* API 34 makes a type mandatory and still has no `mediaProcessing`, so `dataSync` is the only
* sensible fit. See [ForegroundTypeApi33Test] for why this file exists.
*/
@RunWith(RobolectricTestRunner::class)
@Config(sdk = [34])
class ForegroundTypeApi34Test {
@Test
fun `api 34 falls back to dataSync, the only type that fits`() {
assertEquals(ServiceInfo.FOREGROUND_SERVICE_TYPE_DATA_SYNC, ConversionForegroundType.current())
}
}
/**
* The first level with `mediaProcessing`, and therefore the one that tells `>=` from `>`.
* See [ForegroundTypeApi33Test].
*/
@RunWith(RobolectricTestRunner::class)
@Config(sdk = [35])
class ForegroundTypeApi35Test {
@Test
fun `api 35 is the first level that takes mediaProcessing`() {
assertEquals(ServiceInfo.FOREGROUND_SERVICE_TYPE_MEDIA_PROCESSING, ConversionForegroundType.current())
}
}
/**
* The level the rest of the suite runs at, asserted here rather than assumed — it is the one arm
* that was already covered, and leaving it out would make this file look like it is about the old
* levels rather than about all three regimes. See [ForegroundTypeApi33Test].
*/
@RunWith(RobolectricTestRunner::class)
@Config(sdk = [36])
class ForegroundTypeApi36Test {
@Test
fun `api 36 keeps mediaProcessing`() {
assertEquals(ServiceInfo.FOREGROUND_SERVICE_TYPE_MEDIA_PROCESSING, ConversionForegroundType.current())
}
}
@@ -16,6 +16,7 @@ import androidx.work.testing.WorkManagerTestInitHelper
import androidx.work.workDataOf
import kotlinx.coroutines.runBlocking
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Test
@@ -125,6 +126,39 @@ class JobSnapshotsTest {
assertEquals(newer.absolutePath, Reattachment.choose(snapshots)?.job?.outputPath)
}
/**
* A job in the tag query that never recorded an output path at all.
*
* Distinct from the three cases above, which all *have* a path and differ in what it names. A
* job still running, or one that finished without writing its result key, carries no path at
* all -- and `getWorkInfosByTagFlow` returns it alongside the finished ones, because the tag is
* the worker class and every attempt ever enqueued carries it.
*
* The guard is the `?.` in `path?.let(::File)`. Without it the null goes straight into a `File`
* constructor. What this pins is the consequence rather than the null check: such a job must
* not be offered as a result, so `Reattachment.choose` has to walk past it to the job that
* really produced a file. Choosing it would put a Converted screen in front of the user with a
* Save button that has nothing to save.
*/
@Test
fun `a job that recorded no output path is not offered as a result`() {
val real = stagedFile("real.mp4", bytes = 4096)
finishedWithOutput(real)
finishedWithNoOutput()
val snapshots = snapshots()
assertEquals("both jobs carry the tag, so both come back", 2, snapshots.size)
val silent = snapshots.single { it.outputPath == null }
assertFalse("no path means no output, not an empty one", silent.outputExists)
assertEquals("and no time either, for the same reason", 0L, silent.outputModifiedAt)
assertEquals(
"the reattachment has to walk past it to the job that really produced a file",
real.absolutePath,
Reattachment.choose(snapshots)?.job?.outputPath,
)
}
private fun snapshots(): List<JobSnapshot> = runBlocking {
workManager.jobSnapshots(
tag = ConversionWorker::class.java.name,
@@ -152,6 +186,11 @@ class JobSnapshotsTest {
).result.get()
}
/** A job that carries the tag and no result key -- still running, or finished without one. */
private fun finishedWithNoOutput() {
workManager.enqueue(OneTimeWorkRequestBuilder<ConversionWorker>().build()).result.get()
}
private companion object {
/** Two fixed moments a day apart, so the ordering is stated rather than raced for. */
const val OLDER_MS = 1_700_000_000_000L