Compare commits

...
Author SHA1 Message Date
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
8 changed files with 183 additions and 16 deletions
+15 -7
View File
@@ -130,8 +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** — **84.9% of lines (1971/2321), 63.8% of branches**,
measured 2026-08-26 with `./gradlew :app:jacocoTestReport`, against 454 JVM tests in 67 classes.
- **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.
**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
@@ -147,12 +148,19 @@ install for code that can never run — and on API 37 the full APK does not fit
disproportionately Robolectric, so each one added denominator and no numerator — the measurement
was punishing exactly the tests that were hardest to write.
Two things still hold. A floor needs a baseline that has settled, and this one has not: it moved
39 points in a single build change on 2026-08-24, then another 16 as the #52 test push and the
Two things still hold. A floor needs a baseline that has settled, and this one has not. It moved
39 points in a single build change on 2026-08-24; then another 16 as the #52 test push and the
fixes it turned up landed — 69.2% -> 84.9% line, 53.2% -> 63.8% branch — while the denominator
grew 2194 -> 2321, because that work added production code of its own. And **re-measure before
quoting**: this entry was written quoting 81.4%, measured four hours earlier, and was already
three points stale by the time it was ready to merge.
grew 2194 -> 2321, because that work added production code of its own; then again on 2026-08-27
as #132 and #133's ten children landed — 84.9% -> 87.1% line, 63.8% -> **69.1%** branch, 454 ->
502 tests. **Branch moved four times as far as line that last time**, and that is the shape to
expect from this kind of work rather than a surprise: 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; branch coverage is the
whole point.
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
tests before it counts as done. If it is e2e testable, it gets e2e tests. Both clauses apply —
a change that is both needs both.
@@ -505,7 +505,7 @@ class ConversionViewModel @JvmOverloads constructor(
WorkInfo.State.FAILED -> ConversionState.Failed(
info.outputData.getString(ConversionWorker.KEY_ERROR)
?.takeIf { it.isNotBlank() }
?: "Conversion failed.",
?: ConversionWorker.GENERIC_FAILURE_MESSAGE,
)
WorkInfo.State.CANCELLED -> cancelled
@@ -565,7 +565,7 @@ class ConversionViewModel @JvmOverloads constructor(
// than a fresh handle for the second failure's sake: a retry that fails again
// lands back here still carrying the file, not on a bare Failed that would take
// the offer away.
_state.value = ConversionState.Failed(e.message ?: "Could not save the file.", pending)
_state.value = ConversionState.Failed(e.message ?: SAVE_FAILED_MESSAGE, pending)
}
}
}
@@ -24,6 +24,20 @@ const val STAGED_FILE_GONE_MESSAGE: String =
"The finished file is no longer in the cache, so there is nothing left to save. " +
"Start over to make it again."
/**
* What to tell the user when the copy into their chosen destination did not finish.
*
* A fallback, not the usual message: `publish` throws with a real reason most of the time — the
* volume filled, the provider revoked the grant — and that reason is better than this. This is for
* the exception that arrives with nothing to say, which would otherwise reach the screen as an
* empty failure.
*
* Kept next to [STAGED_FILE_GONE_MESSAGE] for exactly the reason that one names: **both ViewModels
* need it**, and saving is what it is about. It was written out twice before — `ConversionViewModel`
* and `JoinViewModel` each carried their own copy of the literal, agreeing by coincidence.
*/
const val SAVE_FAILED_MESSAGE: String = "Could not save the file."
/**
* A staged file that is still there to be saved, and everything the save dialog needs to offer it.
*
@@ -19,6 +19,7 @@ import org.libremediaconverter.convert.ConversionDependencies
import org.libremediaconverter.convert.InputFile
import org.libremediaconverter.convert.InputQuery
import org.libremediaconverter.convert.PendingSave
import org.libremediaconverter.convert.SAVE_FAILED_MESSAGE
import org.libremediaconverter.convert.STAGED_FILE_GONE_MESSAGE
import org.libremediaconverter.convert.ScreenOwnership
import org.libremediaconverter.model.ConcatStrategy
@@ -194,7 +195,7 @@ class JoinViewModel @JvmOverloads constructor(
fun onInputsPicked(uris: List<Uri>) {
val token = ownership.claim()
if (uris.size < 2) {
_state.value = JoinState.Failed("Pick at least two files to join.")
_state.value = JoinState.Failed(ConcatWorker.TOO_FEW_INPUTS_MESSAGE)
return
}
viewModelScope.launch {
@@ -295,7 +296,7 @@ class JoinViewModel @JvmOverloads constructor(
WorkInfo.State.FAILED -> JoinState.Failed(
info.outputData.getString(ConcatWorker.KEY_ERROR)
?.takeIf { it.isNotBlank() }
?: "Joining failed.",
?: ConcatWorker.GENERIC_FAILURE_MESSAGE,
)
WorkInfo.State.CANCELLED -> cancelled
@@ -346,7 +347,7 @@ class JoinViewModel @JvmOverloads constructor(
// than leaving "Start over" -- which deletes it -- as the only thing on offer.
// Passing `pending` rather than rebuilding it is what keeps a retry that fails
// again on a carrying Failed instead of a bare one.
_state.value = JoinState.Failed(e.message ?: "Could not save the file.", pending)
_state.value = JoinState.Failed(e.message ?: SAVE_FAILED_MESSAGE, pending)
}
}
}
@@ -39,7 +39,7 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker
val uris = inputData.getStringArray(KEY_INPUT_URIS)?.map(Uri::parse)
?: return Result.failure(workDataOf(KEY_ERROR to "No input files."))
if (uris.size < 2) {
return Result.failure(workDataOf(KEY_ERROR to "Pick at least two files to join."))
return Result.failure(workDataOf(KEY_ERROR to TOO_FEW_INPUTS_MESSAGE))
}
// Absent, not zero, when the picker could not size every input -- see the same read in
// ConversionWorker and InputQuery for why the two are no longer one number.
@@ -107,7 +107,7 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker
}
FailureOutcome.FAIL -> {
Log.e(TAG, "Joining failed.", e)
Result.failure(workDataOf(KEY_ERROR to (e.message ?: "Joining failed.")))
Result.failure(workDataOf(KEY_ERROR to (e.message ?: GENERIC_FAILURE_MESSAGE)))
}
}
}
@@ -137,6 +137,34 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker
)
companion object {
/**
* What the user is told when a join arrives with fewer than two inputs.
*
* Shared with `JoinViewModel`, which refuses the same condition one layer up so the picker
* can answer without enqueueing anything. Two copies of this sentence existed before, and
* only the one here was pinned by a test (#139) — so the wording could drift on the screen
* without a single test noticing, for one message the user sees from one condition.
*
* Here rather than in the ViewModel because the rule is the worker's: `request(...)` takes
* a `List<Uri>` and checks nothing about its length, so this is the guard that always runs.
*/
const val TOO_FEW_INPUTS_MESSAGE: String = "Pick at least two files to join."
/**
* The last resort when a join fails and the exception says nothing.
*
* Shared with `JoinViewModel`, whose `FAILED` arm falls back to the same sentence when the
* output `Data` carries no error at all — a worker killed before it could write one. The two
* are a chain rather than a coincidence: this is what the worker puts *in* `KEY_ERROR`, and
* that is what the ViewModel says when `KEY_ERROR` never arrived. The user cannot tell the
* two apart and should not have to, so they are one sentence.
*
* The `Log.e` above deliberately keeps its own literal. A log line has a different audience
* and carries the exception with it; coupling it to the user-facing wording would mean
* rewording the screen to change a log.
*/
const val GENERIC_FAILURE_MESSAGE: String = "Joining failed."
const val KEY_INPUT_URIS = "input_uris"
const val KEY_TOTAL_BYTES = "total_bytes"
const val KEY_FORMAT = "format"
@@ -313,7 +313,7 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo
}
FailureOutcome.FAIL -> {
Log.e(TAG, "Conversion failed.", cause)
Result.failure(workDataOf(KEY_ERROR to (cause.message ?: "Conversion failed.")))
Result.failure(workDataOf(KEY_ERROR to (cause.message ?: GENERIC_FAILURE_MESSAGE)))
}
}
@@ -352,6 +352,20 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo
)
companion object {
/**
* The last resort when a conversion fails and the exception says nothing.
*
* Shared with `ConversionViewModel`, whose `FAILED` arm falls back to the same sentence when
* the output `Data` carries no error — a worker killed before it could write one, which a
* refused foreground start after a process restart produces. The two are a chain rather than
* a coincidence: this is what goes *into* `KEY_ERROR`, and that is what is said when
* `KEY_ERROR` never arrived. The user cannot tell those apart and should not have to.
*
* See [ConcatWorker.GENERIC_FAILURE_MESSAGE] for the join-side twin, and the note there
* about why the neighbouring `Log.e` keeps its own literal.
*/
const val GENERIC_FAILURE_MESSAGE: String = "Conversion failed."
const val KEY_INPUT_URI = "input_uri"
const val KEY_DISPLAY_NAME = "display_name"
const val KEY_SIZE_BYTES = "size_bytes"
@@ -0,0 +1,102 @@
package org.libremediaconverter.join
import android.app.Application
import android.net.Uri
import androidx.media3.common.util.UnstableApi
import androidx.work.ListenableWorker
import androidx.work.testing.TestListenableWorkerBuilder
import androidx.work.workDataOf
import kotlinx.coroutines.runBlocking
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.convert.ConversionDependencies
import org.libremediaconverter.convert.RecordingPublisher
import org.libremediaconverter.convert.installTestWorkManager
import org.libremediaconverter.work.ConcatWorker
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment
/**
* The two layers that refuse a short join, refusing it with one sentence.
*
* ## Why this is not "assert a constant equals itself"
*
* `ConcatWorker` and `JoinViewModel` both reject a join of fewer than two files, and before #158
* each carried **its own copy of the literal**. Only the worker's was pinned — by `RefusedJobTest`,
* added in #139 — so the wording on the screen could drift away from the wording in the job with no
* test saying anything, for one message the user sees from one condition.
*
* Sharing a constant makes them agree by construction. What it does *not* do is prove that both
* layers still reach it: a refactor that stops `JoinViewModel` refusing at all, or that gives it a
* different message, passes any test that only reads `TOO_FEW_INPUTS_MESSAGE`. So each layer is
* driven for real here — the ViewModel through `onInputsPicked`, the worker through `doWork` — and
* the assertion is that the two answers are **the same string**, taken from two running layers
* rather than from one declaration.
*
* That is the shape `CLAUDE.md` asks for: revert the sharing and this goes red, because the two
* sites drift the moment they are allowed to.
*
* ## Scope
*
* The arity guard's own behaviour on the ViewModel side — that it refuses one file, that it accepts
* two, that it claims ownership first — is #155's, and this deliberately does not duplicate it.
* This file is about the *agreement between layers*, which is what #158 changed.
*/
@UnstableApi
@RunWith(RobolectricTestRunner::class)
class SharedFailureMessagesTest {
private lateinit var app: Application
private lateinit var viewModel: JoinViewModel
@Before
fun setUp() {
app = RuntimeEnvironment.getApplication()
ConversionDependencies.publisher = { RecordingPublisher(app) }
installTestWorkManager(app, workDataOf(ConcatWorker.KEY_OUTPUT_PATH to "/dev/null"))
viewModel = JoinViewModel(app)
}
@After
fun tearDown() {
ConversionDependencies.reset()
}
@Test
fun `both layers refuse a one-file join with the same sentence`() {
// The ViewModel, refusing before anything is enqueued.
viewModel.onInputsPicked(listOf(ONE_FILE))
val fromScreen = (viewModel.state.value as JoinState.Failed).message
// The worker, refusing a job that reached the queue anyway -- which it can, because
// ConcatWorker.request(...) takes a List<Uri> and checks nothing about its length.
val result = runBlocking { worker(ONE_FILE).doWork() }
val fromJob = (result as ListenableWorker.Result.Failure)
.outputData.getString(ConcatWorker.KEY_ERROR)
assertEquals(
"the screen and the job must say the same thing about the same refusal",
fromScreen,
fromJob,
)
// And that the shared sentence is the one either layer would have written on its own,
// rather than both having drifted together to something else.
assertEquals(ConcatWorker.TOO_FEW_INPUTS_MESSAGE, fromScreen)
}
private fun worker(vararg inputs: Uri): ConcatWorker = TestListenableWorkerBuilder<ConcatWorker>(
context = app,
inputData = workDataOf(
ConcatWorker.KEY_INPUT_URIS to inputs.map(Uri::toString).toTypedArray(),
ConcatWorker.KEY_TOTAL_BYTES to 1024L,
),
runAttemptCount = 0,
).build()
private companion object {
val ONE_FILE: Uri = Uri.parse("content://test/holiday.mp4")
}
}
@@ -146,7 +146,7 @@ class RefusedJobTest {
assertEquals(
ListenableWorker.Result.failure(
workDataOf(ConcatWorker.KEY_ERROR to "Pick at least two files to join."),
workDataOf(ConcatWorker.KEY_ERROR to ConcatWorker.TOO_FEW_INPUTS_MESSAGE),
),
result,
)