948d53b67e7e0db8039debc966a81e6566d5d887
72
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  JMR-devandClaude Opus 5
|
e7caeeac43 |
Finish the startup sweep before onCreate returns, in the JVM suite
Robolectric builds an Application per test class that asks for one, and each onCreate launched a staging sweep on Dispatchers.IO over the shared <cacheDir>/conversions/. Nothing joined them, so a test asserting about a staged file was racing every sweep the classes before it had left in flight (#159). It was CI-only until wave 4 added ten Robolectric classes, at which point OutputPublisherStagingTest started failing locally too. LibreMediaConverterApp gains a protected open sweepScope and publishes the Job onCreate started; the JVM suite substitutes TestLibreMediaConverterApp, whose scope is Dispatchers.Unconfined so the sweep -- a plain function that never suspends -- runs to completion inline. The SupervisorJob is kept so this differs from production in the dispatcher alone. Suite-wide rather than per-test: 27 of the 58 Robolectric classes touch that directory, so opt-in was not a real option. It costs one assertion, knowingly. AppStartSweepTest opened by asserting that the manifest's android:name is what Robolectric instantiated. An application= override replaces the manifest rather than being checked against it, and applicationInfo.className reports the override too, so that claim is now unobservable from this source set and a rewritten version would assert the override against itself. The manifest link is device-only; the cast in setUp still catches the test app ceasing to extend the real one. AppStartSweepTest also joins the published Job instead of polling for ten seconds -- a poll cannot tell "swept" from "not started yet" -- and gains a test pinning that the sweep is complete when onCreate returns, which is the property the substitution exists for and the only place it is checked. Verified by mutation rather than by repetition. Putting the test app back on Dispatchers.IO reddens that test 5 times out of 5, while running the whole suite six times per arm caught nothing either way: at the rate #159 was observed at, a clean six-run arm is roughly a coin flip, so the comparison was underpowered and is not offered as evidence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
ec2cae256f |
Give both engines one rc-to-outcome function, and unify the failure message (#203)
FFmpegEngine and ConcatEngine each carried their own copy of the same `when`, and the copies had drifted: one preferred the fail stack trace and fell back to the log tail, the other only ever read the log tail. Neither was tested -- both live inside a callback handed to FFmpegKit, which does not run on the JVM -- so nothing could see that the two disagreed about what a failed session says. sessionOutcome() now holds the rule and each engine maps Success/Cancelled/Failed onto its continuation. Verified JVM-safe rather than assumed: javap over the committed AAR shows ReturnCode(int) as a plain public constructor with pure static isSuccess/isCancel and a <clinit> that loads no native library. Per #203's decision this unifies on the stack trace, so a join failure now carries the diagnostics a conversion failure always did. The PREFIX stays per-engine: unifying the strategy must not unify the sentence, since a join reporting "FFmpeg failed" would be a worse message than the one it replaces. There is a test for exactly that. The two message sources are lambdas rather than values, and that is load-bearing. getFailStackTrace and getAllLogsAsString are calls onto a native session, and only the failure arm needs either; taking them by value would put both on the happy path of every successful conversion, which the shape this replaces did not -- it read them inside the else branch. Same reasoning as capabilitiesFrom taking a Sequence in #194: a seam should not change what runs when. There is a test that counts the reads, and the eager mutation reddens it. A null return code is a real input rather than a defensive one -- getReturnCode() is nullable and a session killed before reporting has none -- so it fails, with "null" where the number would be. Nothing asserted the old join text: `grep -rn 'Joining failed|FFmpeg failed' app/src/` returns only main, plus ConcatWorker.GENERIC_FAILURE_MESSAGE, which is a different constant this does not touch. Re-run immediately before committing, as the ticket asked. Mutations, all run and restored: swap the ifBlank operands 1 red treat cancellation as a failure 2 red read both message sources eagerly 1 red hardcode the prefix 1 red Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
4e88de3045 |
Stop a permission answer starting a second conversion (#202)
currentInput() answered for Converting, Waiting and Converted as well as Ready. Those three arms were unreachable by tapping Convert -- the button renders only in the Ready branch -- but they were reachable through the POST_NOTIFICATIONS *result*, which ConverterScreen.kt:91 wires to convert() rather than to the button. Reaching one enqueued a SECOND job over a live one: activeWorkId was overwritten, and the first job kept running with its foreground notification orphaned and nothing left holding its id to cancel it. #202 decided to narrow rather than to test it as it stood, because a test written against the old shape would have frozen the double-enqueue as intended behaviour -- the F1/F5 failure mode docs/coverage-read-findings.md names. currentInput() is now (_state.value as? ConversionState.Ready)?.input, which is what JoinViewModel.join() has been all along; the two screens are the same shape and only one of them was over-general. Four cold refusal arms come with it, all reached the same way -- a system callback arriving after the screen has moved on, which is what a result redelivered after process death does: ConversionViewModel.kt:513 currentInput() ?: return ConversionViewModel.kt:600 pendingSave() ?: return JoinViewModel.kt:316 (as? Ready)?.inputs ?: return JoinViewModel.kt:390 pendingSave() ?: return One fixture note worth keeping: the second case needs a real staged file in the worker's output Data. A SUCCEEDED job with no output path maps to Failed rather than Converted, so Data.EMPTY never reaches the state the case is about -- which cost a timed-out awaitState before it was spotted. Mutations, all run and restored: restore the over-general four-arm when 1 red <- the defect this change fixes currentInput()!! at :513 2 red pendingSave()!! in save() 1 red drop both join guards 1 red Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
2125763ebf |
Read FFprobe's answer without spawning FFprobe (#195)
readMediaInformation was 114 missed instructions and 24 missed branches -- the second
largest block on the wave-4 report -- and exactly one line of it needed a device:
FFprobeKit.getMediaInformation(path).getMediaInformation()
Everything after it reads an ordinary object, so it moves into ffprobeInfoFrom and the edge
keeps the call and the null check. Verified JVM-safe rather than assumed: javap over the
committed AAR's runtime jar shows MediaInformation(JSONObject, List<StreamInformation>,
List<Chapter>) and StreamInformation(JSONObject) as plain public constructors whose <clinit>
does not load the native library, so a test builds its own without libffmpegkit present.
The decision worth reaching is containerFrom's SECOND argument. FFprobe reports
"matroska,webm" for both MKV and WebM because they share a demuxer, so the video codec is
the only thing separating them. containerFrom has thirty-three covered branches and not one
can notice that argument being dropped -- the mistake is at the call, not in the callee, so
every existing containerFrom test stays green while every VP9 WebM quietly becomes an MKV.
Two things the tests found rather than confirmed.
The format properties are NESTED under "format": getFormat() resolves through
getStringFormatProperty, not off the top-level object. The first fixture put the keys at the
top level and four cases failed with a null container. The helper says so now.
And one mutation SURVIVED on the first pass -- reading dimensions with
streams.firstNotNullOfOrNull { it.getWidth() } instead of video?.getWidth(). The fixture put
the dimensions on the chosen video stream, which is also the first stream carrying any, so
the two readings agreed and the test could not tell them apart. Separating them needs a
chosen video stream with NO dimensions and a later one that has them, which is a real shape:
FFprobe omits width/height for a stream it could not measure. That case is now its own test
and the mutation reddens it.
Mutations, each run and restored:
drop the video codec argument to containerFrom 1 red
take the LAST video stream instead of the first 1 red
read dimensions from any stream, not the chosen one 0 red -> 1 red after the new case
let an unparseable duration throw instead of zero 1 red
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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>
|
||
|
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>
|
||
|
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>
|
||
|
JMR-dev
|
79cca0eb47 | Merge branch 'test/refused-jobs' into test/mediaprobe-track-seam | ||
|
JMR-devandClaude Opus 5
|
ad47ce6c96 |
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> |
||
|
JMR-devandClaude Opus 5
|
b2790e13d9 |
S1 (#141): cut the track walk into a pure seam, and test the matrix
#84 closed by classifying probeWithExtractor and probeForConcat as device-bound and explicitly not a gap. That was right about FFprobe and right about the measurement boundary, and wrong that these are only orchestration. The track walk is a branch matrix, and androidTest reaches it only through whatever the committed fixtures happen to contain -- so none of its rules is *chosen* by any test there. #133 offered two ways to reach it: drive ShadowMediaExtractor, or cut the loop into a pure function. Taking the second, which is the pattern CLAUDE.md names and work/FailureOutcome.kt documents. extractedFrom and concatInputFrom take List<MediaFormat>; what is left needing a device -- setDataSource, getTrackFormat, release -- is one three-line extension function, which is the thin edge androidTest should be covering. The two are deliberately not merged despite the overlap. One reads duration and not frame rate; the other reads frame rate and not duration. A merged version would compute both for every caller, and ConcatPlanner treats an unknown frame rate as "cannot prove a match" -- so a field the join flow does not need must not start arriving as a number. Eleven tests over cases no fixture provides: two video tracks, two audio tracks, audio outlasting video, a track with no KEY_DURATION, audio declared before video, a subtitle track, and no tracks at all. Six mutations, six red: last video track wins first-video test last audio track wins first-audio test duration = last rather than max longest-track test drop the containsKey guard six tests (getLong throws on a missing key) guess a frame rate of 30 no-frame-rate test join takes the last video track join frame-rate test MediaProbe's missed branches drop 91 -> 70; what is left is the FFprobe half and the two catch arms, which are native and device-bound exactly as #84 said. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
b93ef79931 | Merge remote-tracking branch 'origin/main' into m-117-tmp | ||
|
JMR-dev
|
febd141bea | Merge remote-tracking branch 'origin/main' into merge-124-tmp | ||
|
JMR-devandClaude Opus 5
|
c4bb7d4d2d |
Quote the rate the ticket settled on, and point the save gap at its ticket
Two accuracy fixes to notes the earlier commits left behind. The test KDocs carried "roughly 1-in-130" and a 400-leg-attempt denominator. Both come from earlier comments on #49 that its own census later replaced -- that ticket has three recorded corrections to its rate claims, and a superseded figure in a permanent comment is the exact thing its author kept having to fix. What survives the corrections is the count and the spread: four occurrences, API 33, 35 and 36, every one on attempt 1 and green on re-run. The save exemption described a real defect with nowhere to look it up. It is #123 now, so the KDoc names a number instead of trailing off. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
3599307040 |
Say what the save exemption does not cover, rather than implying it is total
The note claimed `save` is left unguarded because nothing can overwrite what it writes. That half is true -- the only observation that could belongs to a job already in a terminal state. The other half was missing: a save whose copy is still in flight when the user taps Start over lands `Saved` on a screen they have just cleared. Guarding it would drop that write instead, which reports nothing for a file that may genuinely have reached the destination. That is a question about what the screen should offer during a save, and answering it in a race fix would be deciding it by accident. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
3f731d8ea7 | Merge remote-tracking branch 'origin/main' into merge-117-tmp | ||
|
JMR-devandClaude Opus 5
|
cc424dd08f |
Let the user's pick keep the screen a reattachment was about to take
`reattach()` read `_state.value`, found it `Idle`, and then handed the job to `observe()` -- which launches a *separate* coroutine that cannot write until its `collect` has resumed with a `WorkInfo`. So the check happened at one moment and the write landed at another, with a whole pick able to fit in between: the user tapped, their metadata query suspended, the guard saw an empty screen, and the finished job from an earlier session wrote over `Ready(picked)` a moment later. The comment above that guard said "no suspension point between this check and the assignment below, so nothing can interleave". There is no assignment below, and the two lines are in different coroutines. That sentence is why this sat as flaky CI for two days rather than being read as the product race it is. `ScreenOwnership` makes the answer the test already encodes -- the user's pick wins -- true rather than probable. A claim is taken synchronously when the user acts; every write that lands after a suspension point checks the claim it was made under and drops itself if that claim has been superseded. Dropped, not reordered: a write that is dropped cannot come back later. Cancelling the superseded observer was never enough on its own. `Job.cancel` is honoured at the next suspension point, and a collector that has already resumed and is on its way to `_state.value = ...` has none left; the write lands anyway. It also cannot help at all in the case reported, where nothing supersedes the observation until after it has been launched. `JoinViewModel` had the identical shape and nothing watching it, so it gets the same fix and the counterpart test that was missing. Its pick dispatcher becomes injectable for the same reason `ConversionViewModel`'s already was: without that seam there is no way to ask what happens while a pick is still in flight. Closes #49 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
dce516224c |
Offer a fix that works when the file has no video to copy
Refusing "copy the video" for a file that has none built its one suggestion by hand — drop the video track and leave everything else alone. That is valid only when the audio axis already happened to be fine. For any audio the target cannot carry (Vorbis or PCM into MP4, MP3 into WebM) the offer is refused in the next breath, so the Advanced picker showed a one-tap fix leading straight to a second error. Nothing unsafe shipped — ConversionWorker re-validates — but it is a dead end, and it contradicted the promise Validation.Invalid makes in its own KDoc. Route it through the shared repair-and-filter path instead, as every other branch does. Excluding what the *user* asked for rather than the already-repaired spec is what keeps the case that worked working: an MP3 into MP4 still gets its copy offered, because the repair of a copyable track is that same copy. Only a branch that builds its own list can break that promise at all, since suggestions() ends by filtering on validate().isValid. The property test now covers both of them — this one and the image output — rather than reaching them by luck, which is how a dead-end chip survived two earlier widenings of it. Its failures name the probe too: three rows share a spec and differ only in the input. Closes #114 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
2b7520061b | Merge remote-tracking branch 'origin/main' into merge-119-tmp | ||
|
JMR-devandClaude Opus 5
|
4e46eb99f6 |
Say what the theme's dynamicColor parameter does, and test the branches that run
The KDoc claimed dynamic colour "stays switchable so users can opt back to the brand palette". Nothing switches it: MainActivity is the only caller and passes no arguments, so dynamicColor is always true and the two brand-palette branches are dead. A reader who trusted that sentence would go looking for a setting that has never existed. Replace the claim with what is true today and point at #68, which holds the decision -- add a switch, delete the dead branches along with the template palette, or replace that palette first. None of the three is taken here. ThemeKt had no test, so nothing would have caught the branches being swapped either. Assert what the theme resolves by reading MaterialTheme.colorScheme inside the content lambda: the two live branches on background luminance, which is the one thing two schemes off the same device palette do not share, and the dead pair by passing dynamicColor explicitly. Both KDocs say plainly that the test is the only thing that passes it, so the coverage is not misread as evidence a switch exists -- which is the misreading #68 exists to prevent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
83b557409e | Merge remote-tracking branch 'origin/main' into merge-116-tmp | ||
|
JMR-devandClaude Opus 5
|
c887af0d83 |
Offer the file again after a failed save, rather than only offering to delete it
save()'s onFailure keeps the staged file on purpose -- it can be the only copy of an hour of transcoding, and the destination did not receive it -- and then handed the screen a Failed carrying a message and nothing else. That branch rendered exactly one control: "Start over", wired to reset(), which discards precisely the file the comment above it goes out of its way to keep. The intent was already written down in main; the UI did not honour it, and the only rescue was process death followed by reattach -- unadvertised, and bounded by a sweep that collects anything a day old. Failed now carries a PendingSave, and only where the failure came from save(). A transcode that died staged nothing and must not sprout a save button, so the handle is nullable and the observe() arm leaves it null; so does a save that found the file already gone. The branch renders "Try saving again" above "Start over", opening the same CreateDocument flow with the same name and type the first attempt used. A retry that fails again lands back on a carrying Failed rather than a bare one, so the second failure cannot eat what the first kept. Start over still deletes from there, and that is a decision rather than an inheritance: deletion is the user's choice only once the alternative has been offered. pendingStaged remains the single owner of the delete, so the carried handle is a view of it rather than a second owner and no path out of the state can drop a file the old shape could not. pendingSave() exists so save() and each screen's CreateDocument registration answer "what would a save target" once instead of twice -- the entry points cast to Converted/Joined, which answered null for a Failed and fell back to the current pickers, wrong for any spec edited since the job ran and for every reattached job. Both tabs, since JoinViewModel and JoinScreen have the same shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
238142d9cc |
Guard the whole Media3 export instead of only its two ends
transcode() posts its work to a HandlerThread, and everything on that thread has no caller to throw back to: an escaping exception reaches the thread's uncaught handler and takes the process down, while the continuation is never resumed. Both halves of that are bad, and the second is arguably worse — a worker left suspended forever holds a foreground service. The guarding was two narrow runCatching blocks, one around buildTransformer and one around transformer.start, with the two Media3 builders sitting unguarded between them. That gap was not theoretical. EditedMediaItem.Builder rejects a composition with both tracks removed, which is exactly what a plan of (Drop, Drop) asks for, and it does so with a plain IllegalStateException from the constructor. Validation now refuses the spec that produces such a plan, so neither the picker nor ConversionWorker will start one. Routing is a separate question and still answers Media3 for it — a dropped track makes nothing un-hardware-able — so a request that skips validation still arrives here: a job queued before the settings changed, or one made through ConversionWorker.request directly. CopyPlanner's own KDoc already names that path as the reason it re-checks what validation has checked; this is the same belt for the same braces. One guard around the whole body costs nothing on success and turns any such refusal into a failed job with a reason attached. The export body moves into startExport, whose contract is the thing that makes one guard enough: returning normally means the export is running and the listener owns the continuation, throwing means it never started and the caller does. Cancellation is still registered before start. Covered twice on purpose. Robolectric runs the real HandlerThread and the real Media3 builders, so the JVM test exercises the whole sequence and can be run anywhere; the instrumented one repeats it against the real framework. Neither asserts only that the failure is an IllegalStateException, because withTimeout raises TimeoutCancellationException and java.util.concurrent.CancellationException extends IllegalStateException — so that assertion alone calls an unresumed continuation a pass. Both were written that way first, and reverting the guard is what exposed it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
9c809d4e16 |
Refuse a spec that would leave the output with no tracks at all
Validation already refused two ways of asking for an empty file: None on both codec axes, and Copy for a video track the input does not have. It missed the third, because it read only the spec. Name H.265 with the audio off, hand it an MP3, and the spec looks fine — it names a video codec — while CopyPlanner drops that track anyway, because the *input* has no video to encode. The plan is (Drop, Drop), the router still says Media3, and EditedMediaItem.Builder refuses to build a composition with both tracks removed. It refuses it on Transformer's own HandlerThread, where the user sees the app die rather than a reason. Asking the probe as well as the spec catches all three faces with one guard, and the equivalence is exact rather than approximate: CopyPlanner drops video for None or for an input with none, and audio for None, so "(Drop, Drop)" and this condition are the same set. A sweep over every non-image container by codec by codec against both probes asserts that, so a new container or codec cannot reopen the gap on an axis nobody wrote a case for. This newly refuses a combination the Advanced picker accepts today, and that is the point: today it crashes. What it must not do is refuse without a way out. The Copy face had one only nominally — its single hand-built suggestion was None + None, which validation rejects in the next breath, so the one-tap fix fixed nothing. All three faces now go through the shared repair-and-filter path, which for an MP3 into MP4 offers "copy the audio across" and nothing that has to be re-refused. Repair is also stopped from naming a video codec for a file with no video track. It used to fall through to the first codec the container could encode, so the fix offered for an MP3 was "H.264" — a codec CopyPlanner then drops, making the offer a fiction that happened to validate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
a0b6a3dde8 | Merge branch 'main' into fix/probe-dispatcher-seam | ||
|
JMR-dev
|
bda5abea6c | Merge branch 'main' into test/media3engine-mime-tables | ||
|
JMR-dev
|
21eeb6f3f8 | Merge branch 'main' into test/mediaprobe-pure-helpers | ||
|
JMR-devandClaude Opus 5
|
dbba213c51 |
Give the pick a dispatcher, so an escaped error fails the test that caused it
`onInputPicked` hops to a hard-coded `Dispatchers.IO` inside a `launch` with no exception handler -- deliberate, because a real OutOfMemoryError should reach the thread's default handler and take the process down. On the JVM there is no such handler: kotlinx-coroutines-test installs a process-wide collector, once per classloader and never removed, which keeps the error and rethrows it at whichever `runTest` starts next. Every Compose rule is a `runTest`, so the OOM raised by `ConversionViewModelProbeFailureTest` failed some *other* Compose class, and which one moved between runs of identical, green code. Naming the dispatcher gives the throw somewhere to land. With the pick inline inside a `runTest`, the collector's callback belongs to the test that caused the error, so it is handed over and consumed rather than stored for a stranger. Both hops of a pick rather than only the probe, which is where this differs from the seam issue #66 sketched: leaving the metadata query on a real IO thread makes the coroutine resume on a main looper Robolectric leaves paused, and that bounce is exactly the asynchrony that made delivery unpredictable. That buys the assertion the test could not make before -- the real OutOfMemoryError instance, not an inference from a card that never filled in, which is also what a probe returning null looks like. Reverting the hop to `Dispatchers.IO` turns it red: "expected java.lang.OutOfMemoryError to be thrown, but nothing was thrown". Refs #66 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
7f951baf8f |
Make the two codec tables answer for each other, and stop describeAudio printing a NUL
The FFprobe codec vocabulary is written out in at least four places and none of them had a test. Two had already drifted apart. `x264`, `hev1`, `x265` and `vp09` resolved in `CodecNames.videoFromName` and returned null from `AndroidDeviceCodecs.mimeForCodecName`, so the app identified the codec for the source card and for routing and then ran the device capability check blind on the same string; `mpeg4` ran the other way and rendered as a raw name. Nothing could notice, and the reason is structural: a `when` cannot be enumerated, so no test can ask one table what the other one knows. Both are maps now, for that reason alone, and `CodecVocabularyTest` walks the two key sets. A name added to -- or removed from -- one side alone fails the build. The one legitimate asymmetry is listed rather than implied: `mpeg4` is decodable input with no `VideoCodec` to name it, so `CodecNames` is right not to carry it. That list is itself checked, because otherwise it is an escape hatch -- any future divergence could be waved through by adding the name to it, and adding `x265` to it now fails. THIS CHANGES BEHAVIOUR for `x264`, `hev1`, `x265` and `vp09`. A null from `mimeForCodecName` means "unknown to us: assume the platform can handle it and let a failed export trigger the FFmpeg fallback", which is the right policy for a name nobody recognises and the wrong one for a name recognised one file over. A device without the matching decoder now sends those four to FFmpeg up front instead of spending a doomed hardware attempt to discover it. No input loses hardware it could have used: each alias resolves to the MIME its canonical spelling already resolved to, so a device that has the decoder still answers true. `ConversionRouterTest` still passes and that is not evidence either way -- every `canDecode` in it is a hand-written stub that never reaches this table. #74 is the same family one level down. `describeVideo` answered "Unrecognised" for `InputProbe.UNPARSEABLE` and `describeAudio` had no such arm, so an unparseable audio codec would have fallen through to `?: name` -- and the sentinel opens with a NUL, so the source-info card would have rendered a `Text` beginning with U+0000. The two now share one body, which is what stops the next arm being added to one side only. Two corrections to that ticket, taken from the file rather than from the ticket, since it warns about exactly this: - It quotes `audioFromName` as opening with `null, InputProbe.UNPARSEABLE -> null`. It did not; it opened with `null -> null` and the sentinel reached `else`. Naming the sentinel in the shared lookup therefore changes no answer and is documentation, not the fix. - It says `describeVideo`'s arm has no test of its own. It did -- `descriptions stay readable for unknown and missing codecs` asserts it -- so deleting the shared arm now reddens three tests across both sides, not one. Mutations run, each on the full 386-test suite: add "avc3" to CodecNames only -> CodecVocabularyTest red on two counts, CodecNamesTest green: 8 tests, 0 failures, which is the ticket's point about per-table arm tests delete the UNPARSEABLE arm -> CodecNamesTest red on three, one of them quoting the NUL back add "x265" to DECODE_ONLY_NAMES -> CodecVocabularyTest red on the escape hatch delete "vp09" from the MIME map -> CodecVocabularyTest red on three, which is the state this commit is fixing Audio is not cross-checked, and that is a gap rather than a decision: the device capability check is video-only, so this module has no second audio table to compare `AUDIO_ALIASES` against. `Media3Engine.audioMimeTypeFor` is the other half and belongs to #85. `MediaProbe.shortName` (#84) is the fourth table and is untouched here for the same reason. Closes #87. Closes #74. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
5ec2bba64b |
Check the MIME types Media3Engine hands Transformer, and the claim above them
Both tables decide what codec ends up in the user's file, and neither was exercised. Point H265 at VIDEO_H264 and every hardware HEVC export writes H.264 into a file the user asked to be H.265: Transformer does as told, the export succeeds, and the only symptom is a codec nobody chose. One arm carried an assertion rather than a value -- "Never reached: only an Encode plan consults this, and COPY/NONE are not Encode" -- which is a claim about callers parked in a branch of a callee. It is true, and nothing checked it, so it would have gone on reading as true after it stopped being. Proved instead: CopyPlanner answers both codecs before the Encode branch and its fallback draws from ContainerCapabilities.encodableVideo, which contains neither, so a sweep over every spec the planner can be handed asserts no Encode plan carries COPY or NONE. Counters guard the sweep, because `as? Encode ?: let` asserts nothing at all for a Drop or Copy plan. The audio sibling claim did not survive intact. "MP3 and FLAC have no Android encoder; the router routes them to FFmpeg" is true and incomplete: one rule, `audioEncode !in MEDIA3_AUDIO`, diverts Vorbis by identical logic, so three of the six encodable codecs never reach the table. VORBIS -> AUDIO_VORBIS is a correct mapping for a request Transformer is never given. The arm stays -- a right answer in unreachable code costs nothing -- and the comment now says so. The tables are asked of the router's decisions rather than of its codec sets, because the comments claim behaviour and a set can be right while the rule reading it is wrong. Both move to an internal companion object so a JVM test can reach them without constructing an engine, which would start a real HandlerThread to answer an enum lookup; #57's precedent, and the JVM test source set is a friend of main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
fd2bb1d889 |
Test the three MediaProbe helpers nothing else would catch
MediaProbe's MIME table, its image-demuxer rule and its Int reader are pure
functions with no test at all, and each fails silently rather than loudly.
shortName falls through to substringAfter('/') and reports a plausible-looking
string that CodecNames may or may not still recognise, so a dropped arm turns a
stream-copyable file into a re-encode. isImageFormat is checked before anything
else in classify, so a wrong answer overrides both probes. intOr's runCatching
is the only thing standing between a Float frame rate and losing every other
track property the loop had read.
Widen the three to internal, as #57 did, and say in each KDoc why the shape is
what it is -- the _pipe suffix is not a substring test because yuv4mpegpipe is
raw video, and getInteger casts rather than coerces.
Every format name asserted came from ffprobe rather than from memory: a picked
.png reports png_pipe, a .jpg reports jpeg_pipe, a .y4m reports yuv4mpegpipe.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
2f3f461cc1 |
Say what each conversion state puts on screen, and what it withholds
The screen's state machine had a seam and no matrix behind it. Every arm of the `when` returns `Unit`, so an arm can render anything at all and still compile -- a button offered where it cannot work, a state's own data never reaching the node meant to show it, an affordance wired to the wrong callback. The leaf tests cannot see any of that: they compose `FileCard`, `AdvancedPicker` and the three pickers directly and never hold a `ConversionState`. The arm worth guarding most is `Ready`'s `enabled = validation.isValid`. The Advanced picker deliberately lets an impossible container / codec pair be selected, so that one expression is all that stands between an invalid spec and a job that cannot succeed. `enabled = true` compiles, renders an identical screen apart from one colour, and passed the whole suite before this. Callbacks are asserted over the complete log rather than one at a time, so a case reads "this one fired and nothing else". A bare "the callback ran" check stays green on an arm that fires the right callback for the wrong reason. The routing chip needed a tag to be locatable at all: its text comes from the finished job, so a text matcher would have to name a routing explanation the screen does not own. That is the only production change here. Not asserted, deliberately: `Failed`'s error colour, which Compose publishes nowhere in the semantics tree; and the three absent `FileCard`s, which are compile-guarded -- `Idle`, `Saved` and `Failed` carry no `input` -- so those lines state the intent without being what enforces it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
46ad95350b |
Give both screens somewhere for a state to come from
`ConverterScreen` and `JoinScreen` each inlined their whole `when (state)` inside the public entry point, and state arrived only as `viewModel.state`. That left four of the twelve state branches across the two screens with no test that could ever reach them: driving a real ViewModel needs a WorkManager and a media probe in the constructor, and even then `Waiting` follows a denied foreground start and `Converted`/`Joined` follow a worker run that has already succeeded. So the `when` moves into `ConverterScreenContent` and `JoinScreenContent`, which take the state, the settings, the validation and an actions holder. The entry points keep the three launchers and `collectAsStateWithLifecycle` and nothing else. The callbacks travel in `ConverterActions` / `JoinActions` rather than as loose parameters because detekt's `LongParameterList` sits at its default threshold of six and `config/detekt/detekt.yml` does not relax it for `@Composable` -- `AdvancedPicker` already sits exactly on it. Twelve flat parameters would turn a clean detekt run red; data classes are exempt from the rule. Nothing else changed. The body was cut and pasted rather than retyped, so the U+2026, U+2014 and U+00B7 characters the leaf tests match on are the same bytes, and `is Idle -> Unit` in the nested `when` -- permanently unreachable, and deliberately kept -- survives the move. The diff stops above `FormatPicker` in one file and above `FileRow` in the other, which is why the leaf suites #57-#60 landed pass unedited: every one of them composes a leaf directly and none references either entry point. The two new tests are the bite, one per screen and one per direction of the seam: a `Converted` / `Joined` state renders Save, and tapping Save hands back the name the finished job chose. The state matrix itself is #62 and #63. |
||
|
JMR-devandClaude Opus 5
|
df2e42a2b3 |
Name the screen leaves, and give the tests a tag vocabulary
Kotlin `private` on a top-level declaration is file-scoped, so every leaf composable in the two screens was invisible even to the JVM test source set, which is a friend of main. The only three declarations src/test could name were ConverterScreen, JoinScreen and AppRoot -- there was nothing to write a test against, which is why #52 could not be started as filed. `internal` is the same choice MainActivity already documents for Destination: the unit tests can name it, and it stays invisible to anything outside the module. Eleven declarations in ConverterScreen and FileRow in JoinScreen. The tag table is the other half. Tests reference a symbol rather than a literal, which is what keeps the five children that follow independent: "Cancel", "Start over" and "Save file" are each rendered by both screens and by more than one state branch, so rewording one would otherwise redden several PRs at once and no diff would explain why. There were zero testTag, semantics or contentDescription calls anywhere in main before this. Every tag is applied inside main. A tag a test hands down as a Modifier proves only that the test set it -- it would survive the affordance losing its own tag entirely, which is the vacuous shape CLAUDE.md records nine of in one review. That is why FileRow and DetailRow derive theirs from data they already hold rather than taking an index from the call site. TestTags is public rather than internal, and R38.8 is the reason. It reads the table from androidTest, and whether that is a friend source set of main under AGP 9 had no in-tree answer -- nothing referenced a main internal from there. Settled by compiling one: it is a friend, so internal would work today. Public anyway, because that friendship is AGP wiring rather than something this project states, and the KDoc now carries the measurement so nobody has to repeat it. Smoke tests cover each leaf: it renders, and its tag resolves to exactly one node. Counting rather than asserting existence is deliberate, since a duplicated tag fails differently depending on which finder a later test happens to use. The state-branch tags -- Convert, Cancel, Save file, Start over, the progress bars -- have no bite yet: reaching a branch needs the state seam R38.5 extracts, and the state matrix is R38.6/R38.7 by design. Five mutations, all red on the named test alone: FORMAT_CHIPS deleted, FILE_CARD_NAME deleted, detailRow's tag no longer derived from its label, FileRow's no longer derived from its name, and JOIN_MORE given JOIN's value -- the last caught only by the uniqueness check, which is what it is for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
3534c6d996 |
Clean up the empty document a refused open leaves, and stop the space sums wrapping
publish() opened the destination stream outside its guarded region, justified by "nothing has been written at that point, so there is nothing of ours to remove". That reasoning is wrong about what exists: SAF's CreateDocument contract creates the document before publish() is ever called -- which is why every fixture in OutputPublisherPublishTest starts as an existing empty file. A provider that then hands out no stream, because it dropped between the picker and the write or simply returns null, left a zero-byte file at the name the user chose while the screen said the save had failed. The open moves inside the try, so the same two bounds that already govern a failed copy govern this: only a document URI, and only a destination positively known to be empty. The dead-provider case is untouched and now demonstrably by the guard rather than by the placement -- nothing answers for that authority, so no size can be read, and "I could not tell" still refuses to authorise a delete. Its test comment said the old thing and now says that one. The space arithmetic overflows in two places, both live on main and independent of the allocatable-versus-usable question that stays parked: - hasSpaceFor computed `free > required + headroom`. A request within 128 MiB of Long.MAX_VALUE wraps that sum negative, and every free-space measurement beats a negative number, so the check answers "plenty of room" to the largest request it can be handed. Rewritten as `free - headroom > required` with both operands clamped at zero, which is the form the parked branch's StagingSpace.hasRoomFor already argues for. - InputQuery.total folded a join's inputs with nothing stopping the sum from wrapping, and that is the reachable half: no single file overflows, three four-exabyte inputs do. It saturates at Long.MAX_VALUE now, which the check above then refuses. SpaceArithmeticTest ties the two together in the shape the defect had -- the total that came out negative is handed straight to the space check -- and keeps one allowed case so the refusals cannot pass by refusing everything. The negative-size clamp is deliberately left unasserted, with a comment saying why: it only changes the answer when free space is below the headroom, which a test reading the host's real cache volume cannot arrange. R6 / #15, R23 / #32 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
a6cf4f4ff4 |
Stop three enum reads escaping doWork, and pin what the attempt bound buys
Both workers read enums out of their input Data with Enum.valueOf, and all three reads sit
ABOVE the try. A name this build does not define -- which is what a downgrade or a
rollback with work still in the queue produces, since WorkManager keeps work for about a
week -- threw IllegalArgumentException straight out of doWork(). That is D13's signature
verbatim: FAILURE with reschedule = false, output Data with zero entries so the screen
says "Conversion failed." and nothing else, and no staged.delete(), so the partial stays
in cache. readSpec() twelve lines below already handles exactly this case, and its KDoc
says why.
So all three take readSpec's shape: entries.firstOrNull { it.name == name } ?: default.
Consistency argues for it as much as correctness does -- the file already contains the
right answer to this question, three times.
WorkerEnumFallbackTest reaches each read. Two of them pin the value that replaces the
unknown name rather than only that nothing threw: a quality tier falling back to something
arbitrary would convert at a setting nobody chose, and a join's format decides the
extension its output is staged with, which is where FFmpeg infers the container from. The
engine-preference case asserts through the space check instead, because predicting which
engine AUTO picks would tie the test to a routing rule it is not about. Against the
unfixed code all three fail with "No enum constant ...".
MAX_FOREGROUND_START_ATTEMPTS had no test of its value. Both existing cases are written
against the symbol, which pins the relationship and leaves the number free: changed to 2,
the job gives up about ninety seconds after process death -- exactly the long conversion
the retry exists to protect -- and all 257 tests stayed green.
The new assertion is the property the KDoc argues, not the literal: summed against
WorkRequest's own DEFAULT_BACKOFF_DELAY_MILLIS and MAX_BACKOFF_MILLIS, the attempts have
to span at least eight hours, which is what makes "the user will have opened the app by
then" a claim rather than a hope. A deliberate re-tune that keeps the property passes; the
accident does not, and reports the span it got (0.025 hours at 2).
R22 / #31, R10 / #19
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
c5c4c5323b |
Test the edge that feeds reattachment, and stop it reporting ENOENT
Reattachment.choose has twenty tests and every mutation aimed at it bites. Everything that computes its inputs had none, and five mutations there passed the whole 257-test suite. Four are closed here, each verified by applying the mutation and watching the new test go red. jobSnapshots() is the half that has to touch WorkManager and the filesystem, so it is where the untested values live. JobSnapshotsTest drives it against a real WorkManager and a real cacheDir: - A zero-byte staged file is not an output. Relaxing the filter to `exists()` -- which is what a job killed before its engine wrote anything leaves behind -- made the snapshot claim a result, and the user would meet a Save button for a zero-byte "conversion". The same case pins that the path is still reported and that the mtime stays 0 for a file that is not a result. - Each result carries its own file's mtime. Hardcoding it to zero starves the newest-file tie-break of the only data it has, which is precisely the failure the tie-break exists to prevent: the query has no ORDER BY, so an arbitrary winner keeps winning every launch. Timestamps are set with setLastModified and compared against what the filesystem stored, because mtime granularity is not this test's claim to make. ReattachGuardsTest covers the two decisions the ViewModel makes that the pure rule cannot: - A file picked while the query was still in flight is not reattached over. Deleting the guard turns the user's pick into yesterday's job -- with the Save button pointing at a file the card does not name. Made deterministic by holding WorkManager's task executor rather than by racing two IO hops: the query cannot finish until the pick has landed. The test also asserts the brake really gripped, so a reattachment that never arrived cannot pass for one that was refused. - An Ambiguous result is offered without being attributed. Two finished jobs naming one staged file is what the device produced before staging was keyed on the job id; taking the first job's tags labels the file with the other conversion's name, which is the confident lie the KDoc rejects. The neutral label and the absent size are both pinned. ReattachmentTest's FAILED exclusion was only ever tested with pathless FAILED jobs, so a narrow regression ranking a FAILED job that carries a file like a result passed all 257 tests. The live shape is the 2 MB orphan the device pass found: a job killed mid-write leaves a partial, and under that regression the user is offered a truncated file with a Save button. One fixture with outputPath and outputExists set closes it. save() re-checks the staged file, in both ViewModels. The check reattachment made ran inside a tag query that can be hours older than the tap, and cacheDir is what the OS empties when it wants space and what the sweep collects after a day. The file's absence used to arrive as staged.inputStream() throwing, and e.message put "/data/user/0/.../4b4882....mp4: open failed: ENOENT" on screen -- a true statement about a path the user has never seen and cannot act on. It now reads as a sentence with an action in it. The message is one constant next to OutputPublisher because both ViewModels need it and staging is what it is about. Reattachment's KDoc claimed a defect that was fixed in the commit before it -- that "Start over" keeps its staged file -- which would send a maintainer to re-fix D2. Rewritten to say what is actually true: the delete happens, and the gap it leaves is the reset() whose delete is cancelled with the Activity, which is the sweep's job and is named in the sweep's own KDoc. R1 / #10, R2 / #11, R24 / #33, R25 / #34 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
c2e6344aad |
Let WorkManager keep sole ownership of the progress notification
`publishProgress` posted with `NotificationManager.notify(NOTIFICATION_ID, …)` directly, on the
very id WorkManager owns through `setForeground`, using a notification built `setOngoing(true)`.
Two posters for one id is a race about which of them wrote last, and on a Pixel 10 Pro XL it was
lost on attempt 3 of 12 while cancelling a `BEST`-tier job:
attempt 3: terminal state = CANCELLED
attempt 3: +300ms active=0 id1001=false ongoing=null <- WorkManager tore it down
attempt 3: +700ms active=1 id1001=true ongoing=true <- a progress tick put it back
attempt 3: +5000ms active=1 id1001=true ongoing=true
Still there ten minutes later, with no app process left in the world to withdraw it. The orphan's
record carried `flags=ONGOING_EVENT|ONLY_ALERT_ONCE` and **no `FOREGROUND_SERVICE`**, which is
what identifies the poster rather than merely suggesting one: WorkManager's own post goes out
with that flag and this one did not.
Whether the user could then swipe it away is deliberately not claimed here. An earlier reading
said "cannot dismiss", on the strength of `isClearable()` returning false — but that is false
purely because `FLAG_ONGOING_EVENT` is set, the record carries neither `FOREGROUND_SERVICE` nor
`NO_CLEAR`, and API 34+ lets an ongoing notification with no foreground service behind it be
swiped. The SystemUI test was impossible behind a secure lock screen and was never done. The
resurrection is what is reproduced, and it is what this fixes.
Progress now goes through `setForegroundAsync` — the non-suspending half of the same call
`doWork` already makes, which is the supported way to update a running worker's foreground
notification. That is not merely a different mechanism for the same post: it hands the id back to
its owner, so the notification WorkManager withdraws when the job ends is the same one the last
progress update wrote, and there is nothing left holding a second reference to it.
Two guards, and they are independent on purpose:
- **`isStopped` short-circuits the whole function.** A tick arriving after the stop has nobody
left to report to, so a stopped worker publishes nothing at all — the `setProgressAsync`
included, which WorkManager refuses for finished work anyway.
- **`setForegroundAsync` refuses on its own** for work whose state is already terminal. That
closes the window between the `isStopped` check and the update reaching the task thread,
which a check alone can only narrow.
The `~1/sec` throttle is unchanged, in effect and in reason: FFmpeg's statistics callback and
Media3's progress polling both fire several times a second, and pushing every one of them janks
the system UI. Routing them through WorkManager does not make them cheap, so the interval stays
exactly where it was. It is reordered into an early return rather than a nested `if`, which is
the only difference.
The initial `setForeground(...)` in `doWork` is untouched and stays a suspending call inside the
`try`. That placement is the whole of the previous commit on this file: a denied background
foreground-service start throws there, and `FailureOutcome` turns it into a retry rather than a
terminal failure. Converting it to `setForegroundAsync` for symmetry would have moved that
exception into an unobserved future and undone it silently.
The future `publishProgress` gets is not awaited — it is called from an engine callback, which is
not a coroutine, and a progress update is not worth blocking one for. Both of its failure modes
are benign, so a refusal is logged rather than propagated: dropping it entirely would make the
one that actually happens, a job finishing mid-update, invisible.
`ProgressNotificationTest` drives the real worker with a recording `ForegroundUpdater` — the same
seam `DeniedForegroundStartTest` uses — and asserts on both halves, because either alone passes
against something wrong. The positive half is that the update reached WorkManager on the id it
already holds, carrying `Notification.EXTRA_PROGRESS` of 42; a test that only checked nothing was
posted directly would pass just as well against a `publishProgress` that had been deleted. The
negative half is that Robolectric's notification manager holds nothing at all, asserted over the
whole manager rather than one id, so a renamed constant cannot make it pass by asking about a
notification nobody posts.
All three were red before the change: `one throttled progress update expected expected:<1> but
was:<0>` (nothing had reached WorkManager), `and must resurrect nothing expected:<0> but was:<1>`
(the device's resurrection, on the JVM), and `50 ticks inside one throttle window must not be 0
updates`. Each guard was then removed on its own to check the test that names it bites: without
`isStopped` the stopped worker publishes twice — `a stopped worker must publish nothing
expected:<1> but was:<2>` — and without the throttle fifty ticks become fifty updates.
`ConcatWorker` reports no progress at all and owns id 1002 by itself, so it has none of this and
none of it is added.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
b86df47c43 |
Tell an unknown input size apart from an empty file
`queryFile` started at `var size = 0L` and only moved off it when a provider answered the
`OpenableColumns.SIZE` column, which the platform documents providers *may* omit. So "this file
is empty" and "nobody would tell me how big it is" reached `OutputPublisher.hasSpaceFor` as the
same number, and `hasSpaceFor(0)` is not a space check -- it is "is there 128 MB free", which any
phone with a working camera passes.
The reachable value is not an inference. On a Pixel 10 Pro XL, `contentResolver.query` on a
`file://` URI returns null outright, so the cursor block never runs at all and the default
survives untouched: `queryFile gave displayName='input' sizeBytes=0`. Robolectric reproduces that
exactly -- null query, and a descriptor that reports 4321 bytes for the same file -- which is why
every test here is a JVM test rather than a device one.
`InputQuery` replaces the two copies of `queryFile`, which were byte for byte identical in
`ConversionViewModel` and `JoinViewModel`, so a fix to either would have been a fix to half the
app. It asks for the size twice: what the provider says, and then what the file itself says
through `openFileDescriptor(uri, "r").statSize`. The second needs no cooperation beyond the input
being openable, which a conversion is about to require anyway, and it is what answers the device
case above. Only when both decline is the answer null, and `InputFile.sizeBytes` is `Long?` so
that null cannot be spelled the same way as zero again.
**What an unknown size does was the decision, and it is deliberately not a refusal.**
`OutputPublisher.hasSpaceForUnknownSize()` produces the same number the defect produced by
accident -- with no size to reserve for, the headroom is all there is left to check -- and that is
worth saying plainly rather than dressing up. What changed is that it is now the answer to a
question that was asked. `hasSpaceFor` means "there is room for this many bytes" and nothing else
claims it.
Refusing was the obvious alternative and would have been worse than the bug: it turns "no
provider answered the SIZE column" into "this file cannot be converted", for a user who can do
nothing about either. A fixed floor was the other, and there is no honest number for it -- a
1 GB floor refuses a 10 MB conversion on a device with 500 MB free, which is the same failure in
a costume. `SpaceCheckTest` pins the choice from both sides: an unmeasurable input must not end
the job, and a full disk must still refuse it.
That second half is why the default answers *through* `hasSpaceFor`. `FakeFailures.FullDisk` in
the instrumented suite overrides `hasSpaceFor` and nothing else, so the delegation is the only
reason it still refuses an unknown-size job. Replacing the delegation with a bare `true` leaves
the full-disk test red with `expected:<Failure {error : Not enough free space to convert.}> but
was:<Failure {error : FFmpegKit failed to start on brand: robolectric...}>` -- the job sailed past
the guard and died at the engine instead.
Both workers get the same shape. The size arrives as input `Data`, which has no null, so the
absence of the key *is* the unknown -- `getLong(key, 0L)` was the other half of the conflation.
When it is absent the worker measures the input itself, which it can do because it holds the URI:
that covers a `request(...)` built by hand and work enqueued before the size became optional, and
it costs an ordinary job nothing because it runs only on the fallback. `ConversionWorker.request`
writes neither the `Data` entry nor the `JobTags.sizeBytes` tag for a size nobody knows, since a
tag reading `size-bytes:0` would come back through `Reattachment` as a confident claim that the
user's file is empty -- and `reattach()`'s `?: 0L` is gone for the same reason.
A join's total is `InputQuery.total`, which is null the moment a *single* input cannot be sized.
Summing the ones that answered was the competing reading and is rejected: a lower bound is
indistinguishable from a real total once it reaches the space check, so the guard would reserve
for half the job and pass. Reverting it to `sumOf { it ?: 0L }` records `[1111]` for a two-file
join whose second input nothing can measure.
Work already in the queue keeps the old conflation, and there is no fixing it. The previous
`request()` always wrote `putLong(KEY_SIZE_BYTES, sizeBytes)`, so a job enqueued before this
commit for a file nothing could size carries the key *present* and set to zero -- which reads
back as a declared size of zero and is trusted, exactly as before. Its `lmc.size-bytes:0` tag
reads back the same way, so `reattach()` shows such a card "0 B" rather than "Size unknown".
Nothing can separate that from a genuinely empty file after the fact, and a rule that treated a
declared zero as suspect would only rebuild the conflation facing the other way. WorkManager
keeps finished work for about a week, so this is a bounded window that clears itself; new work
never enters it.
`hasSpaceFor`'s KDoc claimed peak usage was "roughly input + output at once" while the arithmetic
reserved `input + 128 MB`. The arithmetic is what stays and the doc now says why: `bytes` is the
input's size standing in for the output's, generous for the ordinary conversion (which is asked
for precisely because it shrinks its input) and short for a re-encode to a bulkier codec; the
128 MB absorbs that error and the transient double copy while `publish` runs. Reserving
`input + output` outright would refuse jobs that fit. This is a pre-flight check that stops an
obviously impossible job from spending minutes finding out, not a guarantee -- a conversion that
runs out of space anyway still fails through its engine.
The measurement side of that line is untouched on purpose. `StorageManager.getAllocatableBytes`
is a separate entry with its own device evidence and its own `informational += "UsableSpace"` in
the lint block; this commit is about the number going *in*. `hasSpaceFor(bytes: Long)` keeps its
signature and stays open, so nothing overriding it had to change.
The file card says "Size unknown" rather than `0 B`. Handled at the call site rather than inside
`formatBytes`, because a formatter that invented a number would be the defect on screen; the card
already degrades in words for a file nothing could read.
Five of the seven new tests were red before a line of production code moved, with the numbers
they were about: `expected:<4321> but was:<0>` for a picked file, `expected null, but was:<0>` for
one nothing can measure, `expected:<[1111, 2222]> but was:<[0, 0]>` for the join picker, and
`expected:<[4321]> but was:<[0]>` and `expected:<[3333]> but was:<[0]>` for what the two workers
asked the space check. The tests assert on the *question* rather than the verdict, which matters:
one that only checked whether the job ran would have passed against the defect, since the defect
is that the guard is vacuous rather than that it refuses.
The remaining two needed the new call to exist first, so each was proved by mutation instead.
Answering the unknown with `hasSpaceFor(0L)` inline leaves `expected:<[]> but was:<[0]>` in both
workers; refusing it instead leaves `an unknown size must not end the job; got Failure {error :
Not enough free space to convert.}`. Making the worker always measure rather than trust a declared
size leaves `expected:<[9999]> but was:<[4321]>`.
`join()`'s own use of `InputQuery.total` is tested separately from the function, because
`StagingCleanupSupport` already records what that distinction costs: a tool can be provably right
while nothing calls it. `SucceedingWorkerFactory` now keeps the input `Data` of every request that
reaches a worker -- the only place it is legible, since `WorkInfo` hands back a job's tags and its
output and never the `Data` it was built with -- and the test reads the enqueued total off it.
Restoring `inputs.sumOf { it.sizeBytes ?: 0L }` leaves every other test in the change green and
this one red with `a total that could not be worked out must not be enqueued as a number`.
`ConversionViewModelProbeFailureTest` expected `InputFile(INPUT, "input", 0L)` for an authority no
provider serves. It expects `sizeBytes = null` now, which is the behaviour change stated where a
reader will meet it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-dev
|
49535998b5 | Merge branch 'fix/worker-durability-and-naming' into scratch/integrate-d2-d3 | ||
|
JMR-devandClaude Opus 5
|
159320dfa0 |
Name a finished file after the job that made it
Six places decided what a converted or joined file should be called, and not one of them asked
the job. `JoinViewModel` reported `JoinState.Saved("joined.mp4")` whatever the format;
`JoinScreen` opened `CreateDocument("video/mp4")` and launched it with `"joined.mp4"`. On the
convert side `save()` and `suggestedOutputName()` built the name from `_settings.value.spec`, and
the screen took the MIME type from the same place -- the picker as it stands *now*, which is not
the spec the job ran with.
All six are right today, and all six are right by accident. The join screen has no format picker,
so the three MP4 literals agree with `ConcatWorker.request`'s default. The conversion pickers are
drawn only in the `Ready` state, so the settings cannot move between enqueue and save. Neither
accident is load-bearing anywhere it is written down.
One of them has already stopped holding, quietly. A job picked up by `reattach()` ran with a spec
that was never in this ViewModel's settings, because those settings belong to a process that no
longer exists -- so a reattached MP3 conversion is offered `.mp4` and `video/mp4` today. The
previous commit made that path more reachable rather than less: `Reattachment` exists precisely
to find work this ViewModel did not start.
The fix is to ask the only thing that knows. The spec travels to the worker as input `Data` and
`WorkInfo` hands input `Data` back to nobody, so the worker is the single point at which the
input's name and the spec that ran are both in scope. Both workers now report the two derived
strings -- the name to suggest and the type to open the dialog with -- in their output `Data`,
and they ride on `ConversionState.Converted` and `JoinState.Joined` from there. `save()` reports
what it saved rather than recomputing it, and both screens read the name and the MIME type off
the state they already collect, remembering their `CreateDocument` contract against that type
instead of a literal. The MIME type is not cosmetic: some providers rewrite a document's
extension to match it, so an MP3 offered as `video/webm` can arrive with the wrong one.
`suggestedOutputName()` is deleted rather than repaired. With the answer on the state there is no
caller left for it, and an accessor recomputing the same string would only be a second place for
it to be wrong -- which is what it was.
`ConcatWorker` gains `DEFAULT_FORMAT` and `outputNameFor(format)`. Three copies of "MP4" is the
shape this entry is about, and the input-Data default, `request`'s parameter default and the
ViewModel's fallback for a job that predates this change are exactly three copies.
Work already in the queue carries neither string, and WorkManager keeps finished work for about a
week, so that is the ordinary case for a few days rather than a corner. Those fall back to the
old derivation, which is a guess -- but it is the same guess the app was already making, it is
confined to jobs enqueued before this commit, and for such a job there is genuinely nothing
better to hand. New work never reaches it. The join's fallback is not even a guess: the format
`ConcatWorker.request` has always defaulted to is the format such a job really used.
`reattach()`'s KDoc carried this as a known wart it was deliberately leaving alone. That
paragraph is now a description of the fix rather than of a defect.
Tested at both ends, since either alone would pass while the other was wrong.
`WorkerOutputNamingTest` runs the real worker on an MP3 job -- nothing like the default preset, so
a name built from the picker is visibly wrong rather than accidentally right -- and compares the
whole success `Result`, which is what makes a missing key fail rather than go unnoticed. The two
ViewModel tests drive a finished job and then move the picker, which is what a reattached job
amounts to from the ViewModel's point of view.
Restoring just the two name sources -- `save()`'s `outputNameFor(displayName, _settings.value.spec)`
and `JoinState.Saved("joined.mp4")` -- turns them red with
`expected:<holiday_converted.mp3> but was:<input_converted.webm>` and
`expected:<joined.mkv> but was:<joined.mp4>`. The convert side loses the extension *and* the name:
`_settings.value` had been moved to WebM, and the display name a reattached job cannot supply had
already fallen back to the placeholder.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
2a68f03134 |
Give every job a staging path of its own
`<cacheDir>/conversions/` is shared by the convert tab, the join tab and `ConcatEngine`, and
until now none of the three named a file that belonged to one job. A conversion derived its name
from the input's display name, so two `holiday.mp4` from different folders wrote the same file.
A join used the constant `joined.<ext>`, so any two joins of one format did. The list file was
the constant `concat_list.txt`, so any two joins at all did, and one of them would read the
other's input list.
The naming half is not a hypothesis. Two independent conversions on a Pixel each computed
`cache/conversions/input_converted.mp4`, the second silently overwrote the first, and a tag query
in a fresh process then returned **two SUCCEEDED `WorkInfo`s naming that one file** with one file
on disk. That is the collision reaching the point where it makes a *fix* ambiguous rather than
just a file: `Reattachment` can offer the bytes, because they are the user's either way, but it
cannot say which job produced them.
`StagingNames` keys the name on the WorkManager request id. That id is what stays still across a
retry -- `WorkerWrapper` builds `WorkerParameters` from the `WorkSpec` id and only increments
`runAttemptCount` -- which matters more here than uniqueness does, and matters more since the
previous commit made retries routine. A failed attempt deletes its staged file on the way out,
and that only collects the partial the *previous* attempt left when the name has not moved.
Opaque rather than sanitised, deliberately. The staged name is never shown to anyone: `save()`
recomputes a suggested name and the user picks the real one in the SAF dialog. So there was
nothing to lose by dropping the display name, and something to gain -- a provider-supplied
display name can contain a separator, be empty, or be four kilobytes long, and `File(stagingDir,
"../escape_converted.mp4")` resolves to a path outside staging. That was reachable before this
commit and is now unreachable by construction rather than by a sanitiser that has to be right
about every case. There is a test for exactly that name.
The extension stays, and is not decoration: `FFmpegConcatCommand` names no output muxer, so
FFmpeg infers it from the output path. A fully opaque name would quietly produce the wrong
container.
`ConcatEngine`'s list file is derived from the output it belongs to rather than taking another
parameter, so the two cannot drift apart, a directory listing shows which list belongs to which
join, and the sweep ages them together.
Three neighbouring comments claimed things that are no longer true, and are corrected rather than
left to mislead the next reader:
- `Reattachment.Ambiguous` said it "resolves on its own once each job stages under a name of
its own". It now does -- for work enqueued from here on. The case is **kept**, because the
queue outlives the change: WorkManager holds finished work for about a week, and the jobs
likeliest to be sitting in it when this code first runs are the ones named the old way.
Behaviour is unchanged and `ReattachmentTest` is untouched.
- `OutputPublisher.sweepStaging` justified its age rule partly on there being "no per-job
namespacing". There is now, and the rule still stands on its own: per-job names stop two jobs
from sharing a file, and say nothing about whether a file's job is still running, which is the
question a sweep actually asks. Same for `StagingSweep` and the note in
`LibreMediaConverterApp`.
- Both ViewModels' `reattach()` explained aliasing as something nothing prevented. Narrowed to
what is still true of work already in the queue.
`ConcatEngineTest` asks `StagingNames` for the list file's name instead of spelling out
`concat_list.txt`. That is the difference between a test and a tautology: a literal there would
have gone on passing after the rename while asserting that a file nothing creates does not exist.
The same trap was live in the two worker tests from the previous commits, whose staged-file
assertions computed a path of their own -- they now assert on the staging directory being empty,
which cannot go vacuous when a name moves.
`PerJobStagingTest` drives the real worker, because the naming function was never the part that
was wrong: what was wrong was which name the worker asked for. Two jobs converting one file must
leave two files; a second attempt at one job must not leave a second; and a display name that
climbs out of staging must not. The first and third fail before the change with "each job must
have staged its own file, found [input_converted.mp4] expected:<2> but was:<1>" and "the output
belongs in staging expected:<1> but was:<0>" -- the latter because the file had landed in
`cacheDir` instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
dcdcbfd3af |
Let a cancelled conversion stay cancelled
Both workers' outer `catch (e: Throwable)` caught `CancellationException` along with everything else and answered it with a `Result`. `runMedia3OrFallBack` goes out of its way to rethrow cancellation rather than fall back to software, and then the catch above it converted it anyway. A coroutine that reports completion inside a scope which has already been cancelled is structured concurrency's one rule broken, and it is the kind of break that stays quiet: nothing downstream complains, and the next thing to hold a resource across that boundary is the thing that finds out. Nothing on screen disagreed today, which is why this is a low-severity entry rather than a bug report. WorkManager cancels the worker's coroutine through `WorkerWrapper.interrupt`, which cancels `workerJob` with a `WorkerStoppedException`; the surrounding `withContext(workerJob)` then throws that whatever the worker returned, and `launch()` resolves it as `ResetWorkerStatus`. The returned `Result` is read only when nothing stopped the worker at all. So the change is about the shape of the code rather than about a symptom. One behaviour does move, and it is worth naming rather than discovering later. A cancellation that is *not* WorkManager stopping us -- FFmpegKit reporting `ReturnCode.isCancel`, which cancels the continuation -- now leaves `doWork` as a cancellation, and `WorkerWrapper` resolves a self-cancelled worker as `Resolution.Failed()` with no output data instead of the `Result.failure(KEY_ERROR ...)` it used to build. Both ViewModels already fall back on blank output data, deliberately and with a test, so the user sees "Conversion failed." either way. That is also the honest answer: the only route to `isCancel` is a cancellation someone asked for. The `staged.delete()` on that path is kept, and moved into the new branch rather than left to the one below it. A cancelled attempt leaves a partial in staging, the next attempt starts `doWork()` from the top rather than resuming it, and this catch holds the only handle to the file. Reaching any of this from a JVM test needed one more thing: `doWork` called `MediaProbe.probe` directly, and it was the last caller bypassing `ConversionDependencies.probe`. FFprobe's loader throws a bare `java.lang.Error` with no native library present, so no unit test could reach a single line below it. The seam's own KDoc says this is what it is for -- coverage of the branches that only run when something goes wrong -- and the default is the same real probe, so the app and the instrumented tests are unchanged. `WorkerCancellationTest` drives the real worker through a real `WorkManager` to the software engine, forced with `FORCE_SOFTWARE` because it is the one preference that decides without consulting the input, so the test does not depend on a routing rule it is not about. The engine stub writes bytes before it throws, which is what makes the delete assertion mean something: a stub that only threw would let a missing `delete()` pass. Three cases -- cancellation propagates, cancellation still deletes, and an ordinary failure is still answered with a `Result` carrying its message, which is the half that would break if the rethrow were widened past cancellation. Before the fix the first of those failed with "cancellation must leave doWork as cancellation, not as a Result; got null" -- `runCatching` had nothing to report, because `doWork` had returned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
5c27801c77 |
Retry work the system refused to start, instead of failing it terminally
`ConversionWorker`'s KDoc says the queue survives process death, and that is the app's stated
reason for choosing WorkManager at all. A Pixel 10 Pro XL says otherwise. An 18-minute
transcode was killed mid-write with `kill -9`; 119 seconds later, on a natural dispatch with
no `cmd jobscheduler run` anywhere in the session, WorkManager recovered it by itself and the
system refused it:
WM-ForceStopRunnable: Found unfinished work, scheduling it.
ActivityManager: Background started FGS: Disallowed [callingPackage:
org.libremediaconverter; uidState: CEM; BFGS denied: true; code:DENIED]
WM-WorkerWrapper: android.app.ForegroundServiceStartNotAllowedException
WM-WorkerWrapper: Worker result FAILURE
WM-Processor: Processor 3d1c9862 executed; reschedule = false
`Found unfinished work` is the clean process-death recovery path, not a force-stop. The job was
ordinary -- `Priority: 300 [DEFAULT]`, not expedited -- so there was no allowance it could have
carried. `HAS_FOREGROUND_EXEMPTION` was set on it and it was still denied; that flag governs the
runtime guarantee once a service is running, not permission to start one.
The cause is structural rather than subtle. `setForeground(...)` sat above `return try {`, so its
throw escaped `doWork()` without reaching `handleTimeoutIfNeeded`, `Result.retry()`, the
`workDataOf(KEY_ERROR ...)` or `staged.delete()`. Three separate costs, all measured on the
device: `reschedule = false`, so nothing ran again despite `run_attempt_count=2`; output `Data`
of `X'ABEF000100000000'`, a header with zero entries, so the screen said "Conversion failed."
with nothing to add; and 2 MB of a partial `long_input2_converted.mp4` left in staging.
`ConcatWorker` had the same shape.
So `setForeground` moves inside the `try` in both workers, and the staging handle is named just
above it -- `createStagingFile` only builds a path, nothing is written until an engine opens it,
so naming it early costs nothing and makes the catch total. `MediaProbe.probe` was outside the
`try` for the same reason and had the same problem; it is now inside too. On a retry the staged
name is unchanged, so the delete on the way out collects the partial the killed attempt left
behind rather than orphaning it.
What to return was the real decision. `Result.retry()`, because the denial is about *when* the
job ran and not about the job: the file is fine, the settings are fine, and the one thing that
grants an app permission to start a foreground service is being in the foreground, which the
user supplies by opening the app. But WorkManager never gives up on its own, so an unbounded
retry means a job nobody comes back for waking the device forever while the screen says "paused"
and never explains itself. `FailureOutcome` therefore bounds it at ten attempts, chosen against
the default backoff rather than as a round number: exponential from
`WorkRequest.DEFAULT_BACKOFF_DELAY_MILLIS` (30 s), doubling, clamped at `MAX_BACKOFF_MILLIS`
(5 h), which spans about 8 h 30 m before the eleventh attempt fails with a message the user can
act on. The counter is `runAttemptCount`, which counts every attempt and not only denied ones --
WorkManager exposes no other -- so a transcode already retried ten times by the six-hour budget
will fail on its first denial rather than getting ten of its own. That is accepted rather than
overlooked, and the timeout branch ignores the count entirely so the budget's own retries are
untouched.
The rule goes in `FailureOutcome` rather than beside it, which is what its KDoc asks for: isolate
a decision whose triggering condition cannot be provoked in a test. It now reads the stop reason
*and* the cause, with precedence stated rather than left to branch order -- once something has
stopped the worker, the exception it was holding describes that stop and not a reason of its own.
The match is on `ForegroundServiceStartNotAllowedException` exactly, never on its
`IllegalStateException` supertype: a muxer that was never started throws one of those too, and
matching the supertype would retry every ordinary failure for eight hours.
Expedited work is still not used, and this is not an argument for it. It maps to JobScheduler
expedited jobs with a short quota, which is the wrong shape for a multi-minute transcode; the
exemption it buys is for starting, and the quota it costs would be paid by every job.
Tested at both levels, because only one of them bites. `FailureOutcomeTest` gains six cases for
the rule -- retry at the bound, give up past it, an unrelated `IllegalStateException` that must
not be mistaken for a denial, a stop reason winning over the exception, and the budget timeout
ignoring the count. `DeniedForegroundStartTest` covers the wiring, which is where the defect
actually was: a `ForegroundUpdater` whose future completes exceptionally makes `setForeground`
throw exactly what the platform throws, because `WorkForegroundUpdater`'s own comment says it
propagates the exception to the caller and `ListenableFuture.await()` unwraps the
`ExecutionException` on the way. Moving `setForeground` back above the `try` turns all four of
those red with a bare `android.app.ForegroundServiceStartNotAllowedException`, which is the same
line the device logged.
Four comments claimed the old behaviour and are corrected with it. `ConversionState.Waiting`
said the budget had run out; `observe()`'s ENQUEUED branch said the budget was the likely cause,
where a denied restart is now the likelier of the two; and `Reattachment.choose` justified
excluding FAILED partly on interrupted workers coming back FAILED "rather than retried", which is
the sentence this commit falsifies. The exclusion stands on its own and says so now.
The bound's own KDoc says what giving up does *not* buy, too. `FOREGROUND_DENIED` reports through
a FAILED job and `Reattachment` excludes FAILED, so a user who was not watching at the eleventh
attempt meets an empty screen rather than the message. What the bound reliably buys is the end of
the retrying.
Both `Waiting` screens change their wording. The state is `ENQUEUED` with a run attempt behind
it, which cannot distinguish the two causes that now reach it, and the old copy named only the
six-hour budget -- which is now the less likely of the two. "Keeping the app open helps it along"
covers both, and for a denied start it is not filler but the actual remedy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-dev
|
07933c80c5 | Merge branch 'fix/native-boundary-guards' into scratch/integrate-d2-d3 | ||
|
JMR-devandClaude Opus 5
|
97fdc49f01 |
Guard the native boundary against what it actually throws
D14: picking a file died instead of reporting an unreadable one when FFmpegKit's native library could not load. `probeWithFFprobe` guarded its call with `catch (e: Exception)`, and `ConversionViewModel.onInputPicked` guarded nothing, so the failure escaped a `viewModelScope.launch` -- which has no handler, and on a device ends the process. All three of the obvious narrow guards catch nothing, which is why this needed reading the AAR rather than guessing. `NativeLoader.loadLibrary` catches the `UnsatisfiedLinkError` that `System.loadLibrary` raises and rethrows a *bare* `java.lang.Error` wrapping it, so `UnsatisfiedLinkError` never escapes and the escaping type carries no information at all. Every touch of the class after the first is a different type again -- `NoClassDefFoundError` -- so a guard written for the first shape lets the second pick onwards crash, which is the harder half to notice. Both are in the test output verbatim. `catch (Throwable)` was the wrong answer for the reason the audit gave: it would swallow a genuine `OutOfMemoryError` in a method that spawns a native process, turning "this device is out of memory" into "this file looks unreadable" and letting the app act on it. So the line is drawn by a named predicate, `isNativeLoadFailure`, rather than by the catch clause -- every class-loading shape is a `LinkageError`, and nothing that means the JVM is failing is one. That disjointness is what makes the guard narrow. This is consistent with the position `config/detekt/detekt.yml` already takes for `TooGenericExceptionCaught`: the boundary's failure types are undocumented, so guessing crashes the app on a file it could have reported. One level up the opposite mistake is available too, and the predicate is what lets both be avoided at once. `TooGenericExceptionThrown` is relaxed for the test source sets only. A test that reproduces a failed native load has to throw what the library throws, and a tidier subclass would leave it passing against a defect it no longer reproduces. Main source is untouched by that and throws nothing generic. `ConversionDependencies.probe`'s KDoc is rewritten rather than left. It said this hazard was "deliberately not fixed here ... its own commit, with its own test", which this is -- leaving it would have replaced one true comment with a false one, which is the same defect class as the D11 work. Both halves are covered independently: reverting the `MediaProbe` catch reds only the two `MediaProbeNativeLoadTest` cases, reverting the ViewModel guard reds only the two injected-seam cases, and widening the ViewModel guard to `Throwable` reds the OutOfMemoryError case -- so the narrowness is pinned, not just the catch. Audited the sibling boundaries named in the audit and left all three alone: `FFmpegEngine` and `ConcatEngine` both construct and run under `catch (e: Throwable)` in their workers, and `Media3Engine` has no native loader of this kind and already routes failures through `runCatching`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |