fix/102-picker-back-press-overshoot
100
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  JMR-devandClaude Opus 5
|
842965a479 |
Say who closed the picker, because it was us and the KDoc denied it
The commit below describes "a back press aimed at a picker that had closed five seconds earlier". The timing is right and the agency is wrong, and the agency is the interesting half. Re-read out of the same logcat: 20:59:35.686 UiDevice: Retrieving node ... [RES='android:id/button1']. 20:59:35.689 UiObject2: Clicking on (927, 2274). 20:59:36.033 MainActivity RESUMED 20:59:36.350 VRI[PickActivity]: visibilityChanged ... newVisibility=false `aerr_wait` and `aerr_close` both missed on that iteration and `dismissASystemErrorDialog` fell through to `android:id/button1` -- the framework's generic AlertDialog positive button, which is on every AlertDialog on the device. It hit one inside DocumentsUI, and that is what closed the picker. The picker did not close on its own; this class closed it. So the destroyed Activity and #271 are one incident rather than two findings that happened to share a trace, and the loop's shape is three iterations rather than two: iteration 2 closes the picker through `button1` and then presses back into an app that is already in front, iteration 3 dismisses the launcher's ANR dialog and presses again, and that press finishes MainActivity. It also sharpens what the fix does. With the re-read, iteration 2 returns -- the app is focused within a second of the `button1` click -- so neither of the two presses that followed it happens at all. The previous message implied the fix caught only the last one. `requireAReadableScreen` now drops a Boolean return value, which this codebase treats as a smell. It is correct there -- the `device.wait` on the next line is the re-probe -- and the call site says so rather than leaving a reader to work out whether it was an oversight. No behaviour change beyond the comment: the fix itself is unchanged and the sweep is re-run because `app/src` is touched. Refs #102, #271 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
89832563e6 |
Stop the picker dismissal destroying MainActivity, and census what turns legs red
`dismissThePicker` guarded its back presses on `Activity.hasWindowFocus`, which a system app-error dialog makes false as well -- it is a fullscreen `system_server` window, which is why `dismissASystemErrorDialog` exists at all. So the guard could not tell "the picker is still up" from "a dialog is on top of an app that is already in front", and the loop dismissed the dialog and then pressed back on the reading it had taken before doing so. Measured on the API 35 gating leg of run 34161043035 attempt 1, whose head is #269's own commit: the save picker returns at 20:59:36.033, the launcher's ANR dialog is dismissed at 20:59:41.169, a back press goes out at 20:59:41.713, the launcher is moved to the front 41 ms later, and MainActivity is DESTROYED at 20:59:42.278. Everything after that in the test throws `NullPointerException: Cannot run onActivity since Activity has been destroyed already`. The fix re-reads the focus after a dialog is actually dismissed, and only then -- so it removes a back press sent on a stale reading rather than retrying one. A picker genuinely in front still leaves the app unfocused and still gets the press, so nothing about what this class can catch changes; and on the ordinary path, with no dialog, nothing is re-read and nothing is waited on. `requireAReadableScreen` twenty lines away has always re-probed after dismissing a dialog; this is the same rule in the one place that did not follow it. It cannot be demonstrated by re-running and the KDoc says so: the launcher ANR is ambient on these runners and is not reproducible on demand, so a green sweep is not evidence for this. The trace is. `docs/ci-failure-modes.md` is the rest of it -- a census of every gating E2E leg-attempt in the repo's history, 129 failures in 1489, classified by mode with a disposition each. It closes out #102, whose own mode turns out to be the emulator's Codec2 HAL segfaulting: a null dereference in `getClientUsage` inside `libcodec2_goldfish_common.so` kills `c2.goldfish.h264.decoder`, and Media3's 25 s export watchdog then aborts the export. Six occurrences, six carrying that crash in the same job's log, 0.5% of API 33-36 leg-attempts, and the same vendor HAL `@FailsOnEmulatorApi37` already names. Three counting rules are in the document and in CLAUDE.md because each was learned by getting it wrong: count per leg-attempt rather than per run, since a re-run to green replaces the conclusion; give every mode its own denominator, since the API 37 row filters seven tests out and some tests are younger than the window; and capture the log before retrying, since `gh run view --job <id> --log` resolves by run and serves the latest attempt. Refs #102 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
b0b8b66d31 |
Name reattach's third outcome, which this KDoc denied existed
The determinism argument said no coroutine had a `_state` write left in flight, because `reattach` "has either returned on its `_state.value !is Idle` guard or found nothing". There is a third outcome: `pruneWork()` is async, so `reattach` can find an unpruned job, pass that guard, and start an `observe()` that is a live coroutine with writes ahead of it. The conclusion survives, by a mechanism the paragraph did not mention. `reattach` reads `ownership.current` before its query and hands that token to `observe`, while `onInputPicked` calls `ownership.claim()` synchronously on the pick -- so once a detail row exists that observation is superseded and every emission returns at `stillHeldBy` before it writes. The claim is therefore "every write in flight is landed or superseded", not "no other coroutine started". This is the KDoc a future reader opens to learn why the test cannot flake, and CLAUDE.md records the same failure mode twice already -- E1/E3, and #226's KDoc that described a draft rather than the code. A correct test with an incomplete explanation is its own defect. Comment only; no test logic changed. Committed with --no-verify on the repo owner's explicit say-so: the gate's cache is keyed on the `app/src` tree hash, so a comment costs a full 33-36 sweep, and CI is already green on the parent commit. ktlintCheck, detekt and compileDebugAndroidTestKotlin were run by hand first and pass -- those are what a comment edit can actually break. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
9fd96d08fd |
Synchronise SafPickerRoundTripTest on state, not on timing (#268)
Its two picker tests failed on roughly half of gating runs, by two measured mechanisms. Both are removed here rather than re-tuned; the fix is in the test. **A -- the Convert tap was lost in the post-probe relayout.** `ConversionViewModel.onInputPicked` writes `_state` twice: name and size first, then the probe. The second write grows the file card and moves the Convert button. Compose computes the tap's coordinate from the semantics node and dispatches afterwards, so a relayout in that gap hit-tests a stationary coordinate against the new layout and the touch lands on whatever moved into the button's place -- silently. Measured as the gap between the pick's FFprobe closing and the tap: 319 ms and 421 ms passed; 46 ms, 98 ms and 124 ms did not. `convertToTheDefaultFormat` now waits for the `Container` detail row before tapping. That row is composed only under `input.probe != null`, so its presence means both of `onInputPicked`'s writes have landed and been laid out -- and nothing else in the ViewModel has a `_state` write in flight at that moment (`reattach` returned on its non-Idle guard, `observe` starts inside `convert()`). The card cannot change height again before the tap. That is a different claim from waiting longer. **B -- the app was not the focused window when Compose was queried.** One failure had a 416 ms gap, so it was not A: the tap landed, `GrantPermissionsActivity` started, back was pressed, and nothing was ever enqueued. A back press goes to whichever window holds *input* focus, while `Until.hasObject` answers about the accessibility tree -- which can carry the dialog's nodes first -- so a back that arrives one window early lands on `MainActivity` and finishes it. `POST_NOTIFICATIONS` is now held before the tap instead of the dialog being dismissed after it. `RequestPermission.getSynchronousResult` returns without starting anything when the permission is already granted, so there is no foreign window, no back press, and nothing the test injects can finish the Activity. `@Before` asserts the grant rather than assuming it. The class KDoc claimed granting "was tried first and did not take". Re-measured at API 34, six consecutive runs: zero `REQUEST_PERMISSIONS` starts, zero `GrantPermissionsActivity`, and exactly two `Scheduling work ID` lines per run -- one per converting test, so neither tap was lost. Also adds a fail-fast that says the Convert tap started nothing, instead of spending the 300 s conversion budget and then naming `action.saveFile`. It is a diagnostic, explicitly not the synchronisation. Not fixed in production. The double write is deliberate, documented progressive disclosure -- blocking the screen on an FFprobe process spawn reads as the app ignoring the tap -- and a layout fix (pinning the button, reserving the card's height) would make A less likely for one widget where waiting on the probe makes it impossible for every tap. The ticket's argument that each added `OutputFormat` widens A does not hold either: the format `FlowRow`'s height is fixed for a given entry list and does not change when the probe lands. What displaces Convert is the card growing, independent of chip count. Mutation, run not predicted: deleting `publish`'s `if (destinationWasEmpty) deletePartialOutput(...)` arm reddens `aFailedSaveDeletesTheDocumentItCouldNotWrite` with "publish did not delete the document it could not write", and reddens nothing else -- its sibling stays green, since the success path never enters that catch. Counts re-derived and unchanged: 72 androidTest tests, 7 markers, 65 gating, FAILS_ON_EMULATOR_API37_BASELINE = 7. Both tests keep @FailsOnEmulatorApi37. `NotificationCancelActionTest`'s KDoc said the suite grants no runtime permissions; that is no longer true and it now says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
a4ca93b00e |
Say what the artefact check measured, not what it implies
Two claims in E9 went further than the evidence, in an entry whose whole subject is a figure that was quoted past its own. "All 16 sat on that one line" was consistent with what was measured, not established by it: `MediaProbe:321` accounts for a 16-branch difference and the denominators now agree at 1338, but the other lines were never enumerated in both reports, so a line that lost coverage elsewhere would have been invisible to the check that was run. Says that now. And `matroskaOrWebm` is not "the only string-literal `when` in that file" -- `shortName` at :429 is a second one. The parenthetical was never checked; it is gone rather than repaired, since the mechanism was not what the measurement established anyway. The previous commit message carries the stronger wording. It is left alone rather than rewritten, because that would need a force push. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
aaa1b64f87 |
Read the union's branch tier, and decide 12 of its 22 sites (E9)
E8 classified the 32 lines neither suite executes and stopped there. It never asked which *arms* neither suite takes on lines both suites run, and that tier is where what is left actually lives. Rebuilt the union rather than reusing E8's artifact: a fresh jacocoTestReport merged with E8's own API 34 .ec against one set of current class files. Union is 99.0% line, 90.1% branch. Controls confirm the device half applied -- 32 lines in FFmpegEngine, 24 in Media3Engine, 15 in ConcatEngine, 9 in MainActivity that the JVM suite never reaches. Three things came out of it that are worth more than the count: - **E8's denominator artefact is retired.** It warned the union's branch denominator ran 16 ahead "entirely inside MediaProbe" and told readers not to quote a MediaProbe branch figure raw. Rebuilt, both denominators are 1338 and MediaProbe:321 reads mb=0 cb=4. All 16 sat on that one line, so it was a property of how the report was built and never of the code. - **CLAUDE.md's "18" is not stale.** It reproduces exactly under `mi == 0 && mb > 0` (19 branches on 18 lines) and not under `ci > 0`, which admits partially-executed signature lines and gives 139. Different metrics, not drift -- stated in E9 so the next read does not "correct" a figure that is right. - **Tier 1 is 22, down from 32.** #252 closed the ten getForegroundInfo lines and added none. Two classes carry a one-line blind spot because #252 changed their bytecode and JaCoCo rightly rejected E8's .ec for them; the bound is measured, not assumed, and the line is not a gap. Of the 22 branch sites, 12 are decided here -- compiler codegen, an F4/F6/F10 exemption already on record, or a thread race -- and recorded in E9 rather than as new F-entries, since coverage-read-findings.md is being rewritten by #261. The other 10 are filed as #262-#266, each naming the mutation that must go red or saying that the read is the ticket. FFmpegCommandBuilder:183's missed arm is VORBIS, which #261 is closing as it lands, so the set is 21 the day it merges. E9 says to re-derive rather than edit that sentence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
e6ac84cd24 | Merge remote-tracking branch 'origin/main' into feat/ogg-vorbis-libvorbis | ||
|
JMR-devandClaude Opus 5
|
d45abe7409 |
Rebuild the FFmpeg AAR with libvorbis, and make Ogg Vorbis reachable (#254)
`FFmpegCommandBuilder` has emitted `-c:a libvorbis` since the day it was written, and libvorbis was not in the AAR this app ships: the configure line omitted `--enable-libvorbis`, and `strings` on both ABIs' `libavcodec.so` named every other external encoder and not that one. The arm was unreachable from both ends, so nobody ever hit it -- but the first user to pick Ogg Vorbis would have got `Unknown encoder 'libvorbis'`. That is #238's shape again: two individually-correct facts, a builder arm and a configure line, that no test put together, and that no coverage number can see. So the binary is rebuilt rather than the arm rewritten. FFmpeg's in-tree `vorbis` encoder was already in there and was tried first; it is experimental, stereo-only, and its quality knob spans 2x its floor against libvorbis's 6x. Shipping it would have meant `-strict experimental`, a forced `-ac 2` that silently upmixes every mono source, and a slider with nowhere to go. What ships instead is the arm as originally written, `-c:a libvorbis -q:a 5`, with `OGG_VORBIS` added to the presets, `VORBIS` added to `ENCODABLE_AUDIO`, and Ogg's per-codec extension fixed so a Vorbis file is not named `.opus`. The flag is `--enable-libvorbis`, read out of ffmpeg-kit's `get_library_name()` rather than guessed: the `--enable-lame` / `--enable-opus` rule predicts `--enable-vorbis`, and that is not it. An unrecognised `--enable-*` is ignored silently, so the artifact was checked before `bin/README.md` was touched -- `libvorbis` present in both ABIs, the configure line otherwise identical, FFmpeg still n8.1.2, 10 shared libraries per ABI, every LOAD still `0x4000`. Both mutations were run on API 34 rather than predicted. Pointing the arm at `libopus` reddens the e2e test with `expected:<[audio/vorbis]> but was:<[audio/opus]>` while its `OggS` assertion still passes, which is why the track MIME is asserted and the container magic is not enough. Adding `-ac 2` back reddens it with `expected:<[1]> but was:<[2]>`: this class's own fixture is mono, so mono staying mono is an assertion rather than a claim. The unit test's load-bearing assertion inverts with this change and is rewritten to say so. It used to assert that `libvorbis` was *absent*; it now asserts the encoder name plus the two flags that must not be there. Nothing on the JVM can tell a real encoder name from a fictional one -- which is exactly how this survived four coverage waves -- so the e2e test is the only thing that proves the positive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
7d1d3191a9 |
Resolve the gate's cache dir with --git-common-dir, and say when it cannot (#258)
CACHE_DIR was the literal ".git/lmc-verify". In a linked worktree `.git` is a FILE containing `gitdir: ...`, so `mkdir -p .git/lmc-verify` fails with "Not a directory" -- and because the write is the last thing the script does, it failed while the gate still printed green and exited 0. Every commit and push from a worktree then re-swept API 33-36 for nothing, silently. That is the worst shape a cache can fail in: invisible and expensive, and it was found by an agent paying for it four times over rather than by the tool saying anything. Measured both ways: in a worktree the old expression gives `mkdir: cannot create directory '.git': Not a directory`, exit 1; `git rev-parse --git-common-dir` gives the real path and exit 0. --git-common-dir rather than --git-dir so the cache is SHARED between worktrees. The key is the app/src tree hash, and identical content is identical content whichever worktree produced it -- a sweep run in one is evidence for all of them. The write also stops being silent. record_sweep() prints when it cannot record, because a cache that never fills looks exactly like one that is working. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
6a8cc01862 |
Expedite user-initiated work, and give both getForegroundInfo overrides a caller (#252)
`ConversionWorker.request` and `ConcatWorker.request` now carry `setExpedited(RUN_AS_NON_EXPEDITED_WORK_REQUEST)`. Conversions and joins are started by a tap; the jobs that have to go back through JobScheduler because no process is left to start them should not queue behind a background chore. The class KDoc that said expedited was "deliberately not used" is replaced with what was actually read out of work-runtime 2.11.2: retries are never expedited (`SystemJobInfoConverter:135`), and a job the system stops mid-run is resolved as `ResetWorkerStatus` and re-enqueued rather than answered by `FailureOutcome`. #252's own premise does not survive measurement, and that is the second half of this change. `getForegroundInfo()` is WorkManager's expedited-work hook, but `WorkForeground.kt:38` opens the library's only caller with `if (!spec.expedited || Build.VERSION.SDK_INT >= 31) return`, and minSdk is 33 -- so `setExpedited` alone leaves both overrides exactly as cold as the first instrumented coverage read found them. Measured on API 34 rather than argued: with the flag set and `doWork` still building its own notification, both methods report `missed 1 / covered 0` and all ten lines `ci=0`, and the whole instrumented suite is green anyway at 71/71. What makes them live is that each worker held two definitions of one notification. `doWork` now posts the override's instead of an identical copy, so `ConcatWorker`'s countless "Joining files" -- which nothing executed and which was therefore free to drift from the "Joining N files" that ran -- is gone. After: both `getForegroundInfo` report `LINE 0 missed / 5 covered`. Five mutations were run and all five went red: dropping `setExpedited` from either request, hard-coding the conversion title, moving its `percent` off zero, and dropping the join's input count. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
f4174e5b06 |
Run actionlint in the gate too, at CI's exact pin
The other half of the hole the previous commit closed. `git ls-files '*.sh'` does not match workflow `run:` blocks, and a good deal of this repo's bash lives there -- so a workflow edit was still the case where the gate passed and CI's Static analysis leg went red. Pinned by digest, read out of status_check.yml rather than copied, for the reason the shellcheck section gives and for actionlint's own: its documented install is `curl | bash` off a moving branch, which does not belong in a repo that pins every action by SHA. The container runtime detection and the SELinux `:z` mount option are hoisted out of the shellcheck branch so both checks share one answer rather than deciding it twice and drifting. Verified that it bites rather than assumed: status_check.yml was given a `needs: [a-job-that-does-not-exist]`, and the real pre-commit hook blocked with actionlint's own message -- `job "static-analysis" needs job "a-job-that-does-not-exist" which does not exist in this workflow [job-needs]`. Workflow restored; nothing but the hook is in this diff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
dd01f9f27c |
Run shellcheck in the local gate, at CI's exact pin
The gate checked ktlint, detekt and Android lint but not shellcheck, so a new or edited .sh file was precisely the case where the hook passed and CI's Static analysis leg still went red. The first file it could not check was itself, and it was caught by hand twice before it was caught here. THE DIGEST IS READ OUT OF status_check.yml RATHER THAN COPIED. shellcheck 0.9.0 and 0.11.0 disagree about how to report a trap handler -- SC2317 on seven body lines against SC2329 once on the declaration, same script, same directive, one red and one green. That is why CI pins by digest, and it is also why a second copy of the digest in this file would be worse than none: when it drifts, the symptom is the gate passing and CI failing, which is the exact failure this section prevents. Runs over `git ls-files '*.sh'` -- all tracked files, not the diff -- because that is what CI does, and the job here is to predict that leg rather than audit the change. podman is preferred over docker for the mount's SELinux relabel; neither present, or the digest unreadable, reports the check as NOT COVERED rather than skipping it quietly. Verified that it bites rather than assumed: a probe script whose only fault was an unquoted `ls $foo` was staged, and the real pre-commit hook blocked on SC2086 before it reached the JVM gate. Probe removed; all tracked .sh are clean under the pinned digest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
8105291f6a |
Make the gate name the levels it ran instead of claiming all of them
The closing line was `green at every supported API level`, printed on both paths -- including the one that had just said `NOT COVERED LOCALLY: API 37` two lines above. A false claim, printed by the tool whose entire purpose is to stop false claims reaching CI, on its first run. It now names them: `green on API 33, 34, 35, 36` when the Pixel is absent, and `green on API 33, 34, 35, 36, 37` when it is attached and passed. Nothing else changes. The app/src subtree is untouched, so this exercises the cache scoping from the previous commit: the sweep is skipped as already green and only the JVM gate runs -- which is the whole reason that key was moved off the repo tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
68bd24e54a |
Gate commits and pushes on a local sweep at every supported API level
New rule, and a hook rather than a habit. Source work needs the unit tests and the instrumented tests green at every supported API level before it is committed or pushed; test work needs the whole suite green at every level. tools/git-hooks/local-gate.sh is wired in as pre-commit and pre-push (symlinks, so shellcheck sees one file), enabled with `git config core.hooksPath tools/git-hooks`. WHAT "EVERY LEVEL" CAN MEAN HERE, measured rather than assumed. 33-36 run the whole suite on emulators. API 37 CANNOT be run on an emulator on this host at all -- not "is red", cannot run: the image logs `3 new surfaceflinger aborts in 45 s (want 0)` and the APK install then fails with `Can't find service: package`, because the framework is gone before Gradle installs anything. Starting 0 tests. So 37 runs on the attached Pixel 10 Pro XL when it is there, and the hook says plainly that the level is uncovered when it is not, rather than claiming five levels having run four. The first cut passed a notAnnotation filter through E2E_EXTRA_GRADLE_ARGS, which run-e2e.sh:587 overwrites with --rerun -- so that argument was discarded and would have been discarded silently. The sweep is cached under the app/src SUBTREE hash, not the whole repo tree. The first cut used the whole tree and that was wrong in a way that would teach people to resent this hook: editing a comment in CLAUDE.md discarded a sweep of byte-identical application code and re-ran forty minutes of emulators to prove nothing. Any change under app/src still invalidates it; the JVM gate always runs. There is deliberately no skip variable -- that would be --no-verify wearing a different hat. Why it is worth the time: #256 spent several gating legs learning one leg at a time what a sweep answers in one pass, and the failing leg MOVED between runs (API 35 red then green, API 34 green then red). One leg at a time reads as someone else's flake; as a sweep it is one signal. Also here, and the reason the rule arrived now: awaitNode treated "the app has no composition right now" as a failure rather than as not-yet. fetchSemanticsNodes throws IllegalStateException when nothing is attached and waitUntil propagates it on the first poll instead of waiting out the deadline. This class spends much of its time behind the picker, the save dialog and the permission dialog, so there is always a window where the app is coming back with no composition -- and on run 34057706195's API 34 leg both SAF tests died in it. Now it is not-yet, with the last composition error carried into the timeout message so a genuinely dead app stays diagnosable. Verified: this commit's own hook swept API 33, 34, 35 and 36 at 71/71 failed=0, API 34 included. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
19e35394e1 |
Bound the conversion against API 35's encode, not API 34's
The API 35 leg of #256 went red on aSaveWritesToTheDocumentTheSystemPickerCreated with a 120 s ComposeTimeoutException on action.saveFile. It was not a cancelled job and not the new teardown: run 34056545386's logcat has 20:05:26.897 FFmpegEngine: ffmpeg ... -c:v libx265 -crf 24 -preset veryfast 20:07:41.693 ConversionWorker: Routing worker_sample.mp4 ... 134.8 s between the encode starting and the next job in the suite, with no cancel between them. The conversion was healthy and still running when the bound fired. CONVERSION_TIMEOUT_MS was 120_000, and its KDoc justified that with "the whole test takes 11.8 s on the API 34 CI leg" -- a real measurement generalised to an API level it was never taken on. Adding a second picker test made this class encode twice, so the second one runs on a more contended emulator and crossed a line that was already marginal. Now 300_000, justified against the 134.8 s, with a note not to re-tighten it from a fast leg's timing. cancelAllWork was SUSPECTED of causing this and did not. A local API 35 run with it passed, which is what sent me to the logcat. pruneWork is kept because it is the narrower call -- only finished records need to go, and cancelling live work is a wider blast radius than teardown in a shared process needs -- and its KDoc now says it fixed nothing rather than claiming a cause it does not have. That correction is the point: the first version of that KDoc asserted a cause from one red CI leg and one green local run on a different machine. A test carrying a confident wrong explanation is the failure mode this whole read has been about. Verified: API 35 at 71/71 failed=0 with the raised bound, and the full gate green. Production is untouched -- git diff origin/main -- app/src/main is empty. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
cbbaf74285 |
Delete the document a failed save could not write (#250)
#226 proved D4's premise -- SAF hands back a document reporting exactly zero bytes, so destinationIsKnownEmpty can answer true -- and then drove the success path, where publish's catch is never entered. So deletePartialOutput had still never run against a real DocumentsProvider; its only assertions were OutputPublisherPublishTest's, against FakeSafProvider under Robolectric. That is the same "asserted only against a fake built to match it" shape #226 was filed to break, one layer down. RecordingPublisher.failOpen makes openDestination return null, which publish turns into error("Could not open destination for writing") AFTER its size probe has run -- so the catch is reached with destinationWasEmpty true on a document DocumentsUI created seconds earlier. Null rather than a throw because openDestination's KDoc says a provider that is present and declines is the half no fake can produce on demand, so that arm is also taken for the first time. Mutation, measured: delete the deletePartialOutput call and this test fails with "publish did not delete the document it could not write". Nothing anywhere went red for that line before. TWO DEAD ACCESSORS #226 LEFT, and the reason is the same one: FixtureDocumentsProvider is declared by the test APK and runs in org.libremediaconverter.test; instrumentation runs in the app's process. A static in the provider is a different object from the one a test can see, so deletedDocumentIds() would have read empty forever, and reset(File) deletes under a filesDir that is not the provider's. Both are removed rather than worked around. That is E7's process wall from a third side, after ACTION_OPEN_DOCUMENT and ActivityScenario. The oracle is the document instead, which crosses the boundary because the app holds a URI grant for it. Still the path rather than the artefact: the size query proves the document existed and was empty moments earlier, and one that no longer answers a query is one something deleted. CLEANUP IS IN TEARDOWN, and the mutation run is why. A failed save keeps its staged file deliberately, so this test ends with a finished job for the next launch to reattach to; its sibling then opened on Converted with no "Choose file" to tap. The first fix tapped Start over at the end of the test body, which does not run when the test fails -- so the mutation run turned one real failure into two, the second looking like an unrelated flake. One cause must produce one red test. Baseline 6 -> 7, with the derived counts in CLAUDE.md, the marker KDoc and status_check.yml moved in the same diff. 71 - 7 is 64, the same gating figure for the third consecutive time, which is how that paragraph goes stale unnoticed. Verified: three API 34 runs at 71/71 failed=0, the mutation red on the right assertion, and the full gate plus pinned actionlint green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
4de169c99b |
Record the first instrumented coverage measurement, and classify the 32 it found
E8. The instrumented suite had never been measured: enableAndroidTestCoverage was unset, so a connected run emitted no .ec at all, and jacocoTestReport reads only testDebugUnitTest. Measured on API 34 by setting the flag temporarily. JVM 2236/2374 line 94.2% 1171/1338 branch 87.5% E2E 1711/2374 line 72.1% 669/1354 branch 49.4% UNION 2342/2374 line 98.7% 1212/1354 branch 89.5% The JVM row reproduced the committed figure exactly, which is the control that says both exec sets match the current class files. The device suite closes 106 lines the JVM suite misses, and the first four are the 81 wave 4 wrote off as native or device edges -- FFmpegEngine 32, Media3Engine 24, ConcatEngine 15, MainActivity 10. The union leaves one. That confirms the read's own hypothesis rather than overturning it; nobody had measured past the boundary it named. All 32 lines reached by neither suite were read, and none is an e2e test gap: nine are compiler-generated, ten are getForegroundInfo() for expedited work this app never enqueues (#252), three are F5, three are uncalled members (#253), one is the Vorbis encode arm (#254), and four are F4-shaped error guards. MediaProbe:210-212 gained a measurement rather than an assumption. F7 ruled probeWithExtractor's catch unreachable because Robolectric's MediaExtractor never throws; probeWithFFprobe calls native ffmpeg-kit, so that reasoning does not transfer. But probe() calls both, and RemuxTest drives it with garbage bytes on a device -- so the ffprobe path has had malformed input on real hardware and did not throw. Same conclusion as F7, different mechanism, now on record. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
e81403c5f3 |
Measure the fourth claim rather than asserting it, and fix three slips
Review of the previous commit found three things of exactly the kind it corrects.
"Both of the advisory runs that exist" asserted exhaustiveness that had not been
checked -- two jobs were read, and the #248 branch had three status_check runs.
All four advisory runs at baseline 6 are now read: 34041156680, 34041593697,
34042397320 and 34043502322 each report expected: 6, received: 4, failed: 4,
and the only SAF test reporting in any of them is the rotation one. The claim
was right; the wording claimed more than the evidence.
CONVERSION_TIMEOUT_MS's KDoc said the bound is "two orders of magnitude" clear
of the real cost. 120 s against a measured 11.8 s is one.
status_check.yml dated the save test's marker to 2026-09-05.
|
||
|
JMR-devandClaude Opus 5
|
54167c052f |
Re-derive the API 37 carrier counts, and correct what #226 left behind
The 2026-09-06 re-check of the instrumented suite. Every drifted line it found came from #226, the last PR of the e2e read's own wave. The suite is 70 tests in 14 classes, 6 carrying @FailsOnEmulatorApi37, gating leg 64. The committed baseline says 6 and the advisory job agrees. Four places still said five carriers of 69: - CLAUDE.md, three sites - FailsOnEmulatorApi37.kt's KDoc - two comments in status_check.yml The gating figure is what hid it. 69 - 5 and 70 - 6 are both 64, so the one number a reader checks against a run had not moved -- which is exactly why CLAUDE.md says to derive these rather than remember them. Two KDoc claims in SafPickerRoundTripTest described a draft rather than the code. The save test says MP3 was chosen so the setup could not depend on device codecs; the code converts at the default MP4_H265/FAST, which routes on canEncode(H265). The negation of the stated reason was true. That is E1 and E3's failure mode committed by the wave that found it, so it is written down as such rather than quietly corrected. Neither picker test has ever reported on the advisory leg. The marker's KDoc said the picker test fails there behind the rotation test; with six carriers the rotation test truncates the run first, and both advisory runs since #226 -- 34042397320 and 34043502322 -- report expected: 6, received: 4, the four being the three Media3 tests plus the rotation. The save test is therefore marked by inheritance, not measurement, and both KDocs now say so. FixtureDocumentsProvider.deletedDocumentIds() has no callers: #226 proved D4's premise and drove only the success path, so deletePartialOutput against a real DocumentsProvider is still asserted nowhere. Filed as #250 with the forcing condition and the mutation; the accessor is kept with a KDoc naming that ticket rather than removed and re-added. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
3b0c262030 |
Record E7's second constraint, and that the premise held (#226)
Doing #226 turned up a second obstacle underneath E7's, with the same cause. The obvious way to avoid driving the app was a host Activity in androidTest owning its own CreateDocument launcher; it cannot be started at all, because instrumentation runs in the target app's process and the component is in the instrumentation one. That is the same fact as E7's second bullet arriving from the other side, and it leaves the app's own Save button as the only launcher available to drive. And the answer #226 was filed for: on API 34, stock DocumentsUI hands back a document URI reporting a size of exactly zero, so destinationIsKnownEmpty can return true and D4's fix is live rather than inert. A "no defect found", and not one that could have been reached by reading -- which is the argument for having done it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
b23ff0f082 |
Wait for the app to come back before asking Compose about it
The save test failed an API 35 leg with "No compose hierarchies found in the app". Dismissing the POST_NOTIFICATIONS dialog presses back and waits for the permission UI to be gone, but going away and the app being in front again are not the same moment, and the next Compose query landed in the gap. Asked of UiAutomator rather than through awaitAppFocus, which is the opposite of what this class argues for elsewhere and is right here: awaitAppFocus goes through composeRule.waitUntil, so it would raise the very error it is being used to avoid. Two more local API 34 runs at 70/0/0/3. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
fa10d94192 |
Save to a document stock DocumentsUI created (#226)
publish deletes a destination it could not write to -- D4's fix, so a failed save does not leave a truncated file at the name the user chose -- but only when that destination was positively zero bytes first. destinationIsKnownEmpty is careful that "I could not tell" never authorises a delete, which makes the precondition load-bearing. Until now that precondition was asserted only against a fake built to match it: OutputPublisherPublishTest writes ByteArray(0) into FakeSafProvider before each case, under a comment stating this is how CreateDocument behaves. If it were false in production, D4's fix would be inert and every existing test would still pass. It is not false. Measured on an API 34 emulator against the real dialog: the document SAF hands back is a document URI and reports a size of exactly zero before anything writes to it. RecordingPublisher reads both at the moment publish sees them, through the ConversionDependencies seam, then lets the real copy proceed so the bytes are checked too. This has to go through the picker, and through the app, and both are platform constraints rather than choices. E7 in docs/e2e-read-findings.md records the first: a DocumentsProvider is reachable only through a picker-issued grant. The second was measured here -- a host Activity in this source set owning its own CreateDocument launcher cannot be started at all, because instrumentation runs in the target app's process and ActivityScenario refuses with "Intent in process org.libremediaconverter resolved to different process org.libremediaconverter.test". So #226 has no cheap half, which is what its comment now says. Three things the flow needed, each measured rather than guessed: Both taps scroll first. On Ready the screen carries a file card, five pickers and then the button, so Convert is below the fold; performClick on an off-screen node dispatches where nothing is and throws nothing, while assertIsEnabled passes either way. The first version sat waiting for a Converted that could never come. The format stays at its default. FixtureDocumentsProvider advertises video/mp4 so the picker's MIME filter has a mutation with a shape, and DocumentsUI honours that on the save side too: choosing MP3 makes the destination audio/mpeg and the fixture root is filtered out of the save dialog entirely. The notification dialog is dismissed rather than pre-granted. Convert converts from the permission callback whichever way the answer goes, so denying is a real user's path and enough. Granting programmatically did not take -- GrantPermissionsActivity appeared anyway and swallowed the tap. The provider gains create, write and delete support, which it needs to be a save target at all. It carries @FailsOnEmulatorApi37 because anything that puts DocumentsUI on screen aborts system_server on that image, as #245 established for the other two; baseline 5 -> 6. Verified on a local API 34 emulator: three full-suite runs at 70/0/0/3, and a publish that writes no bytes fails it with "array lengths differed, expected.length=58677 actual.length=0". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
e7c3e5688f |
Stop the match line claiming a failure count nobody measured
This PR's own advisory leg caught it. With five markers and a truncated run it printed failed: 4 ... baseline: matches (5 expected, 5 failed) three lines apart. The match line has always printed the baseline twice, which was true while `failed` had to equal it to get there -- and the previous commit removed that requirement for truncated runs without noticing this line depended on it. So the truncated spelling says what happened: `matches (5 expected; 4 of 5 failed, on a run the abort truncated -- not compared)`. Pinned by a third case beside the two from that commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
07f7ed4259 |
Merge #221, and make the two counts derived rather than remembered
#221 landed while this was in review, adding one instrumented test: 68 -> 69, and 64 on the gating leg. Its own commit was "Move CLAUDE.md's instrumented counts with the test that changes them", and it still arrived stale -- it says three markers and 61 tests, both true of the main it was branched from and neither true of the main it merged into. That is the third time these two numbers have gone stale in a day, so the paragraph now says where they come from: a grep for @Test over app/src/androidTest minus the marker count, cross-checkable against any run's shape, since a leg below 37 reports the first as `expected` and the API 37 gating leg reports the second. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
7fd95ddede |
Merge main, and re-derive every count it moved
main landed 25 commits while this branch was open, including a third @FailsOnEmulatorApi37 on Media3EngineTest.cancellingARunningExportStopsIt and a batch of new instrumented tests. Every number this branch touches moved with them. Re-derived rather than adjusted, and cross-checked against run 34020234606: the API 34 leg (no filter) reports 68 tests and the API 37 gating leg 64, which is 68 minus main's four markers. With the picker test marked that is five markers, baseline 5, and 63 on the gating leg. The conflict in FailsOnEmulatorApi37.kt is resolved main's way: it had replaced the hardcoded "grows by two" with a reference to the constant, which is the same drift this file exists to prevent and a better fix than the number I put there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
c5b2dc0f55 |
Record what working the e2e tickets found (E7, and #238)
Two results from #223-#230 that belong with the read rather than only in their own tickets. E7 re-scoped its own ticket. #226 split into a cheap headless half and an expensive picker-driven one, on the premise that a real DocumentsProvider can be reached without DocumentsUI. It cannot: an unprotected one is refused at install, instrumentation runs in the app's uid so the test APK's own identity is no help, and adopting shell identity is denied too -- each denial naming ACTION_OPEN_DOCUMENT as the only way in. Measured three ways. So #226 is one item at the picker's cost, not two. The useful half of that distinction is that the input bridge needs no documents provider at all. getSafParameterForRead opens a descriptor through the resolver, so any readable content:// URI exercises it, which is what kept #225 headless. And that is how the read's one production defect surfaced. #238: joining files picked through the system picker failed outright on the stream-copy path, because the concat demuxer whitelists protocols separately from -safe 0 and ffkitsaf was not on the list. Only STREAM_COPY feeds the demuxer a list file, and every existing join test passed Uri.fromFile, so the one broken combination was the only one a user could reach. Worth stating plainly next to the coverage entry: it was not a missed line and not an unasserted value, but two covered things no test put together -- the gap shape a coverage number is worst at, and the reason the read happened. E4 is marked fixed; #243 made that KDoc name the constant rather than restate it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
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
|
557b3edab4 |
Compare the advisory failure count only on a run that finished
The verification dispatch of the reworked harness (34011072884) came back 4 expected / 3 received / 3 failed, where the one before it (34008889182) had been 4/4/4 on the identical configuration. Nothing about the test list changed between them: the abort landed one test earlier and the picker test never started. The baseline check would have called that "one now passes", which is the wrong reading and the kind of notice #120 is about -- a deviation that is wrong often enough to teach everyone to skim past deviation notices. So `failed` is compared only when `completed cleanly` is yes, and `expected` is compared always, because `Starting N tests` is printed before anything can abort and is what actually answers "is the marked set the size the baseline says". Two cases in e2e-report-shape-test.sh, as a pair: a truncated run short by one is not a deviation, and a CLEAN run short by one still is -- so the first cannot have bought its quiet by disabling the check. This is a consequence of adding the fourth marker rather than a pre-existing bug worth its own ticket: with three, the advisory leg had been completing. 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
|
97558c259f |
The root fix was wrong, and this is what it found: the disable does nothing
Three commits back I gave `disable_region_sampling` the `adb root` it needed, on the strength of `Must be root` appearing in every API 37 leg's log. That part was right and the conclusion drawn from it was not. api37-debug run 34010167885, with the restart finally real: pm attempt 1: Package com.android.systemui new state: disabled-user restarting the framework adbd is running as root system_server down after 2 s NOT DISABLED after the restart -- the package state did not survive three rounds of it, `final state: SystemUI STILL ENABLED`, and the leg reported `expected: 0, received: 0`. Making the restart work cost the leg every test it had. Bisected locally on android-37.0: a `stop` 2 s after `pm disable-user` kills system_server before PackageManager flushes its delayed write, and a 15 s pause makes the state survive. That repairs the wrong thing. With the package verified disabled before AND after a clean restart, `com.android.systemui` comes up 3 s after `system_server` regardless -- and CI's own logcat says the same with no restart at all: run 34006456986 verifies the package disabled at 02:29:33 and has SystemUI pid 4275 alive from 02:28:52 for the whole run. So `pm disable-user` does not stop SystemUI starting on this image, with or without a restart, and the restart is removed from all three copies rather than repaired. What is kept is the 45-second window with no new aborts, which is what was always doing the work: the boot aborts land at 02:28:18 and 02:28:43 and the wait is what puts instrumentation at 02:32:42, after them rather than inside one. `pm disable-user` is kept too, because every green leg and every number quoted about this row was measured with it applied. The prose the earlier commits got wrong is corrected in place, and one of the corrections is good news: status_check.yml's caveat that this row runs a configuration no other leg or Pixel run uses, so nothing depending on system UI may trust it, describes a state that has never existed. The row is more comparable to API 33-36 than it has been claiming, not less. 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
|
745c4f62ce |
The local runner had the same missing root, and hid it in /dev/null
Third copy of the same defect. tools/local-emulator/run-e2e.sh sends `adb shell stop` and `start` to /dev/null, so its `Must be root` was never printed and the framework restart it credits has never happened either. That matters for what docs/api-37-emulator-crash.md's abort numbers are evidence of, so the caveat goes next to them rather than in a commit message: on API 37 the image restarts its own framework every minute or so, and a restart landing after a successful `pm disable-user` brings back a SystemUI-less zygote on its own. That produces the recorded rate collapse by accident, and it is why the same code bought nothing on CI's much quieter swiftshader legs, where the logcat shows SystemUI alive for the whole run. Also verified, because the previous commit asserted it: the advisory leg really does run thePickedInputSurvivesARealRotation before the picker test -- run 34008889182 logs the four in the order Media3, Media3, rotation, picker. 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
|
e2f8ef2918 |
Cite the two measurements the last two commits asserted
The advisory-leg count (4 expected, 4 received, 4 failed with the new marker) is api37-debug run 34008889182, dispatched with the annotation as its filter. The force-stop recovery was made to go red before it was believed: on a local API 36 emulator, with the picker left open and the back presses removed, the test passes with forceStopThePicker() and fails with exactly the API 37 message without it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
393b931fff |
Give api37-debug's own SystemUI disable the same root, and say why there are two
The debug workflow's header says it does not fork e2e-run.sh, and it does not -- but it drives the SystemUI disable from its own probe step, so `disable_system_ui` can be turned off for a dispatch. That is a second copy of the same logic, and run 34008889182 showed it carrying the same defect the real leg had: `Must be root` twice, and `system_server down after 40 s` printed for a stop that did nothing. Same fix, and a header note so the next person changing one knows to change both. 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
|
d759ef32f1 |
Take the picker test off the API 37 gating leg, and give the SystemUI disable root
The gating `E2E API 37` leg failed three of the last ten status_check runs. Four runs were read logcat-first -- 34006456986, 34001744574, 34001377499 and the green 34002313300 -- and each carries exactly two `hasReadColorBufferDma` aborts before the suite (surfaceflinger, during boot and the SystemUI disable) and exactly one during it: `system_server`, thread `TaskSnapshotPer`, always inside `pickingAFileThroughTheSystemPickerFillsInTheFileCard`'s window. Nothing else in the gating set reaches the mapper. So that test kills the framework on this image whether it passes or not, and whether the leg goes red is luck: 34001377499 passed it and lost the leg anyway with `failed: 0`, 34002313300 passed it 0.6 s after the abort and went green. That is #108. The test now carries `@FailsOnEmulatorApi37` and the baseline goes 3 -> 4; the marker's own wording widens from "does not pass on this image" to "cannot be run on this image", because this carrier passes about half the time. `docs/api-37-emulator-crash.md` had counted those aborts on 2026-08-24, put them in its table, and then read the pass/fail column alone. The correction is recorded beside the original rather than replacing it. Probed on the same image and recorded there too: there is no shell knob for task snapshots -- not in `getprop`, `settings`, `device_config` or `cmd window` -- so the marker is the available answer rather than the lazy one. Two separate defects came out of the same logcats. `disable_region_sampling` has never restarted the framework on CI. `adb shell stop` and `start` are root-only and every API 37 leg has printed `Must be root` for both, so SystemUI stayed up for the whole run -- visible directly as `WindowManagerShell ... app=com.android.systemui` minutes after "final state: SystemUI disabled". Both waits also printed their own exhaustion as an elapsed time, so "system_server down after ~40 s" is what a stop that did nothing looks like. Measured on the local android-37.0 AVD, same fingerprint as CI: `adb root` makes `stop` return 0 with `pidof system_server` empty. Root is dropped again before Gradle runs, and both waits now say whether they observed anything. And when the picker test does fail, the abort is the coda rather than the cause: `InputDispatcher: No new touched window at (539.0, 525.0)` is in both reds and absent from the green, so the tap on the root is discarded, the picker is never navigated, and all four back presses land on an activity WindowManager says has not added a window yet. `forceStopThePicker` goes around input entirely so `pickTheFixture`'s whole-picker retry -- which exists for exactly this -- becomes reachable. That one is a fix on every API level, not just 37. 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
|
4d090d9a81 |
Move CLAUDE.md's instrumented counts with the test that changes them
The suite goes 60 -> 61 and the gating leg 57 -> 58, because the new join-failure test carries no @FailsOnEmulatorApi37 and so runs on every level including 37. FAILS_ON_EMULATOR_API37_BASELINE stays at 3, checked rather than assumed: e2e-report-shape.sh counts lines matching '^[[:space:]]*@FailsOnEmulatorApi37', which is three annotations -- the fourth occurrence in the tree is the KDoc at Media3EngineTest.kt:195 saying a test is deliberately NOT marked, and the anchor excludes it. Nothing here adds a marker. Carried by this PR rather than the docs one so the file never states a count main does not have. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
39327beea7 |
Assert the join failure message against a real FFmpeg session
#217 closed by noting the join legs had not been run locally. Running them would not have answered it: nothing on either source set drove a real join *failure*, so the message unified in #203 was asserted only against values a JVM test hands to sessionOutcome directly. CI had already run those legs green on the merged commit, so the outstanding item was a gap in coverage rather than a gap in execution. The new case forces a failure with an input that does not exist -- rejected the same way by every FFmpeg build, unlike malformed media -- and asserts the message names the operation, carries the return code, and has detail after it. That last clause is the device-only half. getReturnCode, getFailStackTrace and getAllLogsAsString are native reads; if the log tail came back empty on a device the user would see "Joining failed (1): " with nothing after the colon, and every JVM test would still pass. It deliberately does not pin which detail source wins. On an ordinary non-zero return code FFmpegKit reports no fail stack trace, so stack-trace-first and log-tail-first produce identical text and no assertion here could tell them apart. SessionOutcomeTest pins that, where both sources can be non-blank at once. Claiming it here would be a KDoc asserting more than the test checks. Measured on the local API 33 emulator: 61/61 green with the test, and with both detail sources nulled in ConcatEngine it fails on the intended assertion -- "the message stopped at the return code and told the user nothing, was: 'Joining failed (1): '" -- so the prefix and return code survive the mutation and only the device-only claim goes red. 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> |