test/cancelling-a-running-export
100
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  JMR-devandClaude Opus 5
|
31f249ae04 |
Cancel a running Media3 export, completing #224's third engine
The two FFmpeg engines were done in |
||
|
JMR-devandClaude Opus 5
|
6992f0e783 |
Reattach to a conversion that is still running (#230)
Reattachment.rank gives RUNNING the highest rank of all -- "live work outranks a finished result because a running job is holding a foreground service" -- and no test on either source set had ever produced one. ReattachOnLaunchTest covers a job that finished, one whose staged file is gone, an ambiguous pair, one still queued, and one the user cancelled. ReattachmentTest exercises the ranking as a pure function over fabricated snapshots. What was missing is a ViewModel meeting a real running job, which is also the likeliest reattachment there is: the user starts a conversion, leaves, and comes back while it is still going. The engine is a fake, deliberately. The job has to still be running when the ViewModel is built, and every real conversion in this suite finishes in about a second -- racing that is what made the cancellation tests flaky enough to need retries. A SoftwareTranscoder that blocks until released removes the race outright. Nothing about reattachment depends on which engine is transcoding: the tag query, Reattachment.choose over live WorkManager state, and observe's mapping to Converting all run identically whatever is doing the work. This is what #230 can actually deliver, and the ticket asked for the answer either way. Process death itself stays device-manual. D3/D13 already record that am kill refuses a process holding a foreground service, and there is a more basic obstacle underneath it: instrumentation runs in the app's own process, so any route that really killed it would take the test runner with it and leave nothing to assert with. Observing a relaunch needs two instrumentation runs, which the runner does not provide. So the closest observable analogue is a fresh ViewModel, with no memory of the work, meeting a job that is genuinely mid-flight. The teardown now resets ConversionDependencies. The suite runs without Android Test Orchestrator, so a BlockingTranscoder left in place would hang the next class that converts anything. Verified on a local API 34 emulator: 67 tests, 0 failures, 3 skipped; and making RUNNING unreattachable in Reattachment.rank fails this test and nothing else -- which is also the evidence that the JVM ranking test was not already covering it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
802997439d |
Let a join read the files the user actually picked (#238, #225)
Joining files picked through the system picker failed outright whenever the strategy was stream copy -- the matched-files case the UI advertises as "joined without re-encoding, no quality loss". [ffkitsaf @ ...] Protocol 'ffkitsaf' not on whitelist 'file,crypto,data'! Error opening input file .../joined_from_content.concat_list.txt JoinScreen picks with OpenMultipleDocuments, so real inputs are always content://. ConcatEngine maps each through getSafParameterForRead and FFmpegConcatCommand writes the resulting ffkitsaf: paths into the concat list file. The demuxer applies its own protocol whitelist, defaulting to file,crypto,data, and -safe 0 does not touch it: that permits absolute paths, this permits the scheme they carry. Two separate gates, and only one was open. Nothing caught it because the two halves of the bug never met. Only STREAM_COPY feeds the demuxer a list file -- REENCODE passes each input with its own -i, where the whitelist does not apply -- so joining over SAF worked for mismatched clips. And every join test passed Uri.fromFile, which takes ConcatEngine's uri.path arm instead of the bridge, so matchingClipsAreJoinedByStreamCopy exercised stream copy with a file: path and passed. The one broken combination was the one no test produced and the only one a user can reach. That is #225's gap: FFmpegKitConfig.getSafParameterForRead is on every real conversion and join, and was on no passing test -- only on UnopenableUriTest's failure side, which proves the error message rather than the bridge. ContentUriInputTest now drives both the convert and join paths from a real content:// URI. It uses a plain ContentProvider, because the documents provider cannot be reached. Measured three ways: a DOCUMENTS_PROVIDER without MANAGE_DOCUMENTS is refused at install, instrumentation runs in the target app's process so Instrumentation.getContext() still carries the app's uid and is denied, and adoptShellPermissionIdentity(MANAGE_DOCUMENTS) is denied identically -- the denial naming ACTION_OPEN_DOCUMENT as the only way in. The bridge needs no documents provider: it opens a descriptor through the resolver, so any readable content:// URI exercises it, and an ordinary provider may be exported unprotected. The whole class stays headless. Recorded on #226, which that also settles: its cheap half does not exist. Verified on a local API 34 emulator: 66 tests, 0 failures, 3 skipped; and removing the -protocol_whitelist pair reproduces the production failure verbatim in the join test and nothing else. FFmpegConcatCommandTest pins the flag on the JVM. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
bc8e67888e |
Wait for the pick the launcher test is about (#220)
onInputPicked does not reach Ready on the calling thread. It hops twice --
withContext(pickDispatcher) { InputQuery.describe(...) } and then the probe
-- and pickDispatcher defaults to Dispatchers.IO, a real background thread
Compose's idling knows nothing about. So deliver() returned with the state
still Idle, and asserting immediately was a race the test usually won.
It lost five times on CI in one day, on PRs whose diffs were instrumented
tests and documentation and could not reach it. Two of those failures came
alongside #125's deadlock and could be argued as fallout; three did not.
waitUntil polls through waitForIdle, draining the main looper each time, so
it sees the recomposition the IO hop eventually posts back.
Injecting the dispatcher would be better and is not available here.
pickDispatcher is a constructor parameter precisely so a test can pin it,
but this test composes the real ConverterScreen, which resolves its own
ViewModel through viewModel() -- the seam is one layer below the launcher
edge this class exists to cover, and reaching for it would mean not testing
that edge.
The evidence is the mutation rather than the repetition count, per the note
#218 left: transposing the two launcher callbacks at ConverterScreen.kt:70
and :83 -- the exact defect this test guards -- still fails it, so the wait
did not make it vacuous. A transposed callback leaves the screen in Idle
forever and it fails on the timeout with the meaning it had before.
Supporting evidence, eight consecutive green runs of the class.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
163ce54b77 |
Stop the cancel tests losing their race on a loaded runner
Both cancellation tests I added in |
||
|
JMR-devandClaude Opus 5
|
5416788274 |
Cancel a running join session too (#224)
The FFmpegEngine half landed in ad2a75d; this is the same gap in ConcatEngine. Between them, a real native session being asked to stop is now covered on both FFmpeg paths. The assertion is the session's return code again, and here that is not merely the better choice but close to the only one: ConcatEngine does not delete its output on cancellation at all. Its invokeOnCancellation is FFmpegKit.cancel and nothing else, where FFmpegEngine's also deletes the partial. Whether that asymmetry is deliberate is a separate question, so this asserts what is true of both engines rather than depending on it. The cancel triggers on SessionState.RUNNING rather than on progress. ConcatWorker publishes no progress at all, so there is no callback to hang it on even in principle -- and the conversion side already measured the deeper reason, that the committed clips outrun a callback-triggered cancel. The inputs are the mismatched pair on purpose, so ConcatPlanner chooses REENCODE. A stream copy of two 2 s clips is close to instantaneous and would leave nothing to interrupt; re-encoding is also the case where a user would actually reach for Cancel. Verified on a local API 34 emulator: three runs green at 64/0/0/3, and replacing invokeOnCancellation's body with an empty block fails this test and nothing else, with state=COMPLETED rc=0. Media3Engine's transformer.cancel() is still uncovered and #224 stays open for it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
98c0e4dba2 |
Cancel a running FFmpeg session, which nothing had ever done (#224)
Every cancel in app/src/androidTest is WorkManager.cancelWorkById against work that is queued or already finished: ReattachOnLaunchTest cancels a job carrying a one-hour initial delay, and another immediately after enqueue. On the JVM, WorkerCancellationTest and HardwareFallbackTest's cancellation case drive a SoftwareTranscoder double that records the call. No test on any source set had asked a real native session to stop. That is docs/defect-audit.md D10's forcing condition. It is the one path where cancelling wrong is silently expensive rather than loudly broken: a missed FFmpegKit.cancel leaves the native process encoding to completion while the UI says the job is cancelled. Two things were measured rather than assumed, and both changed the test. The output file cannot be the assertion. invokeOnCancellation deletes the path, and on POSIX unlinking a file ffmpeg still holds open leaves ffmpeg writing to the unlinked inode -- so the path stays gone whether or not the cancel reached the session, and removing FFmpegKit.cancel passes that check every time. The session's own verdict is what separates them: a cancelled session ends with the cancel return code, a completed one does not. Cancelling from the first progress callback loses the race. It was tried first and failed with state=COMPLETED rc=0: every committed fixture is 2-3 s at 320x240, and the encode finishes before the first statistics callback is delivered and acted on. FFmpegKit.listSessions shows the session RUNNING far earlier, so that is what the test waits for. QualityTier.BEST is deliberate for the same reason -- preset medium leaves more of the encode ahead of the cancel. Verified on a local API 34 emulator. Four consecutive runs green at 62/0/0/3, and removing FFmpegKit.cancel while keeping output.delete fails this test and nothing else, with state=COMPLETED rc=1. ConcatEngine and Media3Engine carry the same shape and are not covered here; #224 stays open for them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
e0412329ff |
Press the Cancel button in the notification (#227)
ConversionNotifications.build attaches one action, wired to WorkManager.createCancelPendingIntent(id). Until now createCancelPendingIntent had no references anywhere outside its own declaration -- no JVM test, no instrumented test. That is worth more than an ordinary uncovered line. A conversion runs in a foreground service and the user is invited to leave the app; once they do, this action is the only way to stop it. If the PendingIntent carries the wrong id the button does nothing, the notification stays, and the job runs to completion, with no error, no log and no screen to look at. The obvious version of this test reads NotificationManager's active notifications for id 1001 and taps what it finds. Rejected: the instrumented suite grants no runtime permissions, so POST_NOTIFICATIONS is denied throughout, and whether a suppressed foreground-service notification is returned there is a platform detail that varies. The test would be asserting something about notification visibility rather than about cancellation. The PendingIntent is the subject and where it is read from is incidental, so this builds the notification for a real live work id and fires its action -- a real dispatch reaching real WorkManager, the same way on every API level. The job carries an initial delay so it stays ENQUEUED. A conversion of the 3 s fixture finishes in well under a second on an emulator, so racing a cancel against a running job would be flaky in the direction that fails, and cancelWorkById acts on ENQUEUED identically. What is under test is whether firing the action reaches WorkManager with the right id. Verified both ways on a local API 34 emulator: 61 tests, 0 failures, 3 skipped as written; and building the PendingIntent from a random UUID instead of the request's id fails the new test and nothing else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
948d53b67e |
Read the progress percentage FFmpeg has always been computing (#229)
FFmpegEngine derives progress as stats.time / durationMs * 100, and the statistics callback runs on every conversion in FFmpegEngineTest -- they all pass durationMs = 3_000. But every call site omits onProgress, so nothing on any source set had ever looked at the number. Replacing percent with a constant reddened nothing. What already existed covers the plumbing downstream and not this: #196 covered the worker's progress lambda with a fake engine that reports whatever the test tells it to, and ProgressNotificationTest covers the throttling the same way. The arithmetic was the one part with no reader. The new test passes 30 s as the duration for a fixture that is exactly 3.000 s, so the conversion still encodes the whole clip and the reported percentage tops out around 10 rather than 100. That is what makes it bite. A range check alone is worthless: a constant 0 satisfies both "every value is in 0..100" and "the values never go backwards", and so does a list of [0, 100]. Pinning the band rejects every constant, and because the band sits a tenth of the way up it also rejects an implementation that ignores durationMs, which would report ~100 for the same run. The bound is loose -- 5..25 for an expected 10 -- because the last statistics callback can land slightly before the final frame. Verified on a local API 34 emulator: 61 tests, 0 failures, 3 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
ba16f5a89b |
Make the fallback test say when it cannot test the fallback (#223)
HardwareFallbackTest is the only automated check of the hardware->software fallback against a real codec failure, and it passed on every CI leg without ever attempting the hardware path. Measured on run 34004304566: the API 33, 34, 35 and 37 legs each log Routing sample_h264_444.mp4 -> ... via FFMPEG (NO_HARDWARE_ENCODER) Emulators expose no hardware encoder, so the router never chooses Media3 and runMedia3OrFallBack's catch is never entered. The test's assertions -- succeeded, output non-empty -- are true of that conversion too. It finished in 448 ms, which is not long enough to fail an export and then software-encode a three-second clip. Deleting the catch reddened nothing. The ticket offered two fixes and left the choice open. Trying the first one answered it, and not the way the ticket expected. Pinning deviceCodecs to PERMISSIVE, as ForcedFailureTest does, makes the router choose Media3 -- and the export then SUCCEEDS. On a local API 34 emulator, MediaCodecInfo logs NoSupport [codec.profileLevel, avc1.F4000C, video/avc] for both c2.goldfish.h264.decoder and c2.android.avc.decoder, and ExoPlayer allocates the goldfish decoder anyway, which decodes the High 4:4:4 fixture regardless of the profile it declares. c2.android.hevc.encoder then encodes it and the job reports MEDIA3. So the class KDoc's "Media3 fails partway through the export on every device" is not true of the emulator images, and no routing pressure makes this fixture force a fallback there. Pinning would also swap in software codecs, which is not the path a real device takes -- it is what made the forced run succeed. That leaves assumeTrue on the production premise as the honest answer, now with a measurement behind it rather than a coin flip. The test skips where it cannot mean anything and runs on the Pixel, where it always could. When it does run the assertion is a pair, because KEY_ENGINE_USED is FFMPEG whether the fallback fired or the router went straight there: the router chose MEDIA3 for this request on this device, AND the worker reported FFMPEG. Together, and only together, that is the fallback. Verified on a local API 34 emulator: the test reports SKIPPED and the level reports skipped=3. ForcedFailureTest still covers the fallback wiring on every leg with a double; what needs a real encoder is two real engines disagreeing about a real file. The third permanent skip is recorded in docs/local-emulator.md and beside SafPickerRoundTripTest's run-shape note. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
9f06eb9988 |
Check that FLAC and Opus are FLAC and Opus (#228)
encodesFlacLosslessAudio and encodesOpus asserted only that a non-empty file appeared. Five siblings in the same class check what is in it -- encodesWav reads RIFF two lines away, and GIF, Matroska, MP3, H.264 and H.265 all assert a container marker or a track MIME. convert() throws on a non-zero return code, so these two did prove the command ran. What they could not distinguish is the command running and producing the wrong thing, which is a failure mode this codebase has already had once: F1 in docs/coverage-read-findings.md records a live Vorbis arm in FFmpegCommandBuilder that ContainerCapabilities says cannot exist. Pointing OutputFormat.FLAC's arm at pcm_s16le left both tests green. Four bytes each, in the idiom the class already uses. OutputFormat.FLAC is Container.FLAC, whose ffmpeg format is "flac", so the file opens with the native stream marker fLaC. OutputFormat.OPUS is Container.OGG -- "ogg" -- so it is an Ogg stream and opens with OggS. Verified against both muxers directly rather than assumed from the codec name; the marker belongs to the container, so it does not vary with the ffmpeg build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
c757565d64 |
Read the instrumented suite, and find the test that proves nothing
Four coverage waves have been steered by JaCoCo, which measures testDebugUnitTest only and cannot see app/src/androidTest at all. So nothing had ever asked what the 60 device tests pin, only that they were green. This is that read: a triage, not a test push, in the shape of docs/coverage-read-findings.md. Six findings (E1-E6) are things a test would not fix, and five of those six are prose rather than code — the suite itself is in good condition. Eight tickets carry the rest (#223-#230), each naming the mutation that has to go red rather than a coverage delta. The one that matters is #223. HardwareFallbackTest is the only automated check of the hardware->software fallback against a real codec failure, and it has never attempted the hardware path. Measured on run 34004304566: the API 33, 34, 35 and 37 legs each log Routing sample_h264_444.mp4 -> ... via FFMPEG (NO_HARDWARE_ENCODER) because emulators expose no hardware encoder, so the router sends the job straight to FFmpeg and runMedia3OrFallBack's catch is never entered. Its two assertions — succeeded, output non-empty — are true anyway. It finishes in 448 ms, which is not long enough to fail a hardware export and then software-encode a three-second clip. Deleting that catch reddens nothing anywhere. Two things generalise. A test can assert and still not reach, which neither a coverage number nor a "does it assert something" review can see; the filter that works is whether the test's premise holds on the machine running it. And the codebase already knew — ForcedFailureTest pins DeviceCodecs.PERMISSIVE against this exact hazard and says why, as does ConversionWorkerTest. Their assertions are about the path, so without the pin they fail loudly; HardwareFallbackTest's are about the output, so it passes quietly. That asymmetry is why nobody noticed. E5 records the structural reason this document is separate: F7 in the coverage findings calls probeWithExtractor's catch uncovered when RemuxTest drives it on a device every leg. A JaCoCo-derived document cannot see androidTest, so it will keep re-deriving that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
79097a0256 |
Record wave 4's landed coverage, and that the hang watchdog has fired
Numbers re-measured on
|
||
|
JMR-devandClaude Opus 5
|
32ab54da3c |
Wait for the rotation to rebuild the Activity, not for the composition to idle
thePickedInputSurvivesARealRotation synchronised a rotation with waitForIdle(), which waits for the compose hierarchy to settle. Right after a rotation the window manager has accepted but not yet delivered as a configuration change, the old Activity's composition is already idle -- so it returns, composeRule.activity still resolves to the old instance, and the guard reads an unchanged identity hash. Nothing waited for MainActivity to be rebuilt. That is the clean AssertionError on #217's API 33 gating leg, run 33698846104: it failed the SECOND guard, so the first had passed and the display really had rotated. The wedges this ticket opened with are the same race taken the other way -- land while the composition is being torn down and waitForIdle has nothing coherent to settle on. awaitRecreation() waits on a counter fed by the runner's lifecycle monitor, bounded at 15 s. Deliberately not polling composeRule.activity: that resolves through scenario.onActivity, which blocks on the main thread, so polling it across a recreation is a plausible reading of the very wedge being fixed. Both guards stay. The identity-hash one is now a backstop rather than the primary detector -- a configChanges attribute trips the barrier's timeout first, with a message saying what was waited for. Measured on the local API 33 emulator. With the fix, 60/60 green. With the watcher mutated so the counter never increments, the test fails in 15 s naming itself and its condition, and the run still reports received 60/60 completed cleanly -- where the same missing recreation used to cost the leg 20 minutes and name nothing. The pre-fix flake does not reproduce on this host, so that is a demonstration of the timeout path, not a before/after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
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
|
2db0dc65d3 |
Open the save dialog with the type the job produced (#201)
ConverterScreen.kt:80 -- state.pendingSave()?.mimeType ?: settings.spec.mimeType -- had never taken its left-hand side. Its comment records what the line is for: a retry after a failed save must open with the type its FIRST attempt used, because the fallback beside it is the current picker, which a reattached job never set. So the untested half is the fix and the tested half is the fallback it was added to stop being used. This withdraws a named exemption rather than working around it. FailedSaveRetryTest's KDoc listed the line as not asserted because "it lives in the entry point, above the ScreenContent seam, and reaching it needs a real ViewModel inside a composition". True when written; AdaptiveShellTest (#173) then established composing the real screens with real ViewModels, and #200 added the ShadowActivity mechanics for reading what a launcher launched. The reason the exemption gave no longer holds, so it is withdrawn in the same change rather than left to be taken at face value -- the shape of #141 revising #84's boundary. The job is reattached rather than run because the screen composes its own ViewModel through viewModel() and nothing can be injected into it. That is also the case the line exists for: a reattached job's spec was never in these settings at all. The test asserts the two mime types differ as well as which one is used. Without that, the assertion would pass just as well against the fallback if the fixture ever drifted onto MP4. Mutation: collapse :80 to settings.spec.mimeType -- red. Run and restored. Unrelated, and recorded because it turned up here: #159 now reproduces on this host. The full suite failed once in six runs on OutputPublisherStagingTest:184, and the isolating experiment says it is not this change -- three runs WITH the new test all passed, and the failure occurred on a run with the file removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
6334dcba34 |
Pin the launcher layer, where two callbacks share a signature (#200)
ConversionViewModel.onInputPicked(uri: Uri) and .save(destination: Uri) are both
(Uri) -> Unit, so swapping the two launcher callbacks at ConverterScreen.kt:70 and :83
compiles, renders, and passed the entire suite. Picking a file would attempt a save to it;
choosing a destination would load it as input.
That is the defect class ScreenWiringTest exists for, on the one pair it declines to cover:
it drives converterActions directly and says the launcher-backed actions stay parameters.
Right about the actions seam, and it leaves the edge above that seam unpinned. Join's
equivalents are List<Uri> and Uri, so they are not transposable and get no such test.
Two mechanics, neither used anywhere else in the suite, so both were spiked before any
assertion was written:
shadowOf(activity).nextStartedActivityForResult reads the launched Intent, EXTRA_MIME_TYPES intact
shadowOf(activity).receiveResult(...) reaches ComponentActivity's ActivityResultRegistry
and fires the rememberLauncherForActivityResult callback
createAndroidComposeRule for AdaptiveShellTest's reason: the screens compose real ViewModels
through viewModel(), and the plain rule supplies no ViewModelStoreOwner.
The picker filter rides along, since the harness is the same. ConverterScreen.kt:65-67
records why the all-types wildcard is load-bearing -- the photo picker offers no audio and
misses mkv/flac/webm -- and narrowing it would have made every audio conversion unreachable
from the picker with nothing going red.
One incidental: a KDoc cannot contain the all-types wildcard, because its second half closes
the block comment. The literal is spelled only in the assertion, and the KDoc says why.
Mutations, all run and restored:
transpose onInputPicked and save 1 red
narrow the converter picker to video only 1 red
widen the join picker to every type 1 red
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
7e09f010c7 |
Call the theme the way MainActivity calls it (#197)
ThemeColorSchemeTest resolves every branch of the `when` and always passes darkTheme explicitly, so the $default bridge is never entered and isSystemInDarkTheme() is never called. MainActivity.kt:79 is its only default-argument caller and does not execute on the JVM, which left the app's actual call shape -- no arguments at all -- the one nothing exercised. LibreMediaConverterTheme reported mi=21, mb=6, cb=12 at method level. Not #68. That issue is the two unreachable arms, DarkColorScheme and LightColorScheme, which cannot run because dynamicColor is always true and nothing can flip it; it is an open product decision and stays open. This is the reachable half. The assertion compares schemes rather than reading a luminance threshold, which would be a guess about the device palette. What is asserted is that the no-argument call resolves the SAME scheme an explicit darkTheme of the matching value does, and a different one from its opposite -- true whatever palette the platform hands back, and exactly the claim being made: the default reads the system rather than picking a side. The two assertions are also what stops the pair passing vacuously if all three resolutions were identical. Two @Config(qualifiers = ...) cases rather than two classes: qualifiers are settable per method, unlike the sdk pinning ForegroundTypeRegimeTest needed nested classes for. Mutations, all run and restored, and each reddening a different half -- which is also what shows the qualifiers take effect rather than both cases running in one mode: darkTheme defaulted to false night case red darkTheme defaulted to true light case red isSystemInDarkTheme() inverted both red Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
49249be280 |
Report hardware progress to WorkManager, which nothing had checked (#196)
ConversionWorker.kt:208-210 is a second onProgress lambda at a second call site -- the one handed to engine.transcode -- and it reported ci == 0. Every test in this file drives the FFmpeg path; HardwareFallbackTest reaches runMedia3OrFallBack but its recording transcoder records the call and never invokes the callback it was handed. So the two engines' progress wiring was one tested and one not, and the untested one is the default: ConversionRouter sends everything it can to Media3, which makes this the lambda most conversions actually use. Same asymmetry argument CLAUDE.md records for ContainerCapabilities:94. It goes in this file rather than beside HardwareFallbackTest because this is where progress plumbing lives and where RecordingForegroundUpdater already is -- and because the software and hardware cases now sit side by side, which is what makes the asymmetry visible rather than merely fixed. workerReporting gains an engine-preference parameter defaulted to FORCE_SOFTWARE, so no existing case changes. AUTO with a real H.264 probe, because FORCE_SOFTWARE is exactly what keeps the other tests out of this branch, and because InputProbe() reports UNPARSEABLE -- which PERMISSIVE.canDecode refuses, sending every job to FFmpeg with no test saying why. The percentage is asserted, not merely that an update happened: publishProgress takes a display name and a percent, and replacing the percent with a constant compiles fine. Mutations, both run and restored: empty the hardware onProgress lambda 1 red report a constant percent instead of the engine's 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
|
da8d53851b |
Render the container row for a video nothing could name (#199)
ConverterScreen.kt:668's null arm -- DetailRow("Container", probe.container?.label ?:
"Unknown") in the VIDEO branch -- had never rendered. Every video case in FileCardTest uses
VIDEO_PROBE, which carries container = MP4.
The argument for adding it is the asymmetry, not the coverage. FileCard renders that exact
expression twice, once in AUDIO_ONLY (:660) and once in VIDEO (:668), and "an audio-only
file nothing else could describe degrades one row at a time" drives only the first. Same
expression, same fallback, one kind covered and one not -- which is the same argument
CLAUDE.md records for including ContainerCapabilities:94.
Nor is null an edge case here. InputProbe.container's own KDoc says MediaExtractor cannot
report a container at all, so it comes from FFprobe alone: any run where FFprobe did not
answer produces exactly this shape -- real codec, real dimensions, real duration, no
container. An empty value in its place would read as a rendering bug rather than as a
probe that got half its sources.
The other three rows are asserted alongside, which is what keeps this from being a copy of
the audio-only case. There, everything is unknown at once; here one field is missing from
a probe that is otherwise complete, and the rest have to be unaffected by it.
Mutation: `?: "Unknown"` -> `?: ""` at :668 only, run and restored. The AUDIO_ONLY twin at
:660 is a separate expression, and mutating that one would redden the existing test instead
-- which would prove nothing about this one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
cc215195ee |
Build a command for the audio the user turned off (#198)
audioArgs' Drop arm -- `AudioPlan.Drop -> listOf("-an")` -- was ci == 0. The suite's only
-an assertion lives in "gif generates a palette to avoid banding and drops audio", and that
one comes from the image path at FFmpegCommandBuilder.kt:79/:90, which emits -an directly
and never reaches audioArgs. Two sites, one string, one tested.
It is a live path rather than defensive code. AdvancedPicker renders all of
AudioCodec.entries including NONE, ContainerCapabilities.validate permits audio-off whenever
the input has video, and MKV routes the job to FFmpeg -- so "convert this and drop the
soundtrack" is something a user can do today and nothing had built the command for.
Both halves are asserted, and the second is not padding: -an alone still passes if the arm
falls through to the else and emits an AAC encoder beside the flag, which is a file that is
silent because the flag won while carrying an encoder nobody asked for.
Three mutations, all run and restored. The third is the one that justifies the second
assertion, since the first two break -an as a side effect and so cannot show it:
Drop -> emptyList() red
Drop -> the else arm's aac encoder red (loses -an as well)
Drop -> listOf("-an", "-c:a", "aac") red -- -an intact, caught by assertFalse
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
92bcff8656 |
Make a failure that says nothing still say something (#193)
Three sites, all ci == 0 before this, and all the same rule: work/ConversionWorker.kt:316 cause.message ?: GENERIC_FAILURE_MESSAGE convert/ConversionViewModel.kt:631 e.message ?: SAVE_FAILED_MESSAGE join/JoinViewModel.kt:416 e.message ?: SAVE_FAILED_MESSAGE Every existing test throws WITH a message, so the right-hand side had never been evaluated anywhere in the suite. A Throwable carrying none is not exotic: RuntimeException(), IOException() and most platform exceptions raised without an argument all have a null message, and a native engine that dies is exactly where one comes from. The worker case needed care, and the care is the reason it survived three waves. ConversionStateMappingTest's "a failure with nothing said still says something" looks like it covers that site and does not -- it drives the READ side, map(FAILED, Data.EMPTY), and that side has a fallback of its own at ConversionViewModel.kt:147-149 which turns a blank KEY_ERROR back into the same constant. So a test asserting on the resulting Failed state stays green while the worker's fallback is broken. Measured rather than reasoned: with :316 mutated to .orEmpty(), exactly ONE of 587 tests went red, and it was the new one. Everything else, including the test that appears to cover it, stayed green. So the worker case reads KEY_ERROR off the worker's own Result, before anything downstream can repair it. The two save cases have no such second line -- both write _state.value directly -- so the state is the right thing to assert there, and both also assert that `pending` still travels: a fallback that dropped the handle would leave the file unreachable from the very screen that just said the save failed. Held in one class against the ticket's suggestion of three. They are one rule at three layers, and the masking above has to be explained once rather than three times. FailedSaveRetryTest already sets the precedent for both ViewModels in one file; this adds one worker to that shape. FORCE_SOFTWARE in the worker fixture so the failure comes straight out of runFFmpeg. AUTO would enter runMedia3OrFallBack, whose catch runs the job a second time in software: the same exception arrives, but by a path this is not about and which HardwareFallbackTest owns. Mutations, all run and restored -- each site to .orEmpty(), never to a different constant, which would only prove the test reads a constant: ConversionWorker:316 1 of 587 red (this file) ConversionViewModel:631 red JoinViewModel:416 red Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
e4867ff956 |
Connect the Cancel button to WorkManager, which nothing did (#192)
Both ViewModels' cancel() is one line -- activeWorkId?.let(workManager::cancelWorkById) -- and JaCoCo reports every line of both as covered. The only test of either was SettingsEditsTest's "cancelling with no active job does nothing rather than throwing", whose own comment names the half it drives: "the null side". The other side had never been entered, and JoinViewModel.cancel() had no test at all. So nothing in 584 tests connected the Cancel button to WorkManager. The affordance tests click TestTags.CANCEL and assert the action fires into a stub; ScreenWiringTest asserts the action calls viewModel.cancel(). Both halves were pinned and the join between them was not. No line-level filter could have found this. It takes a method-level read -- mi=11, ci=7, mb=1, cb=1 on both -- a covered method with an arm nothing takes, which is the second of the two filters #194 records and the gap that argued for adding it. The fixture is why this stayed uncovered rather than why it is hard. The test WorkManager runs on a SynchronousExecutor, so an ordinary request finishes inline: by the time a test could call cancel(), convert()'s job was already terminal, leaving only the null arm reachable. setInitialDelay is what TestScheduler honours, so the job sits in ENQUEUED and the test never releases it. Production never sets a delay, so the request is built by hand rather than through ConversionWorker.request -- but ENQUEUED at runAttemptCount 0 is a real state every job passes through, Reattachment.choose ranks it QUEUED, and conversionStateFrom maps it to Converting(input, 0). The delay changes how long the job stays in a real state, not which state it is in. Three tests, and the third is not padding: without it, cancelAllWork() in place of cancelWorkById(activeWorkId) passes the other two. It asserts the shape rather than the identity -- exactly one of two queued jobs is cancelled -- because which one the ViewModel reattached to is the query's business, and Reattachment's ordering notes say queued jobs are left tied deliberately. WorkManager's own record is asserted before the screen. The screen alone would be weaker than it looks: CANCELLED maps to Idle for a reattached job, and Idle is also where a ViewModel that did nothing whatsoever would sit. Mutations, both run and both restored: cancel() -> no-op all three red cancelWorkById(id) -> cancelAllWork() only the third red The second is what shows the third test does independent work rather than restating the first two. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
223fe6deea |
Record what the wave-4 coverage read found, and correct the filter that missed the biggest gap
Five findings (F6-F10) and a methodology correction. The twelve test tickets the same read produced are #192-#203, with #204 for four candidates whose cost was not obviously worth paying; nothing here is work, by this document's standing rule. The correction is the part worth carrying forward. Wave 3 filtered candidates on `mi > 0` and CLAUDE.md recommended it. That filter fails in both directions. It over-reports on Compose: JoinScreen.kt:222 reads mi=10 and also ci=38, and JoinStateAffordancesTest already clicks that Save button and asserts save:joined.mp4 -- the missed instructions are the synthesized $changed/$dirty recomposition-skip path, the same codegen this repo already knew inflated the branch count, showing up in the instruction count too. Every onClick lambda flagged that way turned out to be covered at method level. It under-reports on the case that mattered more. ConversionViewModel.cancel() and JoinViewModel.cancel() miss no line at all, so no line-level filter can see them -- yet only the null arm of activeWorkId?.let(workManager::cancelWorkById) had ever been entered, and nothing in 584 tests connected the Cancel button to WorkManager. That is #192, and it needs `ci > 0 && mb > 0` at method level to surface. Use both filters; `ci == 0` alone is JaCoCo's own missed-line definition and needs no judgement, which is why it is the first. The five findings are what a test would not fix. F6: four more unreachable arms, each traced to the upstream guard that makes it so, one of which (ConversionRouter:214-217) carries a KDoc describing a hazard :117 already removed. F7: probeWithExtractor's catch is unreachable for the same reason probeForConcat's is -- the measurement was on record for one site and not the other, three lines apart in the same file. F8: three more dead members and six unused defaults. F9: both getForegroundInfo overrides are dead because getForegroundInfoAsync is only called for expedited work and nothing sets it -- which sharpens #88's close rather than reopening it. F10: three arms that ARE reachable and still cannot be made to bite, recorded because all three were picked up as candidates and put down again. Six of the ten findings are now "no action" or "not a test gap", and that shape is the honest summary of what is left: arms nothing can reach, members nothing calls, and arms a test can reach but not pin. A coverage number tells none of them apart. One close is qualified rather than overturned. #86 and #133 ruled AndroidDeviceCodecs.probe() out through ShadowMediaCodecList, on the grounds that MediaCodecInfoBuilder cannot set isAlias or canonicalName. A pure seam does not have that constraint and #133 did not evaluate one, so #194 is a different mechanism, not a third run of the same spike -- and its argument is not coverage but that the runCatching fallback logs "assuming permissive" while returning empty sets, which makes canEncode and canDecode answer no for everything. Documentation only: no Kotlin, Gradle or shell file is touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
5a5a680b4c |
Re-measure after wave 3, and write down the two kinds of gap it had to separate
92.8% line (2183/2352), 81.3% branch (1091/1342), 584 JVM tests in 87 classes, measured 2026-09-02 on the tree this branch creates rather than quoted from a PR body. The shape of the wave is worth more than the number, and it is different from the two before it. Waves 1 and 2 were finding uncovered code; by wave 3 there was little of that left, so the gaps had to be sorted before any test was written. Coverage gaps -- filtered to sites where JaCoCo reports mi > 0, which is what separates a real gap from a partial branch on a compound condition, and which cut the candidate list roughly in half. And assertion gaps, where JaCoCo is green and nothing checks the answer: MainActivity's rail and bottom bar were both executed and transposing them passed the entire suite, as did swapping the two progress-notification strings and swapping Content's two destinations. No coverage number would have found any of the three. Naming the required mutation per ticket earned its keep three times, each recorded with what the weak assertion actually was. Also recorded: a green mutation is only evidence when the mutation is a real change -- one classify reordering was semantically equivalent for every reachable input, and a bad mutation and a weak test look identical in the output. Two entries came back as not gaps, which is a result rather than a shortfall: ContainerCapabilities:282's exclude filter cannot drop anything, and probeForConcat's catch arm is unreachable on this runtime -- Robolectric's MediaExtractor never throws from setDataSource, measured across four input shapes. Both denominators moved, in opposite directions and for different reasons, so they are stated rather than folded into the percentage: 1340 -> 1342 branches from MediaProbe.merge, 2348 -> 2352 lines from the ConcatJoiner interface. Neither is new untested code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Recovered onto main after hitting #160's trap for real. #189 was opened against test/concat-engine-seam and, unlike #184-#188, never retargeted to main before merging -- so it merged into a branch that had already been merged and left behind. GitHub reported `merged`, the PR shows MERGED, and none of it was on main: `git merge-base --is-ancestor` is what said so, one line, immediately. That check is the entire reason this was a five-minute recovery rather than a coverage entry that silently stayed three points stale. The failure mode is exactly what CLAUDE.md warns about; what it did not say, and now would, is that the auto-retarget it describes belongs to GitHub's stacking feature, so a stack opened with plain `gh pr create --base` has to be retargeted by hand for every single PR -- and missing one is invisible until you check ancestry. Numbers re-measured on this tree with --rerun-tasks rather than inherited from the branch they were taken on: 584 tests, 0 failures, 2183/2352 line, 1091/1342 branch. Same figures, earned again. |
||
|
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
|
b41341a1cb |
B3 (#175): pin the AAC arm every ordinary conversion takes
audioArgs has six arms. Five are named codecs with tests; AAC arrives through the `else`, so nothing named it -- neither "aac" nor "192k" appeared anywhere in FFmpegCommandBuilderTest. It is the audio MP4 and M4A get, which is to say the audio the picker offers first and most conversions produce. Both halves are asserted, and the bitrate is the half worth arguing for: an -b:a that quietly changed would fail nothing, look wrong in no command line, and surface only as files that sound different from the ones the app produced last month. Both mutations confirmed red -- 192k -> 128k and aac -> libfdk_aac. Asserted through MP4_H264 and M4A_AAC rather than one of them, so an AAC arm added above the `else` later has to keep answering the same way for both. **Deliberately not added here: an ENCODABLE_AUDIO-vs-audioArgs agreement test**, the obvious companion to VideoCodecMimeAgreementTest. It would freeze the answer to F1, which is open: ContainerCapabilities.kt:84 says "nothing here emits a Vorbis encoder" and FFmpegCommandBuilder.kt:188 does. docs/coverage-read-findings.md says in terms that the tempting fix there locks in the wrong answer and that the decision comes first. This is the AAC arm only. 563 -> 564 JVM tests, 0 failures. No production code changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
0f842243b5 |
B2 (#174): read what the progress notification actually says
An assertion gap rather than a coverage one, which is the reason it survived. JaCoCo is green on build()'s `if (indeterminate)` because ProgressNotificationTest drives it through a real worker -- but that test reads the notification id and EXTRA_PROGRESS and nothing else. Nothing had ever read the text. Swapping the two branches passed the whole suite; so did replacing the caller's title with a constant. Both are red now. What it costs to get wrong is small and permanent: a conversion four minutes in still saying "Preparing", or one that has not started reporting yet claiming 0%. Neither is a crash, and nothing else here would have found it. Nothing in the suite had constructed ConversionNotifications directly, and the reason turned out to be mechanical rather than an oversight: build() reaches WorkManager.getInstance for the Cancel action's PendingIntent, so the notification cannot be built without one. installTestWorkManager in setUp is the whole fixture, and the KDoc records the coupling so the next person does not rediscover it. areEnabled() in the same file is deliberately still untested. It has no caller anywhere in app/src/main, so a test would assert that a function nobody calls returns what the platform told it -- and would imply the app handles the disabled-notification case, which it does not. That is F5 in docs/coverage-read-findings.md, and it asks for a decision rather than a test. 561 -> 563 JVM tests, 0 failures. No production code changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
2fbc957119 |
A5 (#171): fire the muxer guard that repairs "MP4 for everything", which had never fired
Media3Muxers' KDoc names the defect this guards -- "the router claimed five containers while the engine silently wrote MP4 for all of them" -- and the repair itself was untested: Media3Engine$buildTransformer$3, the requireNotNull message lambda, was four lines and four branches at 0%. Nothing had ever driven a plan whose container Media3 cannot mux, and factoryFor answers null for fourteen of them. Weakening it does not crash. The wrong output is a playable file with the wrong container, which is why a test rather than a bug report is what would catch it. Same harness and the same two disciplines as Media3EngineEmptyCompositionTest, which is the sibling this joins: assert the plan really is the one the test needs before driving the engine, and rule out CancellationException so an unresumed continuation cannot read as a pass. Three premises are asserted here rather than assumed -- that the plan is still WebM by the time the engine sees it, that Media3 really has no muxer for WebM, and that neither track was dropped, since the empty-composition refusal fires earlier and is a different test's subject. The assertion is on the exception type *and* its message, and the ticket predicted why: replacing requireNotNull with `?: DefaultMuxer.Factory()` does not make the export succeed, it lets it run on and fail some other way. Measured -- that mutation fails the type assertion, so the guard is genuinely what this test is holding, and the message assertion stands behind it. 560 -> 561 JVM tests, 0 failures. No production code changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
a645442acc |
A2 + A3 (#168, #169): the hardware fallback, the cancellation that must not take it, and the name a job may not have
runMedia3OrFallBack was eleven lines at 0% and isCancellation had never been called by any JVM test -- ci=0, not merely a missed branch. The seam to reach it has existed the whole time: ConversionDependencies.hardware, which no unit test had ever set. What kept the path cold is that every worker test uses EnginePreference.FORCE_SOFTWARE, which never enters the function, and the probe defaults to UNPARSEABLE, which PERMISSIVE.canDecode refuses -- so even AUTO would have routed straight to FFmpeg for a reason no assertion mentioned. Both are now stated in setUp rather than inherited. Four behaviours, each with the mutation that proves it: hardware failure falls back to software delete the fallback call ... on a *clean* staging file delete staged.delete() before it cancellation is rethrown, not fallen back delete `if (isCancellation(e)) throw e` engine.close() runs either way empty the finally block the display-name fallback (#169) change "input" to anything else All five confirmed red, then restored. The cancellation one is the reason this ticket was first in the group. runMedia3OrFallBack catches Throwable, so without that re-throw a user cancelling a hardware transcode has the app quietly start a *second* conversion in software -- the one thing cancelling is for. ForcedFailureTest covers the failure half on a device and does not cover this half at all. The clean-staging assertion is made where it is observable rather than by reading the file: the software fake records whether the output existed when it was entered, so a missing delete shows up as FFmpeg finding a half-written hardware output at the path it is about to write. 556 -> 560 JVM tests, 0 failures. No production code changed. ConversionWorker: 22 -> 9 missed lines, 14 -> 7 missed branches. Line 2090/2348 -> 2103/2348; branch 1022/1340 -> 1029/1340. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
5761faced6 |
A4 (#170): join the two halves of an unreadable join clip, and record why the catch arm stays device-only
The ticket asked for two things. One of them is not reachable from the JVM, and saying so is most of the value here. **probeForConcat's catch arm cannot be provoked on this runtime.** Robolectric's MediaExtractor never throws from setDataSource -- measured across four input shapes: an unregistered content:// authority, a missing file://, a file of garbage bytes, and an http:// URL. All four returned normally with trackCount = 0. So a failed read arrives as an empty track list rather than as an exception and reaches the same ConcatInput(null, null, 0, 0, 0) by the other road. The catch stays covered only by ConcatEngineTest on a device. The test file says this rather than implying the arm is handled. **What is reachable, and was genuinely missing, is the span.** Both halves were already covered and neither reached the other: MediaProbeTrackWalkTest pins what concatInputFrom makes of a track list, ConcatPlannerTest's `an unknown codec is not treated as a match` pins what the planner does with a hand-built ConcatInput(video = null). The planner's safety rests on the probe really producing that shape, and the hand-built fixture would go on passing if it stopped. Measured rather than claimed: mutating concatInputFrom's initial `video` to a non-null placeholder leaves ConcatPlannerTest green and turns this red. Dropping the planner's video null guard turns both red -- so that half was already held, and this file does not claim credit for it. The coupling itself is worth writing down: ConcatPlanner guards video against a null codec and audio not at all, and that asymmetry is correct rather than an oversight -- MediaProbe.shortName returns a non-null String, so a null audioCodec means the track is absent and two clips with no audio really do match, while a null videoCodec means absent *or* unreadable. The audio check is safe because the video guard fires first on a clip nothing could read. Nothing held that. 555 -> 556 JVM tests, 0 failures. No production code changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
c2c0bfa848 |
A6 (#172): six one-branch outcomes nothing produced, and one that cannot be produced
Each of these is a site where JaCoCo reported mi > 0 -- a concrete instruction no test
runs -- rather than a partial branch on a compound condition, which is how the group was
filtered in the first place. Six closed, one moved to the exclusions.
AndroidDeviceCodecs:35 the UNPARSEABLE sentinel, refused where an unknown name is not
ContainerCapabilities:94 accepts(container, VideoCodec.NONE, mode) -- the audio twin has
had a test since #136; the asymmetry is the argument
ContainerCapabilities:323 repairVideo's keep-the-requested-codec arm
ContainerCapabilities:348 firstContainerHolding's fallback container
OutputPublisher:230 the resolver call that throws -- the third case the KDoc names
and the one the shape list was missing
JobSnapshots:31 a job that recorded no output path at all
Every one was mutated and confirmed red, then restored. Two are worth stating because
they did not go red first time or would not have:
**The firstContainerHolding test was vacuous on its first draft.** It asserted the
refusal still offered *something*, and deleting the fallback left it green: the source
container is a candidate in its own right, so the list stays non-empty and only its
contents change. Rewritten around AVI, which has no mapping for H.265, and asserting the
codec survives -- without the fallback the app silently offers H.264 instead, which is
the actual loss. This is the failure mode CLAUDE.md records from the mutation review, met
head on rather than in the abstract.
**OutputPublisher:230 needed one line.** `a destination whose size cannot be determined is
never deleted` already walked three RowShapes; QUERY_THROWS was the fourth case its own
KDoc names -- "a resolver call that throws" -- and the only one that reaches `?: false`
through runCatching rather than through a cursor answer.
ContainerCapabilities:282's `.filter { it != exclude }` is **not** closed here and is not
a gap: nothing can make it drop anything. On the shared container `repair` always changes
at least one codec, because a codec it left alone is one validate would not have refused;
every other candidate differs by container; and the single call site passing a non-default
exclude (validateVideo:186) excludes a spec carrying VideoCodec.COPY while every repaired
candidate carries NONE. F4-shaped -- recorded rather than covered, and 550 tests agree.
550 -> 555 JVM tests, 0 failures. Five new tests rather than six: OutputPublisher:230
is one line inside a test that already existed. No production code changed.
Line 2087/2348 -> 2090/2348; branch 1011/1340 -> 1022/1340.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
016030f3e4 |
A1 (#167): pin all three foreground-service regimes, and the boundary between two of them
`ConversionForegroundType.current()` has three arms and the JVM suite executed one. `robolectric.properties` pins everything to `sdk=36`, and `@Config` appears nowhere in `app/src/test`, so 3 lines and 3 of 4 branches were cold. The instrumented test is not a substitute, and the reason is specific rather than general. `ConversionWorkerTest.foregroundTypeMatchesTheRunningApiLevel` asserts against whichever API the leg is, so it covers one arm per leg and never the other two -- and the legs that would cover 33 and 34 are the ones #122 wedges. From docs/coverage-read-findings.md, an API 33 run reported `received: 60` with `failed: unknown`: the regime was exercised and that leg could not have said so if it had broken. This runs all three deterministically in the same ./gradlew invocation. Four classes, not three. 35 shares its answer with 36 and looks redundant; it is the whole point. Relaxing `>= VANILLA_ICE_CREAM` to `>` is invisible at every level except exactly 35 -- measured, not assumed: that mutation failed ForegroundTypeApi35Test alone, while swapping DATA_SYNC and MEDIA_PROCESSING failed 34, 35 and 36. Without the 35 class the first mutation survives the suite. 546 -> 550 JVM tests, 0 failures. No production code changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
dbedfb4708 |
Re-measure coverage after wave 2, and write down how a stacked PR merges
COVERAGE. 87.1% line / 69.1% branch, 502 tests -> 88.9% line (2087/2348), 75.4% branch (1011/1340), 546 tests in 76 classes, as #153's five children land. The branch figure moved for two reasons and the entry now says so, because only one of them is new tests. The numerator rose 974 -> 1011; the denominator *fell* 1410 -> 1340. Both are the seam work: pulling a `when` out of a lambda inside a `collect` deletes the coroutine state machine's synthesized branches around it, and leaves a plain function whose branches a test can choose. `ConversionViewModel$observe$1$1` went from carrying the whole mapping to six branches, while the extracted `ConversionViewModelKt` covers 41 of 42 and `JoinViewModelKt` 38 of 39. That is worth stating rather than quoting the percentage alone. A number that rises because the denominator shrank is a different claim from one that rises because more branches are tested, and this entry has a documented history of explaining its own movements wrongly. STACKED PRS. A new Conventions entry, from two traps measured on 2026-08-27 while landing #144-#151. `gh pr merge` refuses a stacked PR outright -- "must be merged using the asynchronous merge REST API" -- and so does the plain `/merge` endpoint. The one that works is `PUT .../pulls/N/merge-async`, which returns a uuid to poll. The second is worse because nothing looks wrong. GitHub retargets a stacked PR's base to main when the one below it merges, but asynchronously. Merging five about thirty seconds apart outran it, so each merged into its own already-merged base branch. Every call returned `status: merged`, every PR read MERGED, `gh pr list --state open` was empty, and none of the content was on main. What caught it was a coverage re-measure two points below what the same tree had produced an hour earlier -- a fresh `git pull` changed nothing, which is what made it a question rather than a stale checkout. `git merge-base --is-ancestor` answers it in one line. #160 is what the recovery cost. Also recorded: the auto-retarget belongs to the stacking feature. A PR opened with a plain `--base some-branch` does not retarget when that branch merges, and has to be moved by hand -- which is what #163 needed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
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
|
a507736d3d |
W4 (#157): the seven settings edits, and three tests that did not bite until they did
`setPreset` was covered; the six beside it and `cancel()` had no coverage at all.
That asymmetry is the tell -- they are reachable from the JVM suite by exactly
the route `setPreset` already takes, and nothing had asked.
WHAT IS ASSERTED. Not "the setter sets something". Each of these copies into a
nested `OutputSpec`, so the failure worth catching is a setter that writes the
right value into the wrong field, or that rebuilds the spec and quietly discards
the other two. Every test asserts the field it changed AND that the rest survived.
THREE OF THEM DID NOT BITE, AND THE REASON IS WORTH KEEPING. The first run of the
mutations came back with two green:
setContainer rebuilding from OutputFormat.MP4_H265.spec -> GREEN
setQuality also resetting enginePreference to AUTO -> GREEN
Both for one mistake of mine: I asserted "the rest survived" against values that
were still at their defaults. `ConversionSettings` starts at `MP4_H265.spec`,
`QualityTier.FAST` and `EnginePreference.AUTO` -- so a mutation that RESET a
neighbouring field to its default was indistinguishable from one that left it
alone. The tests were checking a value, not a behaviour.
Fixed by moving each neighbour off its default before the call under test. A
third test had the same latent hazard -- it asserted `quality == FAST` -- and was
corrected with the others rather than left to fail later.
That is precisely the shape CLAUDE.md warns about ("five of them passing the
whole suite over a completely unguarded code path"), and it is the second time in
this wave the mutation pass has earned its place: green was not evidence.
Eight mutations, all red after the fix:
setContainer rebuilds from a preset | 1 test
setQuality resets the engine preference | 1 test
setEnginePreference resets the quality | 2 tests
applySuggestion resets quality | 1 test
setVideoCodec writes nothing | 1 test
setAudioCodec writes nothing | 2 tests
setEnginePreference writes nothing | 2 tests
cancel() dereferences a null activeWorkId | 1 test
516 -> 525 tests, 87.7% -> 88.0% line, 70.4% -> 70.5% branch. Gate green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-dev
|
1ff5c4463c | Merge remote-tracking branch 'origin/main' into test/join-state-mapping | ||
|
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-devandClaude Opus 5
|
2d4898ad44 |
Re-measure the coverage entry against the tree this branch creates
84.9% line / 63.8% branch, 454 tests -> 87.1% line (2025/2324), 69.1% branch (974/1410), 502 tests in 71 classes, as #132 and #133's ten children land. The entry already instructs re-measuring before quoting, and that is why this is here rather than in the batch: quoting these numbers before the work merged would have described a tree that did not exist. It nearly went wrong the other way too -- the first measurement for this commit was taken against a main that was three merges stale and read 85.1%. Also says something the bare numbers do not. Branch moved 5.3 points against line's 2.2, and that asymmetry is the expected shape of this kind of work rather than a curiosity: those children targeted decision code -- enum fallbacks, refusal arms, cursor shapes, a `when` over container rules -- where one test chooses a branch the suite had never taken. Line coverage barely notices that. Branch coverage is the whole point. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
83ac7eff2c | Merge commit '79cca0e' into fix/restore-stack-merges | ||
|
JMR-devandClaude Opus 5
|
3a5210ec5d |
S2 + S3 (#142, #143): the two OutputPublisher seams, and where the second one goes
#142 -- openOutputStream refuses two ways and only one was reachable. A provider that has gone away throws from inside the call, which `a destination the provider will not open...` already drives. A provider that is present and declines returns null, and nothing could produce that on demand. openDestination is the seam; the test asserts the failure names the destination, which is what separates the `?: error(...)` from an NPE inside `use`. #143 -- the sweep's re-read. **The seam the ticket proposed does not reach it.** Overriding the listing fires before the entries are snapshotted, so StagingSweep.collectable is handed the new timestamp, the file is never proposed for deletion, and the guard is never exercised. Measured: with an entriesIn seam, deleting the guard outright left the test green. The race is a file that *was* collectable when the snapshot was taken and is not by the time the delete comes round, so the seam has to sit at the snapshot. `snapshot(listing)` does, and deleting the guard now reddens the test. Three mutations after the move, three red: null stream returns silently null-return test null stream via !! instead null-return test sweep deletes unconditionally race test OutputPublisher.kt now has no never-executed lines at all. Two partial branches are left and both are named exemptions rather than gaps: L216's `getOrNull() ?: false` and L304's `getOrDefault(absoluteFile)` are the failure arms of a runCatching whose body cannot be made to throw through any public entry point -- the same shape as the `size >= 0` exemption recorded in the previous commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
79cca0eb47 | Merge branch 'test/refused-jobs' into test/mediaprobe-track-seam | ||
|
JMR-dev
|
2c0bc4a583 | Merge branch 'test/concatworker-failure-arms' into test/refused-jobs | ||
|
JMR-dev
|
7a47285f37 | Merge branch 'test/container-capabilities-audio' into test/concatworker-failure-arms | ||
|
JMR-dev
|
8a2bc86cac | Merge branch 'test/readspec-enum-fallbacks' into test/container-capabilities-audio | ||
|
JMR-dev
|
9b3b9f952b | Merge remote-tracking branch 'origin/test/outputpublisher-seams' into test/readspec-enum-fallbacks | ||
|
JMR-dev
|
a84b24ba27 | Merge branch 'test/refused-jobs' into test/mediaprobe-track-seam | ||
|
JMR-dev
|
0e2525195b | Merge branch 'test/concatworker-failure-arms' into test/refused-jobs | ||
|
JMR-dev
|
713d813a65 | Merge branch 'test/container-capabilities-audio' into test/concatworker-failure-arms | ||
|
JMR-dev
|
c360e82a10 | Merge branch 'test/readspec-enum-fallbacks' into test/container-capabilities-audio | ||
|
JMR-dev
|
699d608b47 | Merge remote-tracking branch 'origin/main' into test/readspec-enum-fallbacks | ||
|
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
|
c60d5d54c6 |
Stop the staging fixture losing a race with the app-start sweep (#159)
`the sweep tolerates a staging path that is not a directory` failed once on run
33069641674, against 468 tests that pass on this machine including under
`--rerun-tasks`:
java.io.FileNotFoundException at OutputPublisherStagingTest.kt:112
468 tests completed, 1 failed
Line 112 was `writeBytes` immediately after `deleteRecursively()`.
`FileOutputStream` answers `FileNotFoundException` for an existing directory, so
something had recreated the path inside that window. That something is
`LibreMediaConverterApp.onCreate`, which ends with
appScope.launch { OutputPublisher(...).sweepStaging() }
on `Dispatchers.IO`, and `sweepStaging` reads `stagingDir`, whose getter calls
`mkdirs()`. Robolectric builds the application for every test that asks for one,
so that background `mkdirs()` is in flight across the whole suite on a thread the
paused main looper does not control and no test awaits.
Retrying closes the window rather than narrowing it, because the race is not
symmetric: `mkdirs()` fails on an existing regular file, so the invariant only has
to survive being *established*. Once a write lands, nothing in the suite can turn
this path back into a directory -- which is also why the new assertion that the
sweep left a file behind is worth making.
The `check()` matters as much as the loop. The next failure here should say
"something recreated conversions/ as a directory", not `FileNotFoundException at
line 112` -- that is the difference between a flake someone reads and a flake
someone re-runs.
The wider problem is #159 and is deliberately not fixed here: `AppStartSweepTest`,
`JobSnapshotsTest` and `SpaceArithmeticTest` all name the same path, and the real
answer is an injectable scope rather than a retry loop in every staging test.
#159's done-when is that this loop can be deleted.
Mutation: `listFiles() ?: return` -> `listFiles()!!` reddens exactly this test.
Gate green: assembleDebug, testDebugUnitTest, compileDebugAndroidTestKotlin,
ktlintCheck, detekt, lintDebug.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
d59e9acce5 |
C6 (#140): OutputPublisher's guarded branches, three of them guarding a delete
destinationIsKnownEmpty's three short-circuits -- no SIZE column, no row, a null cell -- each had to answer false and none was tested. Its KDoc is unambiguous about why: "this decides whether a delete is allowed and 'I could not tell' must never authorise one." The existing tests only ever drove a provider that answers properly, where the answer is zero and the delete is correct. Getting the uncertain cases backwards costs the user a file they already had, on a save that failed. Also discardStaged's null parentFile, and sweepStaging's null listing -- which is not the case the existing `tolerates a staging directory that does not exist yet` covers, because stagingDir's own mkdirs() recreates a missing directory and it then lists as empty. Only a path that cannot be a directory makes listFiles() answer null. Five mutations, three bite: drop !row.isNull(size) short-circuit test red drop row.moveToFirst() five tests red parentFile!! instead of ?: return false parentless test red size >= 0 -> size >= -1 GREEN, does not bite parentless treated as staged GREEN -- bad mutation, see below The first green one is recorded in the test as a named exemption. Measured: getColumnIndex returns -1 for an absent column and isNull(-1) throws CursorIndexOutOfBoundsException, which the surrounding runCatching already turns into `?: false`. Same answer, reached by the exception path, so no behavioural test can pin that conjunct. It stays anyway -- control flow through an exception is worse than a comparison, and another Cursor implementation need not throw. The second was my mistake rather than a finding: substituting stagingDir for the null parent reaches `return false` by a different route, so it proves nothing. parentFile!! is the honest mutation and it goes red. :197, :235 and :258 are now covered. What is left in this file is exactly what the ticket scoped out: :173-174 (#142) and :267 (#143). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
8a88fc4ae7 |
C3 (#137): pin what InputQuery makes of a metadata row
Nothing had ever handed InputQuery a cursor row. UnknownInputSizeTest
drives the no-provider case thoroughly -- query returns null, measure()
answers -- so firstRow's body, displayNameOrNull and sizeOrNull had never
executed at all.
Nine tests over FakeSafProvider's RowShape states. What they pin is not
"reads a cursor" but the rule the class exists for: a size nobody could
determine must arrive as null, never 0. Four separate ways a provider
fails to give one -- a null cell, a missing column, a negative value, an
empty cursor -- plus a provider that throws outright, which is the guard
firstRow's KDoc is written for.
Mutations run, all four bite:
drop `takeIf { it >= 0 }` from sizeOrNull -> negative-size test red
drop `!isNull(it)` from sizeOrNull -> null-size test red
drop the runCatching in firstRow -> throwing-provider test red
drop `!isNull(it)` from displayNameOrNull -> GREEN, does not bite
That last one is recorded in the test's KDoc as a named exemption rather
than papered over. Measured: MatrixCursor.getString on a null cell returns
null while getLong returns 0. So the guard is load-bearing on the size path
-- it is what stops a null becoming a real number -- and unfalsifiable on
the name path, where getString already yields null. It stays regardless:
Cursor.getString's contract makes throwing on null implementation-defined,
and a real provider may do what MatrixCursor does not.
InputQuery.kt now has no never-executed lines. Suite 456 -> 465 tests,
branch coverage 63.8% -> 65.6%.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
44d4c61738 |
C0 (#134): move the fake providers to scaffolding, let them answer wrongly
Two of #132's items are cursor-shaped -- InputQuery's row reads (#137) and OutputPublisher.destinationIsKnownEmpty's short-circuits (#140) -- and the provider that could drive them lived inside OutputPublisherPublishTest and could only answer correctly. Its row was always (file.name, file.length()). Moved FakeSafProvider, FakePlainProvider and the registration helper to FakeProviders.kt, same package, following StagingCleanupSupport.kt and ParkedPickDispatcher.kt. UnreliableOutputStream stays behind: it serves one test, which is the line WorkerStubs.kt draws. Added RowShape, seven ways a provider can answer a metadata query. Column granularity is deliberate -- OutputPublisher reads only SIZE, InputQuery reads both and reaches different answers depending on which is bad -- and so is keeping null, missing, negative and no-row distinct rather than folding them into one "bad" case. That distinction is the whole reason InputQuery exists: hasSpaceFor(0) is only "is there 128 MB free", so a size nobody could determine must not arrive as 0. No production change. OutputPublisherPublishTest, OutputPublisherStagingTest and UnknownInputSizeTest pass unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
44493d9943 |
C5 (#139): the join side's count refusal, found by the residual-gap audit
A gap audit over the eight branches merged together looked for lines still never executed and asked, for each, whether something already accounts for it. Everything mapped except one: `ConcatWorker.kt:42`, the refusal of a join with fewer than two inputs. Its neighbour maps. `ConcatWorker.kt:40` -- the missing-URI-array arm, two lines above -- is covered on the device by `UnopenableUriTest.aJoinWithNoInputArrayFailsWithAMessage`. That is invisible to JaCoCo, which measures `testDebugUnitTest` only, so the report shows both arms cold and cannot distinguish the one that is e2e-covered from the one nothing touches. Only reading the androidTest source separates them. `grep` says nothing in either source set mentions "Pick at least two files to join." Two tests here now do: - `a join of a single file is refused with a message rather than joined` pins the verdict and the message together, via `Failure.equals`, for the reason the file's header already gives. - `a join of two files is not refused for its count` is the control that puts the assertion on the boundary rather than on the string. It refuses the *space* rather than letting the job run: the next thing past the count guard is `ConcatEngine`, which is native, and `NamingPublisher`'s KDoc already records that no JVM test gets past it. A failure carrying the space message is proof execution reached line 57, which is proof it cleared line 42, at no engine cost. Reachability is the header's argument plus one of its own: `request(...)` takes a `List<Uri>` and checks nothing about its length, so a one-item join is a well-formed call rather than a corrupted queue entry. Mutations, each killing exactly the test it should: | mutation | red | |---|---| | guard deleted outright | `a join of a single file is refused...` | | `uris.size < 2` -> `< 3` | `a join of two files is not refused for its count` | Restored, both green. Gate green: assembleDebug, testDebugUnitTest, compileDebugAndroidTestKotlin, ktlintCheck, detekt, lintDebug. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
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-devandClaude Opus 5
|
de6d9526ba |
C5 (#139): the two jobs ConversionWorker refuses before converting
Both exits were cold, and both are reachable for the same reason: a job does not have to come from the picker. WorkManager keeps work for about a week, so a downgrade or rollback hands this build a job enqueued by another one, and request(...) is callable directly. :62 -- a missing KEY_INPUT_URI -- was untested everywhere, JVM and device. The nearest e2e test, ForcedFailureTest.aMissingInputFailsRatherThanCrashing, passes a URI pointing at a file that does not exist, which reaches the engine and fails much later with a different message. :124-126 -- the Validation.Invalid refusal -- had no test at all, though its comment names both arrival paths it exists for. Five tests, in a new file because both are about the *message*. A refusal that fails with empty output Data renders the UI's generic "Conversion failed." with nothing else to say, which is the defect shape DeniedForegroundStartTest records from the device pass; asserting the verdict alone would pass against exactly that. Two of the five are there to stop the others passing for the wrong reason: `a refused spec never reaches an engine` says it failed *before* converting rather than during, and `a valid spec is not refused` is the control -- without it every assertion here would still pass against a worker that refused everything. Three mutations, three red: change the no-input message no-input message test drop the validation refusal both refusal tests validate but keep converting both refusal tests L61-62 and L123-126 are now fully covered, branches included. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
bb3358f209 |
C4 (#138): ConcatWorker's cancellation and give-up arms
ConversionWorker has WorkerCancellationTest and DeniedForegroundStartTest. Its twin had the retry case only -- `a join whose foreground start is denied` already existed -- so two of ConcatWorker's three failure exits were cold: the CancellationException arm, and FOREGROUND_DENIED. Four tests, added to the files that own each rule rather than to a new ConcatWorker file, which is how this suite is organised: a file per rule, tested across both workers. The cancellation seam is worth a look in review. The conversion twin cancels inside the engine, which is honest there because ConversionDependencies has a seam for it. ConcatWorker calls ConcatEngine directly and has none -- it is native and nothing here gets past it -- so the cancellation is injected at the only other point inside the try, setForeground. That is a real shape rather than a contrivance: a job cancelled while WorkManager is promoting it is exactly when that window is open, and the catch arm cannot tell where in the try it came from. FailedFuture moved to WorkerStubs.kt on the way. Two tests now inject two different failures through it, and Kotlin will not take two file-private top-level classes of one name in one package. Four mutations, four red, each isolated: cancellation arm -> Result.failure propagation test only drop delete on cancellation cancellation-partial test only FOREGROUND_DENIED -> Result.retry past-the-bound test only drop delete on the Throwable path give-up-partial test only ConcatWorker's :92, :95-96 and :105-106 are covered; missed branches 4 -> 3. What is left is what the ticket scoped out: the two input guards (e2e), the ConcatEngine success path (native), and getForegroundInfo (#88's named exemption). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
04850a0415 |
C2 (#136): test the audio half of validate, and the one video refusal missing
The two halves of ContainerCapabilities.validate were written together
and only one of them was ever checked. Six audio outcomes had no test --
every one a string the user reads -- while the video twin of each was
already covered.
Seven tests, deliberately shaped like their twins rather than as a fresh
idea about what to assert:
unidentifiable source audio on a COPY twin of `an unidentifiable
source codec cannot be copied`
container cannot hold the copied source twin of `a codec the container
cannot hold is refused...`
container cannot carry it on encode twin of `H265 in AVI is refused`
this app cannot encode it twin of `copying is offered as
the fix when...`
accepts(_, AudioCodec.NONE, _) -> true twin of the VideoCodec.NONE arm
accepts(_, AudioCodec.COPY, _) throws twin of `resolving COPY before
asking the matrix is required`
The seventh is not the audio axis: validateVideo's copy-into-a-container-
that-cannot-hold-it refusal was the one video outcome with no test, and it
is the same shape and the same file.
Each asserts the message verbatim and re-validates every suggestion the
refusal offers. Validation.Invalid promises its suggestions are themselves
valid and names this class as the proof; the existing property test walks
the presets, and no preset reaches suggestions() through validateAudio.
Seven mutations run, seven red, each isolated to exactly one test:
CARRIES_AUDIO check -> false encode-path test only
drop the COPY error arm resolve-first test only
AudioCodec.NONE -> false no-audio-track test only
drop ENCODABLE_AUDIO check unencodable test only
drop audio copy container check audio-copy test only
drop video copy container check video-copy test only
drop unidentified-audio guard unidentifiable test only
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
8ab433b647 |
C1 (#135): pin readSpec's three enum fallbacks
WorkerEnumFallbackTest already existed for this defect class -- a name this build does not define, read above the try, throwing out of doWork entirely: FAILED with reschedule=false, empty output Data so the screen said "Conversion failed." with nothing else, and the staged file never deleted. It covered 2 of the 5 above-the-try reads. readSpec's three were the ones left, and all three were cold. The baseline is the part worth reviewing. readSpec returns the *entire* fallback spec the moment any one axis fails to resolve, so a test starting from MP4_H265 -- which is itself the fallback -- cannot tell a worker that read the spec correctly from one that gave up on it. These start from MKV/H.264, which differs on container and video codec at once, and assert the spec that actually reached the transcoder rather than only that a Result came back. Mutations run, four for three tests: KEY_CONTAINER `?: return fallback` -> `?: error(...)` -> container test red KEY_VIDEO_CODEC same -> video test red KEY_AUDIO_CODEC same -> audio test red fallback = MP4_H264 instead of MP4_H265 -> all three red The first three confirm the tests are isolated to their own axis; the fourth confirms they pin *which* spec ran, which is what "a Result at all" would have missed. readSpec is now fully covered, branches included. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
d9c32c6ce5 |
Add F5: areEnabled() is never called, so it is not a test gap
Found while decomposing #132 into children. It was item 6 there, and it looked like the cheapest item on the list: three cold lines, a KDoc with real user-visible stakes, and a permission Robolectric can flip in one line. grep -rn 'areEnabled' app/src returns the declaration and nothing else. Both workers construct ConversionNotifications and only ever call build(). So the behaviour the KDoc describes -- warning when progress will be invisible -- does not happen, and a test would assert that a function nobody calls returns what the platform told it. Green, vacuous, and worse than nothing, because it would imply the disabled-notification case is handled. Recorded rather than tested, and the summary now names what F1 and F5 have in common: a comment describing behaviour the code lacks, where the tempting fix freezes the wrong answer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
8a23f2a0b8 |
Correct the #122 claim this document got wrong from one green run
The ConversionForegroundType note asserted that #122's wedge no longer kills the API 33 leg, on the evidence of a single run. The PR carrying this document then wedged that exact leg: 23m08s, "wedged: yes -- gradle was killed after 1200s and never returned", failed: unknown. Corrected to what the runs actually show: intermittent, not resolved -- five of the last six completed legs passed in ~7 minutes. And the distinction the wedge row exists to draw is now stated, because it is what keeps #88's reasoning intact: received: 60 means all sixty tests still reported, so the API 33 regime was exercised; it is the failed count that reads "unknown", so the leg could not have reported a break. Also names what that changes -- a @Config(sdk = 33/34) JVM test is worth three lines as insurance against a leg that cannot be trusted to go red, which is a different and much smaller claim than the uncovered behaviour this first looked like. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
25992863e6 |
Name the ticket numbers the findings doc defers to
#132 holds the seven JVM test gaps from the same read, #133 the three seam questions. The doc drew the line between them in prose already; this makes it followable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
232cbd1949 |
Record the code findings from the 2026-08-26 coverage read
Four things came out of re-measuring coverage that a test would document rather than repair, so they go in a doc rather than a ticket: - F1 FFmpegCommandBuilder emits a Vorbis encoder ContainerCapabilities' own comment says nothing emits. Traced unreachable through four call sites, but the interesting reading is the other one: FFmpeg can encode Vorbis, WebM and OGG carry it, and the picker never offers it. - F2 ConversionRequest.hardwareEncodeAvailable is written once and read by nothing; its KDoc describes a Fast-tier preset choice that was removed, and the router computes the same answer itself. - F3 ConversionRequest.videoCodec/.audioCodec have no callers anywhere. Named as NOT a test gap: asserting a delegation restates it. - F4 Two private guards reachable only by direct call. No action, per the judgement #88 reached about getForegroundInfo. Also records two things the read makes look like gaps and are not: the Compose screens' branch numbers (inflated by compiler-synthesised recomposition checks; the line figures are 34/383 and 20/143), and ConversionForegroundType, where #88's premise was re-checked against #122's wedge and holds -- the API 33 leg completes 60/60 cleanly. Entry ids are F1-F4 so they cannot be confused with defect-audit.md's D1-D16, and the confidence vocabulary is deliberately that document's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
7f2a6e1376 | Merge remote-tracking branch 'origin/main' into m-129-tmp | ||
|
JMR-devandClaude Opus 5
|
1d80e88f9b |
Ignore Gradle's .kotlin/, which every local build leaves in the repo root
It has never been committed, so nothing is wrong today -- but nothing stops it either, and `git add -A` would stage Kotlin build-session state into history. It belongs beside /build, .gradle and .cxx, which are the same category and are already here. Placed with them rather than in a section of its own, and left without a comment: unlike tools/ffmpeg/out/ and .claude/, there is no non-obvious choice here to explain. Verified rather than assumed: $ git check-ignore -v .kotlin .gitignore:16:.kotlin .kotlin Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
27d7cc0a86 | Merge remote-tracking branch 'origin/main' into m-127b-tmp | ||
|
JMR-dev
|
c6b581ab5a | Merge remote-tracking branch 'origin/main' into m-115b-tmp | ||
|
JMR-dev
|
c27881ab64 | Merge remote-tracking branch 'origin/main' into m-127-tmp | ||
|
JMR-devandClaude Opus 5
|
0f39964193 |
Say which misfire the hang watchdog actually has
The comment described adopting a later build's worker as an edge case. It is the ordinary CI shape: the worker is found by scanning this daemon's descendants for GradleWorkerMain, which cannot tell one invocation from the next, and the Unit tests job runs testDebugUnitTest and jacocoTestReport back to back against one daemon. Still harmless -- the watchdog only reads and writes -- but a reader should not have to rediscover that. Refs #125. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
71141b5734 | Merge remote-tracking branch 'origin/main' into m-126-tmp | ||
|
JMR-devandClaude Opus 5
|
81ad102f2a |
Stop a deadlocked unit-test run, and make it say what it deadlocked on
The JVM suite had no timeout of any kind, so #125's Room/WorkManager lock-order inversion ran until something outside it gave up: 47 minutes locally, and on CI it would burn the Unit tests job's 30-minute cap and report as a job timeout with no cause. The deadlock is monitor contention, which no interrupt breaks, so nothing inside the JVM could have ended it either. The obvious fix does not work here. A JUnit `Timeout` -- as a rule or as `@Test(timeout = ...)` -- runs the test body on a separate thread, and every Compose test in this source set goes through Robolectric's paused main looper. Both forms fail with "main looper can only be controlled from main thread"; the same tests with the timeout removed pass, so it is the mechanism and not the probe. So the bound comes from outside the test JVM, where it moves no threads: `timeout` on the Test tasks kills the forked worker, and a watchdog jstacks that worker two minutes earlier. The jstack is the point. Gradle's timeout on its own kills silently, a timed-out run writes no XML for the class that hung, and the JVM's own "Found one Java-level deadlock" section naming both monitors is the only reason #125 could be described at all -- so it goes to stdout as well as to a file, because the Unit tests job uploads only reports/tests/. Ten minutes is against the slowest observed passing run, not the typical one: eight CI samples of the whole invocation ranged 62-90s, so this is ~6.7x that and a third of the job cap. A timeout that fires on a healthy slow runner turns a real signal into noise. Both numbers live in a build script that nothing compiles, so HangBoundTest reads them back and the build script joins build.yml as a declared input -- without that the guard would go stale on exactly the edit it exists to catch. Refs #125. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
0f41bc3f6b |
Count the annotation, not the comment saying a test does not carry it
The advisory baseline check has announced a deviation on every PR since #113: the tree "carries 4 tests marked @FailsOnEmulatorApi37" where it carries three and FAILS_ON_EMULATOR_API37_BASELINE says three. The fourth is a KDoc in Media3EngineTest saying the opposite -- "Deliberately not `@FailsOnEmulatorApi37`: nothing here decodes or encodes" -- which the old matcher counted because it looked for the string anywhere on any line. Neither ingredient was wrong on its own, and the number is not the real damage. #83 added this check so that a new failure joining the known ones could not be invisible; a notice that is wrong every single time teaches everyone to skim past deviation notices, which is precisely the signal it was built to create. Editing the baseline to 4 would have silenced it by breaking it -- the check would then have been wrong the moment someone added or removed a real marker. Anchor the pattern at line start and require whitespace or end-of-line after the name. The second half is the part that is easy to get wrong: "only the annotation on a line of its own" also stops counting `@FailsOnEmulatorApi37 @Test`, which is legal Kotlin, and undercounting is the dangerous direction -- it hides a genuine new marker, the one thing this exists to catch. Measured against a fixture carrying every shape at once: the old matcher 5, own-line-only 2, this one 3; on the real tree 4 / 3 / 3, so the baseline is untouched. `grep -v import` goes too, since `^[[:space:]]*@` cannot match an import. The check is a pure function of the working tree, so the fixture is committed and e2e-report-shape-test.sh runs the real report against it -- inside a throwaway repo root, which the script finds from BASH_SOURCE, so no knob had to be added that could point the live count somewhere else. The fixture sits under .github/, where Gradle does not compile it and :app's ktlint and detekt do not see it; running the report against the real root with it committed still reports 3. Every other path through the report is byte-identical to the previous version on both stdout and the job summary -- passing, failing, wedged, no-run, and advisory-with-an-unreadable-baseline all diff empty -- and the two advisory legs differ only by the false line disappearing. No job's status or pass/fail rules change; the advisory leg stays continue-on-error and stays red by design. The test is deliberately not wired into CI: adding a step to Static analysis would add a new way for a gating job to go red, which #120 ruled out. shellcheck still covers the file, since that step reads `git ls-files '*.sh'`. Closes #120 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
4f1a007b58 |
Quote the settled figure, now that the work it was waiting on has landed
This PR was opened quoting 81.4%, measured on |
||
|
JMR-dev
|
bf78e06969 | Merge remote-tracking branch 'origin/main' into m-115-tmp | ||
|
JMR-dev
|
d94906ef42 | Merge remote-tracking branch 'origin/main' into m-117b-tmp | ||
|
JMR-dev
|
b93ef79931 | Merge remote-tracking branch 'origin/main' into m-117-tmp | ||
|
JMR-dev
|
d739b425c0 | Merge remote-tracking branch 'origin/main' into m-121-tmp | ||
|
JMR-dev
|
febd141bea | Merge remote-tracking branch 'origin/main' into merge-124-tmp | ||
|
JMR-dev
|
3d8b89bfab | Merge remote-tracking branch 'origin/main' into merge-121-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
|
25aac95db9 |
Say in the run-shape table when the wedge timeout was what killed the leg
The report added by #111 runs on every path out of e2e-run.sh, including the wedge, and until now it answered a question it had not been asked. On job 98035980326 -- API 34, a docs-only PR -- it printed `received: 59` and `completed cleanly: yes` six seconds before `##[warning] ... WEDGED`, for a leg the WEDGE_TIMEOUT had killed 22 minutes in. `completed cleanly` means only "instrumentation was not aborted", which was true; a reader scanning the table had to notice a separate warning line to learn the leg had died. The wedge cannot be read out of the log, which is why it is passed in: a wedge is gradle never returning, so gradle printed no verdict, no truncation line and no INSTRUMENTATION_ABORTED, and the log it leaves is the log of a run that just stops. Only e2e-run.sh saw `timeout` exit 124. It now derives that fact once and tells the report as E2E_WEDGED_AFTER, and reuses the same variable for capture_wedge so the two cannot drift. The table gains a `wedged:` row above `completed cleanly`, and `completed cleanly` flips to no -- but only where it would have said yes. An abort already says no and names the abort, which the wedge row does not, and a run that left no evidence still says unknown; a wedge on top of either prints both facts. `received`'s source line told the same lie in the same table -- "the run was not truncated, so every expected test reported" is only "gradle never got as far as saying so" when the leg was killed -- so it is qualified on that path. The number itself is unchanged, and so is `failed: unknown`: gradle printed no summary line, so that count genuinely is not knowable. Nothing here decides anything. No exit status, no pass/fail rule, no baseline comparison and no `::notice::` behaviour changes; the leg already failed correctly and still does. Verified against captured CI output rather than a live emulator, as #111 was and for the same reason -- this host cannot run API 37 and cannot wedge on demand. Four real logs (the wedged leg, a green API 34 leg, a failing gating leg, and an advisory leg with its baseline deviation) through both versions of the script, in both env states, comparing stdout and the job summary: only the wedged run with the signal set differs, byte for byte. 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> |