ci/baseline-counter-precision
100
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 JMR-dev
|
71141b5734 | Merge remote-tracking branch 'origin/main' into m-126-tmp | ||
 JMR-devandClaude Opus 5
|
0f41bc3f6b |
Count the annotation, not the comment saying a test does not carry it
The advisory baseline check has announced a deviation on every PR since #113: the tree "carries 4 tests marked @FailsOnEmulatorApi37" where it carries three and FAILS_ON_EMULATOR_API37_BASELINE says three. The fourth is a KDoc in Media3EngineTest saying the opposite -- "Deliberately not `@FailsOnEmulatorApi37`: nothing here decodes or encodes" -- which the old matcher counted because it looked for the string anywhere on any line. Neither ingredient was wrong on its own, and the number is not the real damage. #83 added this check so that a new failure joining the known ones could not be invisible; a notice that is wrong every single time teaches everyone to skim past deviation notices, which is precisely the signal it was built to create. Editing the baseline to 4 would have silenced it by breaking it -- the check would then have been wrong the moment someone added or removed a real marker. Anchor the pattern at line start and require whitespace or end-of-line after the name. The second half is the part that is easy to get wrong: "only the annotation on a line of its own" also stops counting `@FailsOnEmulatorApi37 @Test`, which is legal Kotlin, and undercounting is the dangerous direction -- it hides a genuine new marker, the one thing this exists to catch. Measured against a fixture carrying every shape at once: the old matcher 5, own-line-only 2, this one 3; on the real tree 4 / 3 / 3, so the baseline is untouched. `grep -v import` goes too, since `^[[:space:]]*@` cannot match an import. The check is a pure function of the working tree, so the fixture is committed and e2e-report-shape-test.sh runs the real report against it -- inside a throwaway repo root, which the script finds from BASH_SOURCE, so no knob had to be added that could point the live count somewhere else. The fixture sits under .github/, where Gradle does not compile it and :app's ktlint and detekt do not see it; running the report against the real root with it committed still reports 3. Every other path through the report is byte-identical to the previous version on both stdout and the job summary -- passing, failing, wedged, no-run, and advisory-with-an-unreadable-baseline all diff empty -- and the two advisory legs differ only by the false line disappearing. No job's status or pass/fail rules change; the advisory leg stays continue-on-error and stays red by design. The test is deliberately not wired into CI: adding a step to Static analysis would add a new way for a gating job to go red, which #120 ruled out. shellcheck still covers the file, since that step reads `git ls-files '*.sh'`. Closes #120 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
d94906ef42 | Merge remote-tracking branch 'origin/main' into m-117b-tmp | ||
|
JMR-dev
|
b93ef79931 | Merge remote-tracking branch 'origin/main' into m-117-tmp | ||
|
JMR-dev
|
d739b425c0 | Merge remote-tracking branch 'origin/main' into m-121-tmp | ||
|
JMR-dev
|
febd141bea | Merge remote-tracking branch 'origin/main' into merge-124-tmp | ||
|
JMR-dev
|
3d8b89bfab | Merge remote-tracking branch 'origin/main' into merge-121-tmp | ||
|
JMR-devandClaude Opus 5
|
c4bb7d4d2d |
Quote the rate the ticket settled on, and point the save gap at its ticket
Two accuracy fixes to notes the earlier commits left behind. The test KDocs carried "roughly 1-in-130" and a 400-leg-attempt denominator. Both come from earlier comments on #49 that its own census later replaced -- that ticket has three recorded corrections to its rate claims, and a superseded figure in a permanent comment is the exact thing its author kept having to fix. What survives the corrections is the count and the spread: four occurrences, API 33, 35 and 36, every one on attempt 1 and green on re-run. The save exemption described a real defect with nowhere to look it up. It is #123 now, so the KDoc names a number instead of trailing off. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
3599307040 |
Say what the save exemption does not cover, rather than implying it is total
The note claimed `save` is left unguarded because nothing can overwrite what it writes. That half is true -- the only observation that could belongs to a job already in a terminal state. The other half was missing: a save whose copy is still in flight when the user taps Start over lands `Saved` on a screen they have just cleared. Guarding it would drop that write instead, which reports nothing for a file that may genuinely have reached the destination. That is a question about what the screen should offer during a save, and answering it in a race fix would be deciding it by accident. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
3f731d8ea7 | Merge remote-tracking branch 'origin/main' into merge-117-tmp | ||
|
JMR-devandClaude Opus 5
|
cc424dd08f |
Let the user's pick keep the screen a reattachment was about to take
`reattach()` read `_state.value`, found it `Idle`, and then handed the job to `observe()` -- which launches a *separate* coroutine that cannot write until its `collect` has resumed with a `WorkInfo`. So the check happened at one moment and the write landed at another, with a whole pick able to fit in between: the user tapped, their metadata query suspended, the guard saw an empty screen, and the finished job from an earlier session wrote over `Ready(picked)` a moment later. The comment above that guard said "no suspension point between this check and the assignment below, so nothing can interleave". There is no assignment below, and the two lines are in different coroutines. That sentence is why this sat as flaky CI for two days rather than being read as the product race it is. `ScreenOwnership` makes the answer the test already encodes -- the user's pick wins -- true rather than probable. A claim is taken synchronously when the user acts; every write that lands after a suspension point checks the claim it was made under and drops itself if that claim has been superseded. Dropped, not reordered: a write that is dropped cannot come back later. Cancelling the superseded observer was never enough on its own. `Job.cancel` is honoured at the next suspension point, and a collector that has already resumed and is on its way to `_state.value = ...` has none left; the write lands anyway. It also cannot help at all in the case reported, where nothing supersedes the observation until after it has been launched. `JoinViewModel` had the identical shape and nothing watching it, so it gets the same fix and the counterpart test that was missing. Its pick dispatcher becomes injectable for the same reason `ConversionViewModel`'s already was: without that seam there is no way to ask what happens while a pick is still in flight. Closes #49 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
25aac95db9 |
Say in the run-shape table when the wedge timeout was what killed the leg
The report added by #111 runs on every path out of e2e-run.sh, including the wedge, and until now it answered a question it had not been asked. On job 98035980326 -- API 34, a docs-only PR -- it printed `received: 59` and `completed cleanly: yes` six seconds before `##[warning] ... WEDGED`, for a leg the WEDGE_TIMEOUT had killed 22 minutes in. `completed cleanly` means only "instrumentation was not aborted", which was true; a reader scanning the table had to notice a separate warning line to learn the leg had died. The wedge cannot be read out of the log, which is why it is passed in: a wedge is gradle never returning, so gradle printed no verdict, no truncation line and no INSTRUMENTATION_ABORTED, and the log it leaves is the log of a run that just stops. Only e2e-run.sh saw `timeout` exit 124. It now derives that fact once and tells the report as E2E_WEDGED_AFTER, and reuses the same variable for capture_wedge so the two cannot drift. The table gains a `wedged:` row above `completed cleanly`, and `completed cleanly` flips to no -- but only where it would have said yes. An abort already says no and names the abort, which the wedge row does not, and a run that left no evidence still says unknown; a wedge on top of either prints both facts. `received`'s source line told the same lie in the same table -- "the run was not truncated, so every expected test reported" is only "gradle never got as far as saying so" when the leg was killed -- so it is qualified on that path. The number itself is unchanged, and so is `failed: unknown`: gradle printed no summary line, so that count genuinely is not knowable. Nothing here decides anything. No exit status, no pass/fail rule, no baseline comparison and no `::notice::` behaviour changes; the leg already failed correctly and still does. Verified against captured CI output rather than a live emulator, as #111 was and for the same reason -- this host cannot run API 37 and cannot wedge on demand. Four real logs (the wedged leg, a green API 34 leg, a failing gating leg, and an advisory leg with its baseline deviation) through both versions of the script, in both env states, comparing stdout and the job summary: only the wedged run with the signal set differs, byte for byte. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
dce516224c |
Offer a fix that works when the file has no video to copy
Refusing "copy the video" for a file that has none built its one suggestion by hand — drop the video track and leave everything else alone. That is valid only when the audio axis already happened to be fine. For any audio the target cannot carry (Vorbis or PCM into MP4, MP3 into WebM) the offer is refused in the next breath, so the Advanced picker showed a one-tap fix leading straight to a second error. Nothing unsafe shipped — ConversionWorker re-validates — but it is a dead end, and it contradicted the promise Validation.Invalid makes in its own KDoc. Route it through the shared repair-and-filter path instead, as every other branch does. Excluding what the *user* asked for rather than the already-repaired spec is what keeps the case that worked working: an MP3 into MP4 still gets its copy offered, because the repair of a copyable track is that same copy. Only a branch that builds its own list can break that promise at all, since suggestions() ends by filtering on validate().isValid. The property test now covers both of them — this one and the image output — rather than reaching them by luck, which is how a dead-end chip survived two earlier widenings of it. Its failures name the probe too: three rows share a spec and differ only in the input. Closes #114 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
2b7520061b | Merge remote-tracking branch 'origin/main' into merge-119-tmp | ||
|
JMR-devandClaude Opus 5
|
4e46eb99f6 |
Say what the theme's dynamicColor parameter does, and test the branches that run
The KDoc claimed dynamic colour "stays switchable so users can opt back to the brand palette". Nothing switches it: MainActivity is the only caller and passes no arguments, so dynamicColor is always true and the two brand-palette branches are dead. A reader who trusted that sentence would go looking for a setting that has never existed. Replace the claim with what is true today and point at #68, which holds the decision -- add a switch, delete the dead branches along with the template palette, or replace that palette first. None of the three is taken here. ThemeKt had no test, so nothing would have caught the branches being swapped either. Assert what the theme resolves by reading MaterialTheme.colorScheme inside the content lambda: the two live branches on background luminance, which is the one thing two schemes off the same device palette do not share, and the dead pair by passing dynamicColor explicitly. Both KDocs say plainly that the test is the only thing that passes it, so the coverage is not misread as evidence a switch exists -- which is the misreading #68 exists to prevent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
83b557409e | Merge remote-tracking branch 'origin/main' into merge-116-tmp | ||
|
JMR-devandClaude Opus 5
|
c887af0d83 |
Offer the file again after a failed save, rather than only offering to delete it
save()'s onFailure keeps the staged file on purpose -- it can be the only copy of an hour of transcoding, and the destination did not receive it -- and then handed the screen a Failed carrying a message and nothing else. That branch rendered exactly one control: "Start over", wired to reset(), which discards precisely the file the comment above it goes out of its way to keep. The intent was already written down in main; the UI did not honour it, and the only rescue was process death followed by reattach -- unadvertised, and bounded by a sweep that collects anything a day old. Failed now carries a PendingSave, and only where the failure came from save(). A transcode that died staged nothing and must not sprout a save button, so the handle is nullable and the observe() arm leaves it null; so does a save that found the file already gone. The branch renders "Try saving again" above "Start over", opening the same CreateDocument flow with the same name and type the first attempt used. A retry that fails again lands back on a carrying Failed rather than a bare one, so the second failure cannot eat what the first kept. Start over still deletes from there, and that is a decision rather than an inheritance: deletion is the user's choice only once the alternative has been offered. pendingStaged remains the single owner of the delete, so the carried handle is a view of it rather than a second owner and no path out of the state can drop a file the old shape could not. pendingSave() exists so save() and each screen's CreateDocument registration answer "what would a save target" once instead of twice -- the entry points cast to Converted/Joined, which answered null for a Failed and fell back to the current pickers, wrong for any spec edited since the job ran and for every reattached job. Both tabs, since JoinViewModel and JoinScreen have the same shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
2e0c6737d6 | Merge remote-tracking branch 'origin/main' into merge-113-tmp | ||
|
JMR-devandClaude Opus 5
|
85461943d6 |
Keep the instrumented test counts in step with the suite
The API 37 entry names how many instrumented tests there are and how many the gating leg runs, and this PR adds one. Nothing asserts those figures, which is exactly why they rot quietly: 59/56 becomes 60/57. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
238142d9cc |
Guard the whole Media3 export instead of only its two ends
transcode() posts its work to a HandlerThread, and everything on that thread has no caller to throw back to: an escaping exception reaches the thread's uncaught handler and takes the process down, while the continuation is never resumed. Both halves of that are bad, and the second is arguably worse — a worker left suspended forever holds a foreground service. The guarding was two narrow runCatching blocks, one around buildTransformer and one around transformer.start, with the two Media3 builders sitting unguarded between them. That gap was not theoretical. EditedMediaItem.Builder rejects a composition with both tracks removed, which is exactly what a plan of (Drop, Drop) asks for, and it does so with a plain IllegalStateException from the constructor. Validation now refuses the spec that produces such a plan, so neither the picker nor ConversionWorker will start one. Routing is a separate question and still answers Media3 for it — a dropped track makes nothing un-hardware-able — so a request that skips validation still arrives here: a job queued before the settings changed, or one made through ConversionWorker.request directly. CopyPlanner's own KDoc already names that path as the reason it re-checks what validation has checked; this is the same belt for the same braces. One guard around the whole body costs nothing on success and turns any such refusal into a failed job with a reason attached. The export body moves into startExport, whose contract is the thing that makes one guard enough: returning normally means the export is running and the listener owns the continuation, throwing means it never started and the caller does. Cancellation is still registered before start. Covered twice on purpose. Robolectric runs the real HandlerThread and the real Media3 builders, so the JVM test exercises the whole sequence and can be run anywhere; the instrumented one repeats it against the real framework. Neither asserts only that the failure is an IllegalStateException, because withTimeout raises TimeoutCancellationException and java.util.concurrent.CancellationException extends IllegalStateException — so that assertion alone calls an unresumed continuation a pass. Both were written that way first, and reverting the guard is what exposed it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
9c809d4e16 |
Refuse a spec that would leave the output with no tracks at all
Validation already refused two ways of asking for an empty file: None on both codec axes, and Copy for a video track the input does not have. It missed the third, because it read only the spec. Name H.265 with the audio off, hand it an MP3, and the spec looks fine — it names a video codec — while CopyPlanner drops that track anyway, because the *input* has no video to encode. The plan is (Drop, Drop), the router still says Media3, and EditedMediaItem.Builder refuses to build a composition with both tracks removed. It refuses it on Transformer's own HandlerThread, where the user sees the app die rather than a reason. Asking the probe as well as the spec catches all three faces with one guard, and the equivalence is exact rather than approximate: CopyPlanner drops video for None or for an input with none, and audio for None, so "(Drop, Drop)" and this condition are the same set. A sweep over every non-image container by codec by codec against both probes asserts that, so a new container or codec cannot reopen the gap on an axis nobody wrote a case for. This newly refuses a combination the Advanced picker accepts today, and that is the point: today it crashes. What it must not do is refuse without a way out. The Copy face had one only nominally — its single hand-built suggestion was None + None, which validation rejects in the next breath, so the one-tap fix fixed nothing. All three faces now go through the shared repair-and-filter path, which for an MP3 into MP4 offers "copy the audio across" and nothing that has to be re-refused. Repair is also stopped from naming a video codec for a file with no video track. It used to fall through to the first codec the container could encode, so the fix offered for an MP3 was "H.264" — a codec CopyPlanner then drops, making the offer a fiction that happened to validate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
bf2214a549 | Merge remote-tracking branch 'origin/main' into merge-111-tmp | ||
|
JMR-dev
|
72ff7adfcc | Merge branch 'main' into docs/seven-run-counts | ||
|
JMR-dev
|
994ea8a3dd |
Say in the step log that the summary was written, since nothing else can
The job summary is the deliverable #83 asked for -- "readable without opening a log" -- and GitHub exposes no API that reads a job summary back: the check-run output for the advisory job returns summary: null, so a write that silently did not happen would be invisible to everything except a human on the run page. The step log can be read, so it now carries one line saying which of the two happened, including the case where GITHUB_STEP_SUMMARY is unset entirely, which is what running the script by hand looks like. |
||
|
JMR-dev
|
3e9528454c |
Announce a baseline it cannot read, rather than falling quiet
"A comparison was asked for" and "a number was found to compare against" were one variable, and collapsing them put the report one refactor away from being the thing #83 filed. The sed that reads FAILS_ON_EMULATOR_API37_BASELINE is anchored at the line start, so indenting the const into an object -- or renaming it, or moving it -- empties it, and the old code then skipped the whole comparison while the table kept printing exactly as before. Silent, and indistinguishable from a run that matched. Now an unreadable baseline is itself a deviation, with the notice naming the const so the fix is obvious. Verified against the real captured log of run 32865281555 three ways: baseline file absent, const indented into an object, and the committed file unchanged -- the first two announce, the third stays silent. |
||
|
JMR-dev
|
0702916229 |
Say what the advisory API 37 job actually found, so a new failure is not invisible
That job is continue-on-error and red on every PR by design, which CLAUDE.md states plainly -- and that instruction is exactly why nobody reads it. Nothing in a red X separates "the known three" from "the known three plus yours". A bare failure count would not have fixed it, and this is measured rather than assumed. The run is usually truncated: seven of eight advisory runs read on 2026-08-25 ended in `Test run failed to complete. Expected 3 tests, received 2.` with INSTRUMENTATION_ABORTED, and one did not. A count taken from a truncated run misleads in both directions -- a fourth marked test can still yield the same number if the abort lands earlier, and the known set getting worse can lower it. The test XML does not rescue it either, which was the thing worth checking before building on it: it IS written for an aborted run, and it reports a tidy tests="3" failures="3" for a run the runner had just described as truncated. So the XML is the authority on how many results landed, the runner's own output is the only authority on whether the run finished, and the report reads both and says which number came from where. The baseline is one number beside the marker, because the marker means "cannot pass on this image": the count is both how many tests the advisory leg runs and how many should fail. A smaller failure count is the interesting direction -- it means one now passes, which is the documented trigger for deleting the annotation. Nothing about the job's status changes. It stays continue-on-error, stays red, stays out of the required contexts; a deviation is a ::notice::, never an ::error::. The report is a separate script so it can be run against a real log saved from a real CI run, which is how the comparison was shown to fire. The gating legs get the shape without the comparison: they run the whole suite, so comparing there would announce a deviation five times a run -- but a truncated run reporting fewer results than it ran is what #108 looks like, and "completed cleanly" is the field that would show it. Closes #83 |
||
|
JMR-dev
|
d01a46a708 |
Stop counting the run this page calls inconclusive
R29 found the discriminator claimed "exact across all seven" while r07 is recorded lower down as "inconclusive rather than ruled out, because no evidence came back from it". A row this page calls inconclusive cannot also be counted as evidence for the conclusion. Checking it turned up a second instance of the same over-count, which R29 did not name. The abort-cadence section said "Measured across the seven runs above" -- but the table records r07's aborts as **not readable**, because adb wedged before a crash buffer could be taken. Six runs contributed gaps, not seven. Both now say six, and both say why. The discriminator paragraph also says what excluding r07 costs, which is nothing: it is a `host` row, so the discriminator predicts it would not boot, and confirming a prediction with the one run whose evidence did not come back adds no information in either direction. That is the point R29 made -- claiming six does not weaken the conclusion -- and it is worth stating in the document rather than only in the ticket, because the next reader will otherwise wonder whether a run was quietly dropped. Deliberately left: "four of the seven runs show the directory creation itself is broken during the loop". That is a count of how many runs showed something, not a claim that all seven were readable for it, so it survives. Checked rather than assumed, and named here so the next pass does not re-audit it. R29's other half -- "state how r07's boot outcome was read" -- is not taken, because I do not know and inventing a source would be worse than narrowing the claim. Narrowing is the option R29 offered and the one that can be honest. Closes #38. |
||
|
JMR-dev
|
3806641cb2 | Merge branch 'main' into test/release-permission-guard | ||
|
JMR-dev
|
8d8703ab49 | Merge branch 'main' into ci/build-workflow-permissions | ||
|
JMR-dev
|
4a8e30099e |
Notice if the release job loses the permission that lets it publish
build.yml's `release` job declares `contents: write`, and nothing checked it. Deleting those lines leaves actionlint clean and CodeQL silent -- a narrower permission is not an alert -- and the job is `if: startsWith(github.ref, 'refs/tags/v')`, so no pull request and no merge can exercise it. Measured with the declaration removed: every gating check still passed. The first thing that would notice is a release failing to publish, at the moment someone is trying to cut one. The deletion also looks like tidying. #106 has just put a top-level `permissions: contents: read` directly above it, so a reader could reasonably take the job-level block for a duplicate. It is an override, and a comment saying so is not a check. BackupExclusionsTest is the precedent: configuration rather than code, load bearing, and unguarded because nothing compiles it. The part worth reading twice is the second commit-worth of work in here. The test passed, and then the mutation that is supposed to redden it did not: BUILD SUCCESSFUL in 614ms Gradle cannot infer that a test depends on a file outside the source set, so the task stayed UP-TO-DATE and the test never ran. Under --rerun-tasks the same mutation failed it properly, which is the tell: the assertion was right and the wiring was not. A guard that does not re-run when its subject changes is not a guard -- it is a test that will be green on the day it matters, which is worse than no test because it reads as cover. Fixed by declaring the workflow as a task input. Verified the whole way round afterwards, without --rerun-tasks: mutate the file and the task re-runs and fails; restore it and the task re-runs and passes. What this pins and what it does not: it asserts the declaration exists in the release job's block. It cannot assert a release actually publishes -- that needs a tag push, which is the thing no PR can do. A tripwire against silent removal, not proof the path works, and the KDoc says so. Closes #107. |
||
|
JMR-dev
|
e7d84cc69f | Merge branch 'main' into docs/api37-point-release | ||
|
JMR-dev
|
865a4a7c8e | Merge branch 'main' into docs/benchmark-populate-path | ||
|
JMR-dev
|
49c483d877 |
Declare build.yml's token reach in build.yml
CodeQL alert #1, the only open one on this repository: actions/missing-workflow-permissions, warning / medium, build.yml:23 Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Alerts 2, 3 and 4 were the same rule against status_check.yml and are fixed -- that file has a top-level block. build.yml declares permissions in exactly one place, the release job's `contents: write`, and has no top-level default, so the `test` job inherits the repository setting. **Nothing is over-privileged today.** The repository default is already `read` (default_workflow_permissions: read, can_approve_pull_request_reviews: false, read from the API rather than assumed), so the test job holds a read token now. Saying so matters: this is hygiene, and a commit that implied it was closing a live hole would be overstating it. What it buys is that the default CANNOT widen these jobs later without someone editing this file. That is not invented for the occasion -- it is the argument status_check.yml already makes, which even names this file: the token's reach should be readable here, and a default that widens later should not silently widen these jobs with it. build.yml's release job makes the opposite declaration for the same reason. So the principle was decided, applied in two workflows and in one job of this one, and the top level of build.yml was the gap. Verified the thing that would actually break: the release job's `contents: write` still wins. Top level is a default, not a ceiling -- parsed and printed both, test inherits `contents: read`, release keeps `contents: write`. Also ran the ticket's mutation, and it found something. Deleting the release job's `contents: write` leaves actionlint green and CodeQL quiet -- a narrower permission is not an alert -- so nothing would catch it until a tagged release failed to publish. That is a separate gap and is filed rather than fixed here. actionlint clean at the pinned digest. Comment and permissions only; no step, job or trigger changes. Closes #100. |
||
|
JMR-dev
|
1b220856ab |
Say 37.0 is the choice, not the only api-level that exists
R19 raised two things about this comment. One resolved itself: it used to explain why the matrix had no API 37 row at all, and #56 added the gating row, so that half is gone. The other survived, and this is it. The comment read api-level must be "37.0". A bare 37 is not an SDK package and fails during setup The second sentence is true and was measured -- it cost a run to find. The first overstates it. What must be true is that the api-level is a POINT release; 37.0 is one of several. api37-debug.yml's own input descriptions already say so: API level, as the SDK spells it. 37.0, 37.1, 37.2-beta3, 36 ... System image target. android-37.1 and 37.2-beta* ship ONLY as google_apis_ps16k and docs/api-37-emulator-crash.md measures android-37.0 rev 6 and android-37.1 rev 8 side by side, both aborting. So the repo already knows 37.1 exists and behaves the same; only this comment implied otherwise. That matters for the reader it is written for. Someone debugging this row and wondering whether a newer image helps reads "must be 37.0" as a constraint and stops. The measured answer is that it does not help, which is a better thing to learn than a rule that is not one -- and the ps16k-only wrinkle above 37.0 is the detail that would actually bite them. Comment only. No job, matrix, filter or gating behaviour changes. actionlint clean at the pinned digest. Closes #28. |
||
|
JMR-dev
|
40ae524388 | Merge branch 'main' into fix/dead-assertion-probe-test | ||
|
JMR-dev
|
a1d79c212a | Merge branch 'main' into ci/actionlint | ||
|
JMR-dev
|
d37c391c60 |
Stop telling people to stage the benchmark the one way it cannot be staged
RealMediaBenchmark's class KDoc said:
Populate with:
adb push <file>.mp4 /sdcard/Android/data/org.libremediaconverter/files/
Twelve lines below, the `samples` property KDoc -- on `get() = context.filesDir` -- says:
Internal storage, not the external files dir. Files placed in the external dir by
`adb push` or `adb shell cp` stay owned by the shell user, and the app then gets
EACCES trying to read them -- which presents as an unparseable input rather than a
permission problem.
Different directories, and the second exists specifically to explain why the first fails.
Anyone following the class KDoc stages files the benchmark cannot read, gets a skip, and
reads the skip as "not staged yet" -- the failure mode the property KDoc warns about, walked
into by the instruction in the same file.
The fix is not a corrected command. Restating the mechanism in a second place is what let
these drift, and a replacement command I have not executed would be the same defect with a
fresher date. The class KDoc now names [samples] as the single place that answers it.
Two things added that are checkable rather than remembered: the exact filenames the tests
look for, via [H264_SAMPLE] and [AV1_SAMPLE] -- the old text said `<file>.mp4`, so even the
right directory left you guessing -- and a note that the two skips every green E2E leg
reports are these.
Not claimed: that the benchmark misbehaves on CI. An earlier version of the ticket said so;
it was wrong, and measuring settled it -- both tests report SKIPPED on the gating legs, the
guards work, and "harmless in CI" is accurate. The failure that prompted the look is
Media3EngineTest, tracked as #102.
Closes #101.
|
||
|
JMR-dev
|
3fb25235c0 | Merge branch 'main' into test/device-codecs-encode-consequence | ||
|
JMR-dev
|
95902a7889 |
Delete an assertion that could never fail, and say what guards instead
ConversionViewModelProbeFailureTest's pickedProbe() helper held:
val ready = awaitState(viewModel.state, "Ready with a probe") {
it is ConversionState.Ready && it.input.probe != null
}
assertNull("nothing here should reach a terminal failure", (ready as? ConversionState.Failed))
The predicate requires `Ready`. `Ready` and `Failed` are sibling subtypes of one sealed
interface, so `ready as? Failed` is always null and the assertNull could never fire. R26
filed this PLAUSIBLE on types read; it is measured now.
Flipping the line to assertNotNull failed 3 of the 4 tests in the class -- three, because
pickedProbe() has three callers, which is also why a dead line here was worth removing
rather than shrugging at: it read as coverage in a helper the whole class depends on.
Deleted rather than replaced. There is nothing for a live assertion to add: a pick that
ended in Failed never satisfies the predicate, so awaitState fails on its timeout naming
what it was waiting for -- "Ready with a probe" -- which is a better failure message than
the assertion would have produced. The comment now says that, so the next reader does not
re-add the guard the predicate already is.
This is the ninth vacuous assertion this line of work has turned up, and the pattern is
consistent: they hide in helpers, they pass, and they look like care. The suite is green
before and after, which is exactly the point -- deleting a dead assertion cannot change a
result, and if it had, the line was not dead.
Closes #35.
|
||
|
JMR-dev
|
1535b61a96 | Merge branch 'main' into fix/sdkmanager-pipefail | ||
|
JMR-dev
|
240528facb | Merge branch 'main' into docs/readme-restart-claim | ||
|
JMR-devandClaude Opus 5
|
25f162923c |
Close the ANR dialog that was hiding every window from UiAutomator
SafPickerRoundTripTest began failing on gating legs at API 33, 34, 35 and 37 ninety minutes after it landed, on diffs that cannot cause it -- two KDoc comments, a MIME lookup table, a README paragraph. Every failure named the fixture root, so #93 was filed as a root-discovery race. It was not one, and finding out what it was took making the test say something else first. DocumentsUI was fine throughout: its own `ProvidersAccess: Matched roots` names the fixture authority five times inside the sixty seconds the test spent failing. What failed was reading any window at all -- 1095 `Retrieving node with selector` against 1095 `Node not found` on that leg, against 7 and 2 on the green one. So this now asks whether the app's OWN window is readable before it opens a picker, and prints the accessibility window list when it is not. That list named the culprit on the next occurrence: What it could see: com.android.systemui[type=3], android[type=3] No TYPE_APPLICATION window at all, on a device that had just logged `Displayed org.libremediaconverter/.MainActivity`. `android[type=3]` is system_server, and the same logcat says what it was holding, minutes before this class ran: ANR in com.google.android.apps.nexuslauncher Reason: Input dispatching timed out (Application does not have a focused window) Window{4ed8414 u0 Application Not Responding: com.google.android.apps.nexuslauncher} The launcher ANRs on a loaded runner emulator and the dialog it leaves behind never goes away. It is opaque and fullscreen, so AccessibilityWindowManager drops every application window beneath it -- which is how the app can be Displayed and unreadable at once, the contradiction that made this look like a SAF bug for six PRs. Present on both legs examined, API 33 and 34, at the failure timestamp. So the dialog is dismissed, by resource id rather than by localised button text, `aerr_wait` first so the app under it is left alone. Waking the device and rebuilding the UiAutomation connection are kept behind it and are recorded as measured non-causes rather than as fixes. A second PickActivity is not a remedy for this either, and that was measured: the failing leg opened one for the second test, in the same DocumentsUI process, and read as little from it. The whole pick is still retried, but for a smaller and separate claim -- a picker whose lists were built before their data arrived, which #80's node-level re-find cannot reach because it re-acquires a handle inside the one picker. One API 37 run failed a step deeper, on the file rather than the root. That shape has not been reproduced or diagnosed; the reopen covers it because a fresh pick re-walks from Recent, and the KDoc says that rather than claiming more. Two things the retry must not become. It must not tolerate an absent root, or #64's MIME mutation goes vacuous -- so a missing node is reported rather than retried away, and the mutation was re-run: both tests still fail, still with "the system picker never showed BySelector [TEXT='\QLMC R38 fixtures\E']", in 126 s and 127 s against the 1200 s wrapper timeout. And it must not decide the picker has closed by asking the same accessibility window list that is broken -- so the back presses are counted against Activity.hasWindowFocus, which comes from the framework. Each new path was forced on and measured rather than trusted: the injected-failure run showed the reopen recovering, with four OPEN_DOCUMENT starts for two tests; the rebuild was forced unconditionally and the suite stayed green, ruling out a connection that comes back without FLAG_RETRIEVE_INTERACTIVE_WINDOWS; the dialog dismissal was forced with no dialog present, ruling out a blind click breaking a healthy run. Dismissing a real ANR dialog has not been observed, because the fault has never reproduced locally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
d0b9745220 | Merge branch 'main' into docs/readme-restart-claim | ||
|
JMR-dev
|
3f140fc2b1 |
Lint the bash inside the workflows, not only the bash in files
The shellcheck step added a few hours ago reads `git ls-files '*.sh'`. That is four files. It does not read the inline `run:` blocks, and a good deal of this repo's bash lives there: the release verification in build.yml, the emulator setup and teardown in status_check.yml and api37-debug.yml. "shellcheck runs in CI" was true of the files and not of the blocks, and CLAUDE.md said so rather than pretending otherwise. actionlint closes that half. It parses each workflow and runs shellcheck over every `run:`, on top of its own checks for expression syntax, `needs:` references, matrix keys and action input names. Pinned by digest, for the reason shellcheck is pinned -- a new rule making untouched files fail is a red build whose diff cannot explain it -- and for a second reason of its own. actionlint's documented install is bash <(curl -s https://raw.githubusercontent.com/.../download-actionlint.bash) off a moving branch. Running that in a repository that pins every action by SHA would contradict its own supply-chain posture more than the linter is worth. That is why #70 was filed instead of bolted onto the shellcheck commit. It reported exactly one finding, and it is fixed here rather than suppressed: build.yml parsed `ls` to pick the release APK (SC2012). The glob was already in the line, so a bash array reads it without the pipe. Gradle's output names have no spaces today, which is the kind of assumption that holds right up until it does not. Proved it catches something, rather than trusting a green run: planting `if [ $UNQUOTED = bad ]` into a build.yml `run:` block produces shellcheck reported issue in this script: SC2086:info:4:6: Removed again afterwards. A linter that cannot be shown to catch a plant is not wired in, it is just running -- and SC2086 in a `run:` block is invisible to the .sh-file step, which is the whole argument for this commit. CLAUDE.md loses the "does not cover inline run: blocks" caveat, because it no longer does. Both linters verified clean at their pinned digests. Closes #70. |
||
|
JMR-devandClaude Opus 5
|
b3208ef8c7 |
Hold the two claims the codec MIME tables only asserted in prose
Two reasoned decisions were sitting in comments with nothing under them. `AndroidDeviceCodecs.mimeFor`'s `COPY, NONE -> null` arm explains itself by naming a consequence at another seam: returning null is what makes `canEncode` answer true, because a copied or absent track places no demand on the hardware. #90 pinned the null; nothing pinned the answer. Put a MIME in that arm and a device with no matching encoder starts refusing stream copies — jobs that encode nothing — and the router hands FFmpeg a re-mux Media3 could have done. Asserted now against `forTesting(encoders = emptySet())`, with an H.264 refusal alongside so a `canEncode` that simply said yes could not satisfy it. The second is a whole table. `Media3Engine.videoMimeTypeFor` is `VideoCodec -> MIME` on the same axis as `mimeFor`, and until #85 and #87 widened both to `internal` no test could see them together. Each had per-arm coverage pinning its own answers, which is exactly the shape that cannot notice the two tables describing different codecs: change one arm and its own expectation together and both suites stay green while the device is asked about H.265 and Transformer is told to produce H.264. They do not agree everywhere, and forcing them to would be a regression, so the test sorts every codec into the three buckets that exist and asserts the fourth is empty. H.264 and H.265 must match. VP8, VP9 and AV1 are named by the device table and not by Transformer's, deliberately: `setVideoMimeType` rejects them so the router never asks Media3, while the device may genuinely own a VP9 encoder and `canEncode` has to answer about it truthfully. COPY and NONE are named by neither. Sorting rather than filtering means a convergence fails too, so moving the line requires saying so in the file. Audio has no partner — `AndroidDeviceCodecs` enumerates video MIME types only, so `audioMimeTypeFor` has nothing to cross-check against and a missing audio encoder is still discovered by failing rather than up front. Named in the KDoc as unfinished rather than left as an unexplained asymmetry. Closes #86 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
5a8aedf53d |
Read sdkmanager's status, not the status of the yes feeding it
run-e2e.sh installs a missing system image with
yes | sdkmanager --install "$pkg" > /dev/null 2>&1 || { echo " FAILED to install"; ... }
`yes` never ends. The moment sdkmanager exits and closes the pipe, `yes` dies of SIGPIPE
with 141, and this script runs under `pipefail`, which takes the rightmost non-zero status.
So a package that installed perfectly reported "FAILED to install $pkg" and returned 1.
R32 filed this PLAUSIBLE on shell semantics, unexecuted. It is demonstrated now:
set -o pipefail; yes | true -> 141 (three runs, three times)
set -o pipefail; yes | sh -c 'exit 3' -> 3
${PIPESTATUS[1]} for those two -> 0 and 3
The pipeline status genuinely cannot tell a clean install from a broken one; PIPESTATUS
can. That is the whole change -- no restructuring of the licence flow, so a fresh SDK still
gets its licences accepted exactly as before.
`echo no | avdmanager` eleven lines below is deliberately left alone, and the comment says
so. One line fits the pipe buffer, so echo has already exited before the close and there is
no signal to receive: `echo no | true` measured 0 on five consecutive runs against `yes |
true`'s 141 on three. Only an unbounded producer is exposed. Someone reading this fix later
would otherwise "fix" the echo too and change a line that was never wrong.
Why it went unnoticed: it only misfires when the image is ABSENT, and every existing
checkout already has the images. R32 noted the branch that made this the normal path. The
failure is also silent in the worst way -- the install succeeds, the script says it failed,
and the AVD is then created from a package that is really there.
shellcheck clean at the pinned digest (0.11.0, the version CI runs), bash -n clean.
Closes #41.
|
||
|
JMR-dev
|
a0b6a3dde8 | Merge branch 'main' into fix/probe-dispatcher-seam | ||
|
JMR-dev
|
bda5abea6c | Merge branch 'main' into test/media3engine-mime-tables | ||
|
JMR-dev
|
21eeb6f3f8 | Merge branch 'main' into test/mediaprobe-pure-helpers | ||
|
JMR-devandClaude Opus 5
|
2063fe06aa |
Point the coroutines-test comments at the file that still uses it
Both the dependency declaration and its catalog entry named EscapedCoroutineErrors.kt as the sole reason kotlinx-coroutines-test is on the test classpath. That file is gone, and nothing in the gate -- not ktlint, not detekt, not lint -- fails on prose naming a deleted file, so this would have survived as a reference a reader could only resolve through git history. The dependency itself stays, and for a reason worth restating where it is declared: `runTest` is what registers the collector callback, so the one test that deliberately lets an error escape is the scope that receives it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
47a423413b |
Say which conversions come back, rather than that they all do
README promised, without qualification: "Conversions run as durable background work, so they survive leaving the app and are restored after a restart." The first half is true and the reattachment work made it truer. The second half has one exception the sentence does not admit, and it is the case a user is most likely to hit without understanding it. When Android refuses a foreground-service start, FailureOutcome retries -- ten attempts on the default exponential backoff, 30 s doubling to a five-hour clamp, about eight and a half hours in total -- and then returns FOREGROUND_DENIED on a FAILED job. Reattachment excludes FAILED (Reattachment.kt:176). So the job is not restored, and neither is the message explaining why: the user opens the app to an empty screen. FailureOutcome's own KDoc already says this plainly -- "a user who was not watching when the eleventh attempt ran will find an empty screen rather than the explanation". The code was honest and the README was not, which is the wrong way round for the two documents. The replacement says what actually happens and ends with the thing the user can act on: reopening the app is what grants permission to run, so a conversion stalled this way should be started again rather than waited on. That is the same reasoning FOREGROUND_DENIED_MESSAGE is written on -- "open the app and start it again" is the fix, not filler. Deliberately not claimed: that the app tells you. It does not, and #16 is the open ticket for giving a present, willing user a way to make that retry happen now. Writing "you will be told" here would be the same defect this commit is fixing, one release earlier. Verified against the current code rather than the finding's date -- R35 was filed as PLAUSIBLE on 2026-08-22 and both mechanisms it names are still in place. Closes #44. |
||
|
JMR-dev
|
dab28d5f44 | Merge branch 'main' into fix/codec-vocabulary-drift | ||
|
JMR-devandClaude Opus 5
|
4aba3bbd2e |
Stop swallowing coroutine errors nobody asserted on
`drainEscapedCoroutineErrors()` cleared the collector at rule-construction time
with `runCatching { runTest {} }`, and discarding what it found was the whole
mechanism: it could not tell the one known deposit from an escaped error nobody
had asserted on. That traded a loud, misleading failure for a silent one, which
was acceptable only while exactly one depositor existed and the seam to remove it
did not.
The seam exists now, so the depositor is gone: the OOM is consumed by the test
that raises it. Every Compose class takes the v2 `createComposeRule()` directly,
and a future escaped error fails a test again instead of disappearing.
The two findings the drain's KDoc carried that outlive it: the v2 rule and the
non-v2 `StateRestorationTester` do interoperate -- the note now sits at the two
declarations that pair them -- and a drain could never have been a `@Before`
(the rule's `runTest` wraps it) or a `@BeforeClass` (Robolectric runs that
outside the sandbox classloader, where the collector is a different object).
Full JVM suite run twice in a row with the drain deleted: 373 tests, 0 failures
both times.
Closes #66
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
dbba213c51 |
Give the pick a dispatcher, so an escaped error fails the test that caused it
`onInputPicked` hops to a hard-coded `Dispatchers.IO` inside a `launch` with no exception handler -- deliberate, because a real OutOfMemoryError should reach the thread's default handler and take the process down. On the JVM there is no such handler: kotlinx-coroutines-test installs a process-wide collector, once per classloader and never removed, which keeps the error and rethrows it at whichever `runTest` starts next. Every Compose rule is a `runTest`, so the OOM raised by `ConversionViewModelProbeFailureTest` failed some *other* Compose class, and which one moved between runs of identical, green code. Naming the dispatcher gives the throw somewhere to land. With the pick inline inside a `runTest`, the collector's callback belongs to the test that caused the error, so it is handed over and consumed rather than stored for a stranger. Both hops of a pick rather than only the probe, which is where this differs from the seam issue #66 sketched: leaving the metadata query on a real IO thread makes the coroutine resume on a main looper Robolectric leaves paused, and that bounce is exactly the asynchrony that made delivery unpredictable. That buys the assertion the test could not make before -- the real OutOfMemoryError instance, not an inference from a card that never filled in, which is also what a probe returning null looks like. Reverting the hop to `Dispatchers.IO` turns it red: "expected java.lang.OutOfMemoryError to be thrown, but nothing was thrown". Refs #66 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
8ac6e2b1c2 |
Name the format in the image-demuxer failures
Bare assertTrue/assertFalse report java.lang.AssertionError and nothing else, so the mutation that proves this test bites -- relaxing the _pipe suffix to a substring -- went red saying only that a line failed. The format name is the one thing a reader needs, exactly as the MIME is in the sibling test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
4ff44be1d7 |
Give the Robolectric choice a reason that is still true
Two test classes justified using Robolectric by asserting that the alternative does not
exist:
AppRootRestorationTest "The instrumented tests cannot run on the development
host at all (see CLAUDE.md)"
OutputPublisherStagingTest "The instrumented suite cannot run on the development
host, so this is the only place [it] can be caught"
Both were true when written and stopped being true on 2026-08-22, when the segfault was
traced to SwiftShader's Reactor JIT against SELinux execheap rather than to the machine.
tools/local-emulator/run-e2e.sh has run API 33-36 here since.
The first one cites CLAUDE.md as its authority, and PR #73 corrected CLAUDE.md to say the
opposite. So it was no longer merely stale: a reader who followed the reference found the
contradiction, with the citation making the wrong half look verified. That is the worst
version of this -- R14, R15, R20 and R25 were all the same defect, and this is the fifth.
The choice itself was never wrong, which is why the fix is not to move these tests. Both
belong on the JVM, and the honest reason is cost rather than impossibility: neither needs
anything a device supplies, and both run inside the same ./gradlew invocation as every
other unit test instead of booting an emulator. That argument survives the correction; the
premise did not.
The old line also has a second failure mode worth naming. "Nobody can execute this" invites
a reader to skip the local run and let CI decide, which is the opposite of what the
definition-of-done in #51 asks for.
Verified: the string appears nowhere in app/src now, and testDebugUnitTest, ktlintCheck and
detekt are green.
Closes #46.
|
||
|
JMR-devandClaude Opus 5
|
7f951baf8f |
Make the two codec tables answer for each other, and stop describeAudio printing a NUL
The FFprobe codec vocabulary is written out in at least four places and none of them had a test. Two had already drifted apart. `x264`, `hev1`, `x265` and `vp09` resolved in `CodecNames.videoFromName` and returned null from `AndroidDeviceCodecs.mimeForCodecName`, so the app identified the codec for the source card and for routing and then ran the device capability check blind on the same string; `mpeg4` ran the other way and rendered as a raw name. Nothing could notice, and the reason is structural: a `when` cannot be enumerated, so no test can ask one table what the other one knows. Both are maps now, for that reason alone, and `CodecVocabularyTest` walks the two key sets. A name added to -- or removed from -- one side alone fails the build. The one legitimate asymmetry is listed rather than implied: `mpeg4` is decodable input with no `VideoCodec` to name it, so `CodecNames` is right not to carry it. That list is itself checked, because otherwise it is an escape hatch -- any future divergence could be waved through by adding the name to it, and adding `x265` to it now fails. THIS CHANGES BEHAVIOUR for `x264`, `hev1`, `x265` and `vp09`. A null from `mimeForCodecName` means "unknown to us: assume the platform can handle it and let a failed export trigger the FFmpeg fallback", which is the right policy for a name nobody recognises and the wrong one for a name recognised one file over. A device without the matching decoder now sends those four to FFmpeg up front instead of spending a doomed hardware attempt to discover it. No input loses hardware it could have used: each alias resolves to the MIME its canonical spelling already resolved to, so a device that has the decoder still answers true. `ConversionRouterTest` still passes and that is not evidence either way -- every `canDecode` in it is a hand-written stub that never reaches this table. #74 is the same family one level down. `describeVideo` answered "Unrecognised" for `InputProbe.UNPARSEABLE` and `describeAudio` had no such arm, so an unparseable audio codec would have fallen through to `?: name` -- and the sentinel opens with a NUL, so the source-info card would have rendered a `Text` beginning with U+0000. The two now share one body, which is what stops the next arm being added to one side only. Two corrections to that ticket, taken from the file rather than from the ticket, since it warns about exactly this: - It quotes `audioFromName` as opening with `null, InputProbe.UNPARSEABLE -> null`. It did not; it opened with `null -> null` and the sentinel reached `else`. Naming the sentinel in the shared lookup therefore changes no answer and is documentation, not the fix. - It says `describeVideo`'s arm has no test of its own. It did -- `descriptions stay readable for unknown and missing codecs` asserts it -- so deleting the shared arm now reddens three tests across both sides, not one. Mutations run, each on the full 386-test suite: add "avc3" to CodecNames only -> CodecVocabularyTest red on two counts, CodecNamesTest green: 8 tests, 0 failures, which is the ticket's point about per-table arm tests delete the UNPARSEABLE arm -> CodecNamesTest red on three, one of them quoting the NUL back add "x265" to DECODE_ONLY_NAMES -> CodecVocabularyTest red on the escape hatch delete "vp09" from the MIME map -> CodecVocabularyTest red on three, which is the state this commit is fixing Audio is not cross-checked, and that is a gap rather than a decision: the device capability check is video-only, so this module has no second audio table to compare `AUDIO_ALIASES` against. `Media3Engine.audioMimeTypeFor` is the other half and belongs to #85. `MediaProbe.shortName` (#84) is the fourth table and is untouched here for the same reason. Closes #87. Closes #74. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
5ec2bba64b |
Check the MIME types Media3Engine hands Transformer, and the claim above them
Both tables decide what codec ends up in the user's file, and neither was exercised. Point H265 at VIDEO_H264 and every hardware HEVC export writes H.264 into a file the user asked to be H.265: Transformer does as told, the export succeeds, and the only symptom is a codec nobody chose. One arm carried an assertion rather than a value -- "Never reached: only an Encode plan consults this, and COPY/NONE are not Encode" -- which is a claim about callers parked in a branch of a callee. It is true, and nothing checked it, so it would have gone on reading as true after it stopped being. Proved instead: CopyPlanner answers both codecs before the Encode branch and its fallback draws from ContainerCapabilities.encodableVideo, which contains neither, so a sweep over every spec the planner can be handed asserts no Encode plan carries COPY or NONE. Counters guard the sweep, because `as? Encode ?: let` asserts nothing at all for a Drop or Copy plan. The audio sibling claim did not survive intact. "MP3 and FLAC have no Android encoder; the router routes them to FFmpeg" is true and incomplete: one rule, `audioEncode !in MEDIA3_AUDIO`, diverts Vorbis by identical logic, so three of the six encodable codecs never reach the table. VORBIS -> AUDIO_VORBIS is a correct mapping for a request Transformer is never given. The arm stays -- a right answer in unreachable code costs nothing -- and the comment now says so. The tables are asked of the router's decisions rather than of its codec sets, because the comments claim behaviour and a set can be right while the rule reading it is wrong. Both move to an internal companion object so a JVM test can reach them without constructing an engine, which would start a real HandlerThread to answer an enum lookup; #57's precedent, and the JVM test source set is a friend of main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
fd2bb1d889 |
Test the three MediaProbe helpers nothing else would catch
MediaProbe's MIME table, its image-demuxer rule and its Int reader are pure
functions with no test at all, and each fails silently rather than loudly.
shortName falls through to substringAfter('/') and reports a plausible-looking
string that CodecNames may or may not still recognise, so a dropped arm turns a
stream-copyable file into a re-encode. isImageFormat is checked before anything
else in classify, so a wrong answer overrides both probes. intOr's runCatching
is the only thing standing between a Float frame rate and losing every other
track property the loop had read.
Widen the three to internal, as #57 did, and say in each KDoc why the shape is
what it is -- the _pipe suffix is not a substring test because yuv4mpegpipe is
raw video, and getInteger casts rather than coerces.
Every format name asserted came from ffprobe rather than from memory: a picked
.png reports png_pipe, a .jpg reports jpeg_pipe, a .y4m reports yuv4mpegpipe.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-dev
|
b9abe85580 |
Say three where a third test joined, and stop the name claiming to be exact
#80 added SafPickerRoundTripTest's rotation case to @FailsOnEmulatorApi37, because a real rotation aborts the framework on android-37.0. Three tests carry the marker now -- two in Media3EngineTest, one in SafPickerRoundTripTest -- and five statements still described two. Four were counts, and wrong: status_check.yml "notAnnotation removes the two tests that do not pass" status_check.yml "The two API 37 tests the gating row above excludes" CLAUDE.md "the gating leg runs the other 55" (59 - 3 = 56) CLAUDE.md "do not read a green run as evidence those two tests pass" The fifth was worse, because it was not a count. The advisory job's header justified its name with an invariant: "It is named for WHAT IT RUNS, deliberately. Both tests drive a full H.264 -> H.265 hardware transcode through Media3Engine" The rotation case drives no transcode. So the comment did not merely miscount -- it asserted a property of the job's contents that had stopped being true, and that property was the entire argument for the name. The name is unchanged, deliberately, and the header now says so instead of implying the question never arose. This is not a required context, it is red on every PR by design, and it is one people have learned to look for; renaming a check costs more than the imprecision does. What replaced the invariant is the honest rule: THE MARKER IS THE DEFINITION, NOT THE NAME -- this job holds the tests that cannot pass on the API 37 emulator image, whatever their subject. Two things stay as they were because they are still true. "the two Media3EngineTest cases that pass here" is correct: that class has four tests and two carry the marker. And the decoder theory is still a claim about the Media3 pair alone, so it now says so rather than being read as covering a rotation failure it has nothing to do with. Nothing about the job's behaviour changes: same name, same continue-on-error, same marker, same selection on both rows. Verified: yaml parses, five jobs, matrix still 33/34/35/36/37. The check to re-run when a test next joins or leaves the marker, which is the event that broke this twice: grep -rn "@FailsOnEmulatorApi37" app/src/androidTest --include='*.kt' | grep -v import | grep -c FailsOn It must equal the number every corrected comment states. It is 3. Closes #81. |
||
|
JMR-devandClaude Opus 5
|
3925f1aa9f |
Re-find the picker node when it goes stale, and re-measure API 37
CI found a flake this workstation could not, and fixing it overturned half of what
the previous commit recorded about API 37.
THE FLAKE. UiObject2 caches the AccessibilityNodeInfo it was found with, and
DocumentsUI is still settling when a node first appears -- its list rebinds, the
roots strip lays out, a window animates. If the node is replaced in that gap,
click() throws against the handle rather than missing the target:
androidx.test.uiautomator.StaleObjectException
at androidx.test.uiautomator.UiObject2.getAccessibilityNodeInfo(UiObject2.java:1042)
at androidx.test.uiautomator.UiObject2.click(UiObject2.java:526)
at SafPickerRoundTripTest.pickTheFixture(SafPickerRoundTripTest.kt:223)
It is not intermittent on a COLD emulator -- CI hit it on API 33, 34 and 35, every
one of them, on the first run. It never appeared here because the local emulator had
been warm for an hour. tapPickerNode now re-finds the node and taps again, three
attempts. That retries acquiring a handle to a node that has to be there anyway:
every attempt still goes through awaitPickerNode, which fails outright if it is
absent, so the MIME mutation's bite is untouched. Verified with `pm clear
com.google.android.documentsui` between runs, five for five green on API 34.
AND THE CORRECTION IT FORCED. The previous commit marked the whole class
@FailsOnEmulatorApi37 on the strength of two measured failures. One of them was
this bug. Re-measured with the fix, one method per fresh android-37.0 emulator:
thePickedInputSurvivesARealRotation INSTRUMENTATION_ABORTED:
System has crashed.
pickingAFileThroughTheSystemPickerFillsInTheFileCard PASSED
So a rotation, which rebuilds every surface at once, is what the gralloc mapper does
not survive; starting another app's activity is not. The marker moves to the one
method that earned it, and the picker test runs on the gating API 37 leg like
anything else. The workflow comment, run-e2e.sh and the doc all say that now.
The lesson is worth more than the measurement, and the doc keeps it: an annotation
is a claim about an IMAGE, and a broken test makes every image look broken. Both a
framework abort and a stale node read as "the run fell over". Re-measure after
fixing a test before deciding what the platform did.
Also measured rather than assumed, since it is what keeps the gating leg green: the
runner's annotation filter honours a class-level marker, expanding it to every
method. On API 34, `annotation=` selected exactly 4 tests (2 Media3EngineTest + 2
here) and `notAnnotation=` selected 55 with neither of these in it. CI's own gating
API 37 leg then reported 55 / 0 on the previous push. That is why moving the marker
to a single method is a narrowing rather than a repair.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
a3c835b7c9 |
Keep the picker test off the API 37 gating leg, having measured why
The API 37 emulator images abort surfaceflinger inside the guest's Gralloc5 mapper,
init SIGKILLs zygote with it, and the framework restarts under the run. run-e2e.sh
and the CI leg disable SystemUI to remove the trigger -- but that removes the IDLE
one, RegionSamplingThread's nav-bar luma sampling. Driving DocumentsUI and rotating
the display are not idle. They are the first things in this suite that generate
surface traffic of their own.
Both tests were measured on android-37.0 under swangle_indirect with SystemUI
disabled and verified quiet, and measured SEPARATELY -- inferring the second from
the first is the mistake docs/api-37-emulator-crash.md opens by correcting. They
fail in the two shapes a framework restart produces:
thePickedInputSurvivesARealRotation
INSTRUMENTATION_ABORTED: System has crashed.
Expected 59 tests, received 50
(5 hasReadColorBufferDma aborts; the framework dies DURING the test, so six
later tests never run and the XML carries a failure with no text at all)
pickingAFileThroughTheSystemPickerFillsInTheFileCard
androidx.test.uiautomator.StaleObjectException
at androidx.test.uiautomator.UiObject2.click(UiObject2.java:526)
(3 aborts; the picker's root node was rebuilt between finding it and tapping it)
Both pass on API 33 and API 36 locally -- whole suite, 59/0/0/2 on each -- which is
the same evidence pattern that made the Media3EngineTest pair the image rather than
the app.
So the class carries @FailsOnEmulatorApi37 and runs on the advisory leg.
THREE PLACES SAID "nothing in this suite touches system UI", and that is what makes
the SystemUI-disable deviation defensible. It is no longer true of the suite, and all
three are corrected rather than left to rot -- the workflow comment, run-e2e.sh's
header, and the doc. The rule they state is being APPLIED, not broken: the thing that
depends on system UI is excluded from the leg that cannot be trusted for it.
Two consequences stated rather than left to be discovered:
- run-e2e.sh applies no annotation filter, unlike CI, so a local `run-e2e.sh 37`
reports these two on top of the Media3 pair AND DOES NOT FINISH. Its totals come
back short and which later tests ran is arbitrary. The summary row now says so;
it previously promised "exactly two failures", which would have read as a
regression in someone else's diff.
- The advisory job is still named "E2E API 37 Media3 hardware transcode", and half
of what it now runs is neither. Renaming a check touches branch protection, so it
is deliberately not done here; the doc records the staleness and the revisit
trigger now says the marker covers two unrelated bugs that can go green apart.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
650ca8fca3 |
Pick a file the way a user does, then rotate the phone
Two things nothing in this repo asserted, and they are one test class because
separately the second one asserts nothing new.
THE PICKER. ConverterScreen opens SAF with a MIME filter, and a filter is a thing
that can hide the user's file. Narrow it and the app still builds, still renders,
and still passes every JVM test -- the user taps "Choose file" and gets an empty
picker. The round trip now runs for real: DocumentsUI is driven with UiAutomator to
a fixture root, and the app is asserted to come back with the file.
The file card's name is not the only assertion, because a name proves less than it
looks: it comes from a metadata query, which a URI with no read grant answers just
as well. The "Container: MP4" detail row only appears once something has opened the
file and read its header, so it is what says the picker handed back a URI the app
can USE.
THE ROTATION. MainActivity declares no configChanges, and ConversionViewModel holds
the picked file in a plain MutableStateFlow with NO SavedStateHandle behind it.
Nothing persists it. The only thing that carries it across a rotation is the
ViewModelStore the Activity retains -- which no test anywhere asserted.
Two guards run before that assertion, because both ways it could pass while proving
nothing are silent: the display rotation really changed, and MainActivity really was
a different instance afterwards. Without the second one this is a recomposition test
wearing a rotation's name.
MUTATIONS, RUN RATHER THAN ASSERTED, on a local API 34 emulator.
Narrowing the filter to arrayOf("application/x-lmc-no-such-type") takes the fixture
root out of the picker entirely -- DocumentsUI matches the request against
Root.COLUMN_MIME_TYPES and drops roots that cannot answer -- and both tests fail:
java.lang.IllegalArgumentException: the system picker never showed
BySelector [TEXT='\QLMC R38 fixtures\E']
Making the ViewModel composition-scoped fails ONLY the rotation test:
androidx.compose.ui.test.ComposeTimeoutException: Condition (a node tagged
converter.fileCard.name exists) still not satisfied after 30000 ms
and :app:testDebugUnitTest stays BUILD SUCCESSFUL under it. That divergence is what
#64 exists to establish and what its own comment doubted; the PR body has the
verdict and why the doubt was reasonable.
THE PROVIDER HAD TO BE JAVA. It is the only Java file in the module. A
manifest-declared provider is a component of the instrumentation PACKAGE, so the
system starts a plain org.libremediaconverter.test process for it with only the test
APK on its dex path -- and the test APK is built without the Kotlin stdlib, because
the app APK has it and duplicating it is what checkDebugAndroidTestDuplicateClasses
prevents. The Kotlin draft died on its first query:
java.lang.NoClassDefFoundError: Failed resolution of: Lkotlin/jvm/internal/Intrinsics;
at org.libremediaconverter.saf.FixtureDocumentsProvider.queryDocument
The compiler emits that reference for the null checks on nearly every function, so
no Kotlin dialect avoids it. Same reason nothing in that file imports androidx.
No new test tags: CHOOSE_FILE, FILE_CARD_NAME and detailRow already named both ends.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
b18f45def7 |
Give the system file picker something to pick
Nothing in either source set drives SAF as a picker. The only SAF coverage is the publish side, in OutputPublisherPublishTest, against hand-written ContentProvider fakes -- so the launcher wiring in ConverterScreen, the MIME filter it passes, and the grant that comes back have never been executed by a test. Driving the real picker needs three things this repo did not have. UiAutomator, because DocumentsUI is another process. Compose's matchers stop at this process's composition and Espresso's stop at its view hierarchy; neither can see or tap a window belonging to another package. It FLOATS, at "2.+", which is the same argument the catalog already makes for work and lifecycle rather than a new one: androidx.test.uiautomator is inside floatedGroupPrefixes, so the componentSelection guard makes "+" mean "newest RELEASED", and that is load-bearing here -- this library publishes 2.4.0-alphas above its stable, so without the guard the float would be a pin to a prerelease. Resolved to 2.4.0 (released) on debugAndroidTestRuntimeClasspath, checked rather than assumed. It is deliberately NOT pinned alongside ktlint/detekt/JaCoCo/Robolectric: those are pinned because a new rule or a new runtime changes the verdict on files nobody touched. UiAutomator has no verdict -- it taps what a selector names, and a selector that stops matching is this repo's test to fix, in a diff that explains itself. The "2." rather than a bare "+" is the one thing held back: a major is where the selector API would be free to change under exactly that assumption. A DocumentsProvider, because DocumentsUI does not browse a filesystem -- it lists what providers offer it. Writing a file into Downloads would have worked and tested less: the fixture root declares Root.COLUMN_MIME_TYPES, and DocumentsUI filters the drawer by it, which is what gives the MIME filter a mutation with a shape rather than "one file among the hundreds in Downloads was not listed". Its contents are also exactly one file, where a shared directory accumulates whatever earlier runs left behind. And the first AndroidManifest.xml this source set has ever had, to declare it -- a ContentProvider is instantiated by the system and cannot be registered from test code. In androidTest rather than src/debug so it is installed by the instrumentation APK only, and never appears in a developer's own file picker. Two things worth knowing before editing either file. XML comments cannot contain "--", which the manifest's first draft failed the build on; and "*/" inside a KDoc closes the comment, which the provider's did. Both are silent in review and loud in the build. No test yet, and no new test tag: TestTags.Converter.CHOOSE_FILE and FILE_CARD_NAME already name both ends of the round trip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
02555ceb91 |
Ask each join state what it lets the user do next
`JoinScreenContent` decides the whole join UI in one `when`, and until R38.5 gave it a state
parameter nothing could ask it anything: `Waiting` follows a denied foreground start and `Joined`
follows a finished `ConcatWorker` run, so neither is reachable by driving a real `JoinViewModel`.
`JoinScreenContentTest` used that seam to prove it exists, on one state. This is the matrix behind
it -- seven states, each pinned to the affordance it offers and the callback that affordance is
wired to, asserting on the value handed back rather than on something merely having fired.
Two of the thirteen assert things nothing else in the suite has ever asked.
The rows are read back sorted by their position on screen and compared as an ordered list. A join
is the one flow where the order of the inputs is the content of the output -- the empty state
promises "in the order you want them" -- and `JoinLeafTagsTest` proves only that a row tags itself
with the file it shows, which a reversed list would satisfy just as well.
The progress bar is asserted to be indeterminate, not merely present. It carries no percentage on
purpose, because FFmpeg reports progress against one input's duration and that means nothing across
a concatenation; the converter screen's bar is determinate, so "there is a bar" is exactly the
assertion that would let a fabricated percentage land here unnoticed.
Three mutations, each reverted after:
- `Text(s.message)` -> `Text("")` in `Failed`: "a failed join renders the message it carries" fails
with `could not find any node that satisfies: (Text + InputText + EditableText contains 'The
second file has no audio track, so joining stopped.')`.
- `when (s.strategy)` -> `when (ConcatStrategy.STREAM_COPY)` in `Joined`: "a re-encoded join says
the files differed" fails on the copy for the branch that no longer runs.
- `s.inputs.forEach` -> `s.inputs.reversed().forEach` in `Ready`: the ordering test fails
`expected:<[join.fileRow:intro.mp4, join.fileRow:middle.mp4, join.fileRow:outro.mp4]> but
was:<[join.fileRow:outro.mp4, join.fileRow:middle.mp4, join.fileRow:intro.mp4]>`.
Test-only: no file under `app/src/main` changes, and no tag is added to `TestTags`, because every
string these states render is either already tagged or unambiguous as text. The typographic
characters in the asserted copy -- U+2026 in "Joining N files...", U+2014 in the `Joined` and
Paused lines -- were checked byte-for-byte against `JoinScreen.kt` rather than retyped; an ASCII
lookalike compiles and then quietly matches nothing.
Closes #63.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
2f3f461cc1 |
Say what each conversion state puts on screen, and what it withholds
The screen's state machine had a seam and no matrix behind it. Every arm of the `when` returns `Unit`, so an arm can render anything at all and still compile -- a button offered where it cannot work, a state's own data never reaching the node meant to show it, an affordance wired to the wrong callback. The leaf tests cannot see any of that: they compose `FileCard`, `AdvancedPicker` and the three pickers directly and never hold a `ConversionState`. The arm worth guarding most is `Ready`'s `enabled = validation.isValid`. The Advanced picker deliberately lets an impossible container / codec pair be selected, so that one expression is all that stands between an invalid spec and a job that cannot succeed. `enabled = true` compiles, renders an identical screen apart from one colour, and passed the whole suite before this. Callbacks are asserted over the complete log rather than one at a time, so a case reads "this one fired and nothing else". A bare "the callback ran" check stays green on an arm that fires the right callback for the wrong reason. The routing chip needed a tag to be locatable at all: its text comes from the finished job, so a text matcher would have to name a routing explanation the screen does not own. That is the only production change here. Not asserted, deliberately: `Failed`'s error colour, which Compose publishes nowhere in the semantics tree; and the three absent `FileCard`s, which are compile-guarded -- `Idle`, `Saved` and `Failed` carry no `input` -- so those lines state the intent without being what enforces it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
46ad95350b |
Give both screens somewhere for a state to come from
`ConverterScreen` and `JoinScreen` each inlined their whole `when (state)` inside the public entry point, and state arrived only as `viewModel.state`. That left four of the twelve state branches across the two screens with no test that could ever reach them: driving a real ViewModel needs a WorkManager and a media probe in the constructor, and even then `Waiting` follows a denied foreground start and `Converted`/`Joined` follow a worker run that has already succeeded. So the `when` moves into `ConverterScreenContent` and `JoinScreenContent`, which take the state, the settings, the validation and an actions holder. The entry points keep the three launchers and `collectAsStateWithLifecycle` and nothing else. The callbacks travel in `ConverterActions` / `JoinActions` rather than as loose parameters because detekt's `LongParameterList` sits at its default threshold of six and `config/detekt/detekt.yml` does not relax it for `@Composable` -- `AdvancedPicker` already sits exactly on it. Twelve flat parameters would turn a clean detekt run red; data classes are exempt from the rule. Nothing else changed. The body was cut and pasted rather than retyped, so the U+2026, U+2014 and U+00B7 characters the leaf tests match on are the same bytes, and `is Idle -> Unit` in the nested `when` -- permanently unreachable, and deliberately kept -- survives the move. The diff stops above `FormatPicker` in one file and above `FileRow` in the other, which is why the leaf suites #57-#60 landed pass unedited: every one of them composes a leaf directly and none references either entry point. The two new tests are the bite, one per screen and one per direction of the seam: a `Converted` / `Joined` state renders Save, and tapping Save hands back the name the finished job chose. The state matrix itself is #62 and #63. |
||
|
JMR-dev
|
3d55004286 |
Count the Robolectric tests, which JaCoCo has never counted
The three #52 test PRs landed 56 new tests and the coverage figure moved 29.8% -> 29.7%. That looked like the tests being worthless. It was the measurement. Robolectric loads every class it touches through its own sandbox classloader, and those classes arrive with no source location. JaCoCo skips no-location classes unless told otherwise, and nothing here told it. So not one Robolectric test has ever contributed coverage in this repo -- and Robolectric is what exercises the framework edge: both workers, the publisher, both ViewModels, every Compose screen. Same commit, same 335 tests, same 0 failures, only the block below added: LINE 652/2194 29.7% -> 1519/2194 69.2% BRANCH 425/1424 29.8% -> 758/1424 53.2% OutputPublisher 0.0% -> 97.5% MainActivityKt 6.8% -> 86.4% ConversionViewModel 0.0% -> 85.4% ConverterScreenKt 6.6% -> 62.8% The discriminator, so this is not cargo cult: inside ConverterScreenKt, `describe` is the one non-Composable and is exercised by a plain JVM test. It reported 8/8 covered while every @Composable in the same class reported 0 -- including ones whose mutations demonstrably failed the build when reverted. Across files the split is exactly Robolectric-vs-not: StagingSweep, tested purely, 100%; OutputPublisher, ConversionViewModel and FailureOutcome, tested under Robolectric, 0%. `excludes = listOf("jdk.internal.*")` is not decoration. Without it JaCoCo walks JDK-internal classes Robolectric has no location for either and the test JVM dies rather than reporting a number. CLAUDE.md's coverage bullet is rewritten, because it was wrong twice over. The figure was an artifact, and the explanation attached to it -- that coverage fell as the suite grew from 11 test files to 43 because the denominator outran the numerator on framework-edge code "the JVM cannot reach" -- described a cause that does not exist. The JVM reaches that code fine. The new tests were disproportionately Robolectric, so each one added denominator and no numerator: the measurement was punishing precisely the tests that were hardest to write, and the conclusion drawn from it was that writing them had not helped. Mutation, run both ways on this branch: remove the block and jacocoTestReport collapses back to 29.7% / 29.8%; restore it and it returns to 69.2% / 53.2%. Two things that were true stay true. There is still no coverage gate, and a floor still needs a settled baseline -- this one just moved 39 points in one build change. And "re-measure before quoting it" was already written down; following it is the only reason this was found. |
||
|
JMR-dev
|
7b578c1ccf | Merge branch 'main' into docs/instrumented-tests-correction | ||
|
JMR-dev
|
3c5a37fd3c | Merge branch 'main' into test/r38-2-filecard | ||
|
JMR-dev
|
4ea5afefe1 | Merge branch 'main' into test/r38-4-advanced-picker | ||
|
JMR-dev
|
3d51fefeff |
Say where instrumented tests run, instead of where they used to not run
CLAUDE.md carried three claims about instrumented tests. All three were false, one of
them contradicted a paragraph forty lines below it in the same file, and a subagent
working on #58 hit the contradiction and had to stop and flag it rather than trust the
project's own instructions. That is the cost being paid here: this file is what every
contributor and every agent reads first.
"Instrumented tests do not run locally" -- they do, API 33-36, since
|
||
|
JMR-dev
|
6cd17f25aa |
Pin shellcheck, because the unpinned one disagreed with the local run
The step added in the previous commit went red on its own PR, and the reason is the one
CLAUDE.md already gives for pinning ktlint, detekt and JaCoCo: "a new rule in a linter
makes files nobody touched stop passing, so CI goes red on a PR whose diff cannot explain
it." Here it was not even a new rule, just a different version of the same tool.
The runner's ambient shellcheck is 0.9.0. The container used to check locally was 0.11.0.
They disagree about how to report `on_signal`, which is installed as the INT and TERM trap
eleven lines below its declaration and so is never called by name:
0.11.0 SC2329, once, on the function declaration -- "never invoked"
0.9.0 SC2317, seven times, one per command in the body -- "appears to be unreachable"
The disable directive named SC2329, so 0.11.0 was silent and 0.9.0 reported seven findings.
Nothing about the script was wrong; the local check simply was not the check CI ran.
Two changes, because either alone still leaves a way to be surprised:
- CI runs shellcheck from an image pinned by digest, so an upgrade is a line in this
file that someone chose, not something that arrives on a Tuesday. The version is
still printed, so a finding out of nowhere can be tied to that line.
- The directive names SC2317 and SC2329 both, so a contributor whose distro ships 0.9.0
gets the same answer locally as CI gives. Verified against both images: clean under
0.9.0 and clean under 0.11.0.
CLAUDE.md now says to check with the pinned digest rather than with whatever is installed,
which is what would have caught this before the push.
|
||
|
JMR-devandClaude Opus 5
|
fea88a281f |
Say in tests what the file card says when it does not know
"Size unknown" is the line a stream fixing D5 reported as untestable. It is two assertTextEquals calls, and it needed two rather than one: the size line renders independently of the probe, so it is asserted with a probe and without one. That independence is the contract, and a test of the probed case alone would leave the branch a user hits first -- the card is on screen before the probe finishes -- unguarded. The rest of the card degrades in words the same way, and none of it was covered: the four InputKind branches, "No video track", "No audio track", describeVideo's "Unknown", and the two `> 0` guards that drop the dimension and length rows rather than printing 0 and 0:00. Each guard gets a case on both sides, because the present side alone stays green when the guard is deleted -- what deleting it produces is "Size: 0x0" and "Length: 0:00", the same invented-measurement defect as "0 B". The four pure helpers go in a plain JVM class beside it, with formatBytes pinned at each threshold and one byte below it. A `>=` quietly becoming a `>` is only visible from a value sitting exactly on the boundary. Two things the issue could not have known: - Its second acceptance criterion, "delete the return@Column and watch the Reading... test go red", cannot happen -- it does not compile. The early return is what smart-casts `probe` non-null, so ten uses below it fail with "Only safe (?.) or non-null asserted (!!.) calls are allowed on a nullable receiver". The exit is enforced by the compiler, not by a test. Both compilable regressions someone would land instead are covered and were run red. - CodecNames.describeAudio has no UNPARSEABLE arm, unlike describeVideo, so it answers the raw sentinel rather than "Unrecognised". Unreachable today, because the UNPARSEABLE kind renders the explanatory line instead of rows. Left alone; recorded on the PR for R38.5. The divider's absence is not asserted and cannot be: Material 3 renders it as a Box with no semantics modifier, so it contributes no node. What is asserted is everything it precedes, plus the card's child count. The class KDoc says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
7c69d0699a |
Hold the Advanced panel's gate, and the error card outside it
`AdvancedPicker` is the one leaf on the converter screen that carries its own state, and `ValidationError` is deliberately invoked after the `AnimatedVisibility` that gates the chip rows -- so an invalid spec explains itself and offers one-tap fixes while the section is collapsed. That is the only route out of an invalid spec for a user who never opened Advanced, it was completely untested, and folding the two `if` blocks into one is a plausible tidy-up that compiles. `AdvancedPickerTest` covers the gate in both directions, clicks each of the four colliding chip labels through its own row tag, and does every assertion about the error card with the toggle untouched. `AdvancedPanelSavedStateTest` is the `DestinationSaverTest` split for `expanded`: `StateRestorationTester` saves into an in-memory map, so it proves `rememberSaveable` is in use and nothing about the representation. Driving a real `SaveableStateRegistry` shows the picker saves the `MutableState` itself rather than the `Boolean`, which only survives a rotation because `mutableStateOf` on Android returns a `Parcelable` one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
baaaa934e0 |
Check the shell, and stop one-off issues falling off the board
Two gaps, both found the same way -- by something going wrong quietly.
`gh issue create` does not touch the project board. The issue is created, carries its
labels, and is invisible in the Kanban, which looks exactly like a ticket nobody filed.
On 2026-08-24 eight issues filed as a scripted batch all reached the board and one filed
as a one-off minutes later did not; it surfaced only because someone went looking for it.
A batch carries the board step inside its loop. One-offs are where it slips, so
tools/github/file-issue.sh is for one-offs.
Three things it does that a two-command shell snippet would not:
- Resolves the project, Status field and option ids BY NAME, every run. Caching them
is the obvious optimisation and the wrong one -- a renamed or reordered column would
then have this writing a stale id into the board with no error anywhere.
- Reads the item back. A mutation returning 200 says the request was accepted, not that
the board shows what was asked for; the read-back is the only step that checks the
claim this script exists to make. It is a GraphQL query because REST cannot do it --
the `fields` array REST returns on a project item carries Title and nothing else, so
a REST-only check reports every item's Status as unset.
- Exits 3, loudly, with the issue number on a line of its own, when the issue was
created but the board step failed. That exact combination is the failure being
prevented; it must never be the quiet path.
Shell was the other language here with nothing checking it -- four scripts, one of them
the CI entry point. shellcheck now runs in the Static analysis job over
`git ls-files '*.sh'`, so a script added later is covered without editing the workflow,
and it runs at full severity with `info` included.
That raises two findings today and both are the tool being wrong, so both are answered
with a targeted `disable` carrying its reason rather than by lowering the severity:
run-e2e.sh's `on_signal` is reported as never invoked when it is installed as the INT and
TERM trap eleven lines below it, and the `$names` inside file-issue.sh's queries are
GraphQL variables that must not expand -- expanding them would send the shell's idea of
$owner to the API instead of declaring a parameter. A blanket --severity=warning would
have hidden both, and the next real finding with them.
The gradle step gains `if: !cancelled()` so a shellcheck failure cannot cost the
ktlint/detekt/lint lists -- the same reason that step already passes --continue.
Not covered, deliberately: shellcheck here reads .sh files, not the inline `run:` blocks
in the workflows, where a good deal of this repo's bash actually lives. actionlint does
read them, and finds one pre-existing info-level issue in build.yml. Wiring it in means
pinning a container digest, because every action here is pinned by SHA and actionlint's
usual installer is a curl-pipe-bash off a moving branch. Its own ticket, not this commit.
|
||
|
JMR-devandClaude Opus 5
|
2bed40d080 |
Hold the three pickers to the constant they hand back
The format, quality and engine pickers are the same dozen lines with a
different enum substituted, and both ways they can go wrong are silent.
An onClick that closes over the picker's `selected` parameter instead of
the chip's own entry returns one constant for every chip; an inverted
`entry == selected` lights every chip but the right one. Neither throws,
neither changes the labels on screen, and a test that only asserted the
callback ran would pass over the first of them.
So each click test presses every chip in the row and compares the whole
recorded list against `entries`, which makes the constant load-bearing
rather than the click count, and each selection test asserts over every
chip rather than only the one that should be lit.
Verified by mutation, not by the suite going green:
- `onSelect(format)` -> `onSelect(OutputFormat.MP4_H264)` fails with
`expected:<[MP4_H264, MP4_H265, WEBM_VP9, ...]> but was:<[MP4_H264,
MP4_H264, MP4_H264, ...]>`
- `format == selected` -> `format != selected` fails both format
selection tests on `Selected = 'true'` for a chip that should not be
- `onSelect(preference)` -> `{}` fails with `expected:<[AUTO,
PREFER_HARDWARE, FORCE_SOFTWARE]> but was:<[]>`
- `selected.description` -> `QualityTier.FAST.description` fails the
quality prose test on the missing BEST line
Labels are read off the enums so a reword cannot redden this file for
the wrong reason. `EnginePreference` has no label of its own, so the
screen's own `label()` supplies that set. The one display literal with
no symbol behind it, the custom-spec line, was copied out of the source
byte for byte because it holds a U+2014 that would fail silently if
retyped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
df2e42a2b3 |
Name the screen leaves, and give the tests a tag vocabulary
Kotlin `private` on a top-level declaration is file-scoped, so every leaf composable in the two screens was invisible even to the JVM test source set, which is a friend of main. The only three declarations src/test could name were ConverterScreen, JoinScreen and AppRoot -- there was nothing to write a test against, which is why #52 could not be started as filed. `internal` is the same choice MainActivity already documents for Destination: the unit tests can name it, and it stays invisible to anything outside the module. Eleven declarations in ConverterScreen and FileRow in JoinScreen. The tag table is the other half. Tests reference a symbol rather than a literal, which is what keeps the five children that follow independent: "Cancel", "Start over" and "Save file" are each rendered by both screens and by more than one state branch, so rewording one would otherwise redden several PRs at once and no diff would explain why. There were zero testTag, semantics or contentDescription calls anywhere in main before this. Every tag is applied inside main. A tag a test hands down as a Modifier proves only that the test set it -- it would survive the affordance losing its own tag entirely, which is the vacuous shape CLAUDE.md records nine of in one review. That is why FileRow and DetailRow derive theirs from data they already hold rather than taking an index from the call site. TestTags is public rather than internal, and R38.8 is the reason. It reads the table from androidTest, and whether that is a friend source set of main under AGP 9 had no in-tree answer -- nothing referenced a main internal from there. Settled by compiling one: it is a friend, so internal would work today. Public anyway, because that friendship is AGP wiring rather than something this project states, and the KDoc now carries the measurement so nobody has to repeat it. Smoke tests cover each leaf: it renders, and its tag resolves to exactly one node. Counting rather than asserting existence is deliberate, since a duplicated tag fails differently depending on which finder a later test happens to use. The state-branch tags -- Convert, Cancel, Save file, Start over, the progress bars -- have no bite yet: reaching a branch needs the state seam R38.5 extracts, and the state matrix is R38.6/R38.7 by design. Five mutations, all red on the named test alone: FORMAT_CHIPS deleted, FILE_CARD_NAME deleted, detailRow's tag no longer derived from its label, FileRow's no longer derived from its name, and JOIN_MORE given JOIN's value -- the last caught only by the uniqueness check, which is what it is for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
64c1a60a97 |
Drain the escaped coroutine error before the next test starts
`ConversionViewModelProbeFailureTest.an OutOfMemoryError is not swallowed` deliberately lets a real error escape `viewModelScope.launch`, which has no exception handler by design -- the ViewModel's KDoc says an OOM raised in the probe should reach the thread's handler and take the process down. On the JVM something else takes it. kotlinx-coroutines-test installs a process-wide collector for uncaught coroutine errors, keeps whatever it catches, and hands the backlog to the next `runTest` that starts, which throws UncaughtExceptionsBeforeTest. Every Compose test is a `runTest`: that is how `createComposeRule` runs a composition. So the error lands on an unrelated test in an unrelated file, and the message names neither the test that caused it nor the error's origin. Nothing has hit it yet only because the sole Compose test in the repo happens to run before the ViewModel one. R38 adds six more Compose classes in exactly the two packages that surround it, and the first two of them made the suite fail in two different files on two consecutive runs of identical, green code -- the throw is on a real Dispatchers.IO thread, delivered after the state assertion that ends the test responsible, so which class catches it is a race. Drain it where a Compose rule is built. A @Before cannot: the rule's `runTest` wraps the statement that calls it, so it has already thrown. @BeforeClass cannot either, because Robolectric runs it outside the sandbox classloader, where the collector is a different object. Constructing the rule is early enough, since JUnit builds a fresh test-class instance -- and every @get:Rule field on it -- before evaluating any rule. This is containment, not the cure. The cure is a seam: give the probe hop an injectable dispatcher the way the constructor already does for cleanupDispatcher, so the error has somewhere to land. That is a production change and deserves its own commit. kotlinx-coroutines-test was already on the unit-test classpath through compose-ui-test-junit4; it is declared now because a file imports it. Pinned, like robolectric and the linters: org.jetbrains.kotlinx is not one of the groups the prerelease guard covers, so a float here would be free to take a milestone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
225ecdd7e6 |
Split the API 37 leg so the part that works can gate
CI has never run the API level this app targets. The reason it did not was never "API 37 is untestable" -- it was that two tests fail on the emulator image, so one row would be permanently red or permanently allow-listed. This splits that row instead of choosing between those two. E2E API 37 gates. It runs 55 of the suite's 57 instrumented tests and must be green. E2E API 37 Media3 hardware transcode runs the other two, reports, and never blocks (continue-on-error). Both are driven off ONE marker, @FailsOnEmulatorApi37: the gating job passes notAnnotation, the advisory job passes annotation. Two lists would drift, and drift is silent in both directions -- a test that ends up in neither job reads as green. Excluding by class was not an option either: Media3EngineTest has four tests and two of them pass here, so notClass would have thrown away real coverage. The advisory job is named for what it runs, not for what we think is wrong. Both its tests drive a full H.264 -> H.265 hardware transcode, which is what distinguishes them from the two Media3EngineTest cases that pass -- those never decode video. The goldfish-decoder theory sits in a comment inside the job, where it can be corrected without renaming a check people have learned to look for; docs/api-37-emulator-crash.md keeps measurement and inference apart. The SystemUI disable moves into .github/scripts/e2e-run.sh behind E2E_DISABLE_SYSTEM_UI, unset everywhere but the two API 37 jobs, so the other four legs run byte-identical commands -- the same shape as E2E_EXTRA_GRADLE_ARGS. It runs BEFORE the streamed logcat starts, deliberately: `adb shell stop` would end that logcat and nothing restarts it, so a disable placed after it would cost the leg its diagnostics for the part of the run that matters. The body is probe v2 from api37-debug.yml -- the version measured 4/4 -- not the older one-round form: three rounds, waits for system_server to actually be gone, verifies against `pm list packages -d`, and requires a 45 s window with zero new aborts. The weaker probe reported success on a run that then started SystemUI eight more times. The caveat is written next to the row rather than left implicit: this leg runs with SystemUI disabled and the framework restarted under it, a device configuration no other leg and no Pixel run uses. Anything that touches system UI must not trust it, and the Pixel check before each release is still the only API 37 run with SystemUI intact. docs/api-37-emulator-crash.md's "So should CI take API 37?" said no on three reasons. Two were claims about CI that had never been measured; the section now carries the eight runs that measured them, and the third reason is what the split answers. docs/local-emulator.md and api37-debug.yml's header carried the same "the matrix stops at 36" claim and are corrected with it. CLAUDE.md is left alone deliberately -- its "CI's matrix therefore stops at API 36" clause is now false, and that correction is parked in the doc's existing "Correction owed to CLAUDE.md" section, where two others are already waiting. Making E2E API 37 an actually-required check is a repository-settings change and must come after this is on main: adding a required context that does not exist on the default branch blocks every PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
b3a705e3da |
Measure the API 36 control and record what CI cannot measure
Three additions to docs/api-37-emulator-crash.md, all from a CI investigation run through .github/workflows/api37-debug.yml. A third measured bullet: API 36 against API 37, back to back, same two tests, same renderer, same SystemUI-disable path. 37.0 fails both on c2.goldfish.h264.decoder (32660148155); 36 passes both in 4.603 s with the same decoder in its logcat (32660152961). That falsifies "the stripped configuration is what breaks these tests" -- a reading the other measurements never addressed, because they all compare against a device that still had SystemUI. It carries its two uncontrolled variables rather than dropping them: API 36's framework restart happened with zero aborts logged where API 37's had two, so a restart under an active abort loop is still uncontrolled; and the images differ on the encoder side, which is a second reason "broken h264 decoder" is the wrong shape of claim. The decoder-mechanism bullet is unchanged and still labelled inference. This adds a measurement next to it; it does not retract anything. The intact-SystemUI counterfactual is unmeasurable on a GitHub runner, and now says why. Seven dispatches, zero verdicts, with a mechanism rather than bad luck: while the framework crash-loops the guest cannot reliably create per-user private directories, so an app installed during the loop has no cache dir and the fixture copy dies in @Before before any codec exists. googlesdksetup and nexuslauncher hit the same thing. The result XML masks it behind an UninitializedPropertyAccessException in tearDown, which reads as a defect in this repository and is not one. Abort cadence corrected. "Roughly every 20 s" was the watchdog's sampling interval, not the cadence: measured gaps are 20-90 s, median 60-70 s, three to five per run, with sys.boot_completed held at 1 throughout. The wrong figure lived in api37-debug.yml's own comments, so that line is corrected too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
acc71bcaee |
Verify the SystemUI disable instead of trusting what pm reported
Four dispatches of one configuration -- API 37.0, swiftshader_indirect,
SystemUI disabled -- came back three green and one not, and the odd one out
was not a different failure so much as the same run without the fix applied.
In 32646029143 `pm disable-user` reported `new state: disabled-user` and
SystemUI then started eight more times:
14:41:24 ActivityManager: Start proc 6412:com.android.systemui ... GradientColorWallpaper
14:45:05 ActivityManager: Start proc 17299:com.android.systemui ... GradientColorWallpaper
with ten more RegionSampling aborts and a surfaceflinger pid that never sat
still (489, 1570, 3524, 4396, 6038, 7987, 9732, 11520, 13208, 15048). The
framework is being SIGKILLed every twenty seconds while this runs, so a
package-state change can go down with the system_server that accepted it.
Two things were wrong, and the second is why the first went unnoticed:
- one disable attempt was treated as sufficient
- the wait after `adb shell stop` was not a wait. It asked `service check`
0.3 s later and got `found` from the system_server that was still on its
way out, so it never waited for anything. Both the good and the bad run
printed `services back after 5 s`, which is how a broken fix looked
identical to a working one.
Now: up to three rounds of disable -> take the framework down and confirm
system_server is actually gone -> bring it back -> verify the package is in
`pm list packages -d` -> require a 45 s window with zero new aborts. Nothing
is believed because a command said so.
Also adds measure_baseline, default true. The 45 s pre-measurement is what
makes the rate comparable with the local figures, but it is 45 s of
crash-looping before the disable has to land, which is a worse starting
point than a real leg would have.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
93398c4616 |
Resolve adb by path in the API 37 watchdog
The first three dispatches came back with every watchdog sample reading `boot=? surfaceflinger=none zygote64=none dma_aborts=0`, on runs where the device demonstrably booted and the action's own adb was working two steps away. The watchdog was not measuring anything. The emulator action puts platform-tools on PATH with core.addPath, which writes GITHUB_PATH and therefore only affects LATER steps. The watchdog is started before the action -- that is the whole point of it -- so it inherits the runner's own PATH, where a bare `adb` is not necessarily anything. Every call failed into `2>/dev/null` and the sampler dutifully recorded the silence as zero. It now resolves adb by path, preferring ANDROID_HOME, re-resolving on every iteration in case platform-tools arrives later, and echoing the path it settled on. The launch step prints ANDROID_HOME and `command -v adb` for the same reason: a repeat of this failure should be one line to spot, not three runs of quiet zeros. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
8fdad6e20b |
Add a dispatch-only workflow for the API 37 CI question
status_check.yml stops its E2E matrix at 36 and says the android-37.0 image is why. That is established locally under -gpu host and under ANGLE, and it is not established for CI: runners use -gpu swiftshader_indirect, and the one local measurement of that mode was void for a local reason -- Fedora denies execheap to SwiftShader's JIT, so the emulator died before the guest mattered. What CI does at API 37 has therefore never actually been measured. This is that E2E job with the matrix replaced by workflow_dispatch inputs, so a hypothesis costs a dispatch rather than a commit: renderer, API level, image target, channel, SystemUI disable, boot timeout, whether the suite runs at all, and free-form emulator and Gradle arguments. It triggers on nothing else and gates nothing. It calls .github/scripts/e2e-run.sh rather than forking it, and pins the same disk-size, ram-size, action SHAs and KVM setup as the job it copies, so a run here measures the renderer and not a different device. The watchdog is load-bearing rather than decorative. The emulator action calls killEmulator() from its own catch block, so a run whose emulator never boots is torn down before any script: line executes and leaves nothing behind -- which is the exact failure shape API 37 is suspected of. It starts before the action, samples sys.boot_completed, the surfaceflinger and zygote pids and the hasReadColorBufferDma abort count every 20 s, and keeps a rolling copy of the crash buffer so the last read survives the teardown. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
6c34fad17c |
Write down that testable code is not done until it is tested
Stated as a project norm: if a piece is unit testable it gets unit tests, and if it is e2e testable it gets e2e tests, before it counts as done. Both clauses, not either/or. Recorded here rather than left as a habit because the recent review measured what happens without it. Forty-six mutations were run against a 257-test suite; thirty-six bit and NINE were vacuous, five of those passing the entire suite while a reattachment code path sat completely unguarded. That code had shipped, been reviewed, and looked tested. "The suite is green" was true and meant nothing. The convention also names the two things that make it enforceable rather than aspirational. Unit-testable is broader than it looks, because the pure-seam pattern converts device-bound logic into a testable function plus a thin edge, and Robolectric now covers the rest including Compose. And e2e is genuinely runnable locally since the emulator renderer cause was found -- until last night, "run the instrumented suite" was not a request anyone could act on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
2747bb8627 |
Measure the coverage figure instead of carrying it forward
CLAUDE.md has said "~31% of lines" since the lint/format work landed. Measured on main today it is 29.8% (629/2113 lines, 408/1424 branches). The number went DOWN, which is worth stating rather than quietly correcting. The JVM suite went from 11 test files to 43 over the same period, so the intuition -- and the review finding that prompted this, which called the direction certain -- was that coverage must have risen. It did not: main source grew from 4,114 to 5,715 lines as the fixes added Reattachment, JobSnapshots, JobTags, InputQuery, StagingNames, StagingSweep, NativeLoadFailure and an Application class. The denominator outran the numerator. That is not an argument against the tests. It is an argument against quoting a coverage percentage from memory, which is exactly how the stale figure survived. The line now carries the measurement, its date, and the instruction to re-measure. R30 / #39 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
f8e6bfa2a3 | Merge remote-tracking branch 'origin/main' into tools/api-37-emulator | ||
|
JMR-dev
|
0896cef758 | Merge remote-tracking branch 'origin/main' into fix/review-app-gaps | ||
|
JMR-devandClaude Opus 5
|
d3975617a8 |
Put a gate on the three pieces of wiring that had none
Three separate mutations passed the full 257-test suite, all for the same reason: the tool was tested and the thing that calls it was not. The join half of per-job staging. Reverting ConcatWorker to the constant the audit's own D8 table names -- "joined.<ext>", one string for every join of a format -- left everything green: PerJobStagingTest drives only the conversion worker, and StagingNamesTest pins only the pure function. The new case drives two real ConcatWorkers with different ids and reads what they asked for rather than what is on disk, because ConcatEngine is native, so neither join gets past it here and the catch on the way out deletes what it staged. The recorder moves into WorkerStubs, which is what that file is for, and the enum test that already had a private copy now uses it. The process-start sweep. Deleting the one line in LibreMediaConverterApp.onCreate() -- the only reason that class exists, and the backstop for every leak discardStaged cannot reach -- left everything green too. The test stages one file a day old and one written now, calls onCreate() again, and asserts both halves: the abandoned one is collected and the live one is not. The second half is what says this is a sweep rather than the clearStaging() it replaced, which could take a file out from under a running job. The mtime is set explicitly, because "written long enough ago" is not something a test can wait for when the period is twenty-four hours. Casting the Robolectric application to LibreMediaConverterApp is an assertion in itself: it fails if android:name ever stops pointing here, in which case the swept line would be correct code that never runs. The backup and device-transfer exclusions. Reverting data_extraction_rules.xml to the template's boilerplate left the unit tests green AND lintDebug green -- it is a resource, so nothing was reading it -- and the failure it causes is one nobody meets in development. WorkManager's queue is the app's whole backup payload, and its rows name content:// grants and cacheDir paths that do not survive a transfer; reattachment queries by tag on launch, so a fresh install would come up attached to a job the user never ran on it. The test reads the compiled resource table, so what it pins is what the APK carries, and it asserts domain and path for all four entries in both sections -- an <exclude> with no path is skipped unchecked by lint's own detector, so half an entry could protect nothing. Its KDoc records the one thing it does not cover: the manifest attribute that points the system at the file. R8 / #17, R9 / #18, R11 / #20 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
7ae660ee37 | Merge remote-tracking branch 'origin/main' into tools/api-37-emulator | ||
|
JMR-devandClaude Opus 5
|
775a44753b |
Say that the default sweep is red on purpose, and narrow two claims
R16 / #25 -- the branch put API 37 into the default APIS list, where it is permanently two
failures short of green, so a bare `run-e2e.sh` exits 1 by design and nothing said so.
Somebody running it from habit, a wrapper or a hook gets a red exit forever and either
stops reading exit codes or debugs a normal state.
Documented rather than suppressed. The script's own comment already argued that an
expected-red level belongs in the exit code -- reversing that is the branch owner's call,
not a correction -- and the review's alternative needs an exact-set comparison of the
failing test names before it can subtract 37's contribution, which is a new mechanism that
cannot be validated without a device. So:
- the header now states the exit code (0 all green / 1 any level red / 2 refused to
start), says a bare run is 1 by design and why, and gives `run-e2e.sh 33 34 35 36` as
the sweep that can be green;
- a red sweep prints one note after the summary saying the same thing, because the exit
code is read in the terminal and not in the docs -- but ONLY when 37.x is the only level
that went red. `overall` is set by any red level, so a note keyed on "37 was in the
list" would have called a genuine API 34 failure "by design", which is the defect this
is meant to prevent, one layer up. mark_red records which level it was, where the loop
already knows;
- docs/local-emulator.md says it where the default is documented.
R27 / #36 --
|
||
|
JMR-devandClaude Opus 5
|
3534c6d996 |
Clean up the empty document a refused open leaves, and stop the space sums wrapping
publish() opened the destination stream outside its guarded region, justified by "nothing has been written at that point, so there is nothing of ours to remove". That reasoning is wrong about what exists: SAF's CreateDocument contract creates the document before publish() is ever called -- which is why every fixture in OutputPublisherPublishTest starts as an existing empty file. A provider that then hands out no stream, because it dropped between the picker and the write or simply returns null, left a zero-byte file at the name the user chose while the screen said the save had failed. The open moves inside the try, so the same two bounds that already govern a failed copy govern this: only a document URI, and only a destination positively known to be empty. The dead-provider case is untouched and now demonstrably by the guard rather than by the placement -- nothing answers for that authority, so no size can be read, and "I could not tell" still refuses to authorise a delete. Its test comment said the old thing and now says that one. The space arithmetic overflows in two places, both live on main and independent of the allocatable-versus-usable question that stays parked: - hasSpaceFor computed `free > required + headroom`. A request within 128 MiB of Long.MAX_VALUE wraps that sum negative, and every free-space measurement beats a negative number, so the check answers "plenty of room" to the largest request it can be handed. Rewritten as `free - headroom > required` with both operands clamped at zero, which is the form the parked branch's StagingSpace.hasRoomFor already argues for. - InputQuery.total folded a join's inputs with nothing stopping the sum from wrapping, and that is the reachable half: no single file overflows, three four-exabyte inputs do. It saturates at Long.MAX_VALUE now, which the check above then refuses. SpaceArithmeticTest ties the two together in the shape the defect had -- the total that came out negative is handed straight to the space check -- and keeps one allowed case so the refusals cannot pass by refusing everything. The negative-size clamp is deliberately left unasserted, with a comment saying why: it only changes the answer when free space is below the headroom, which a test reading the host's real cache volume cannot arrange. R6 / #15, R23 / #32 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
a6cf4f4ff4 |
Stop three enum reads escaping doWork, and pin what the attempt bound buys
Both workers read enums out of their input Data with Enum.valueOf, and all three reads sit
ABOVE the try. A name this build does not define -- which is what a downgrade or a
rollback with work still in the queue produces, since WorkManager keeps work for about a
week -- threw IllegalArgumentException straight out of doWork(). That is D13's signature
verbatim: FAILURE with reschedule = false, output Data with zero entries so the screen
says "Conversion failed." and nothing else, and no staged.delete(), so the partial stays
in cache. readSpec() twelve lines below already handles exactly this case, and its KDoc
says why.
So all three take readSpec's shape: entries.firstOrNull { it.name == name } ?: default.
Consistency argues for it as much as correctness does -- the file already contains the
right answer to this question, three times.
WorkerEnumFallbackTest reaches each read. Two of them pin the value that replaces the
unknown name rather than only that nothing threw: a quality tier falling back to something
arbitrary would convert at a setting nobody chose, and a join's format decides the
extension its output is staged with, which is where FFmpeg infers the container from. The
engine-preference case asserts through the space check instead, because predicting which
engine AUTO picks would tie the test to a routing rule it is not about. Against the
unfixed code all three fail with "No enum constant ...".
MAX_FOREGROUND_START_ATTEMPTS had no test of its value. Both existing cases are written
against the symbol, which pins the relationship and leaves the number free: changed to 2,
the job gives up about ninety seconds after process death -- exactly the long conversion
the retry exists to protect -- and all 257 tests stayed green.
The new assertion is the property the KDoc argues, not the literal: summed against
WorkRequest's own DEFAULT_BACKOFF_DELAY_MILLIS and MAX_BACKOFF_MILLIS, the attempts have
to span at least eight hours, which is what makes "the user will have opened the app by
then" a claim rather than a hope. A deliberate re-tune that keeps the property passes; the
accident does not, and reports the span it got (0.025 hours at 2).
R22 / #31, R10 / #19
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
961cfa72a2 |
Derive the suite size instead of writing it down in two documents
R4 / #13 and R20 / #29 are one defect: an absolute test total in an unregenerated document, written the same day it went stale. This branch was cut at |
||
|
JMR-devandClaude Opus 5
|
c5c4c5323b |
Test the edge that feeds reattachment, and stop it reporting ENOENT
Reattachment.choose has twenty tests and every mutation aimed at it bites. Everything that computes its inputs had none, and five mutations there passed the whole 257-test suite. Four are closed here, each verified by applying the mutation and watching the new test go red. jobSnapshots() is the half that has to touch WorkManager and the filesystem, so it is where the untested values live. JobSnapshotsTest drives it against a real WorkManager and a real cacheDir: - A zero-byte staged file is not an output. Relaxing the filter to `exists()` -- which is what a job killed before its engine wrote anything leaves behind -- made the snapshot claim a result, and the user would meet a Save button for a zero-byte "conversion". The same case pins that the path is still reported and that the mtime stays 0 for a file that is not a result. - Each result carries its own file's mtime. Hardcoding it to zero starves the newest-file tie-break of the only data it has, which is precisely the failure the tie-break exists to prevent: the query has no ORDER BY, so an arbitrary winner keeps winning every launch. Timestamps are set with setLastModified and compared against what the filesystem stored, because mtime granularity is not this test's claim to make. ReattachGuardsTest covers the two decisions the ViewModel makes that the pure rule cannot: - A file picked while the query was still in flight is not reattached over. Deleting the guard turns the user's pick into yesterday's job -- with the Save button pointing at a file the card does not name. Made deterministic by holding WorkManager's task executor rather than by racing two IO hops: the query cannot finish until the pick has landed. The test also asserts the brake really gripped, so a reattachment that never arrived cannot pass for one that was refused. - An Ambiguous result is offered without being attributed. Two finished jobs naming one staged file is what the device produced before staging was keyed on the job id; taking the first job's tags labels the file with the other conversion's name, which is the confident lie the KDoc rejects. The neutral label and the absent size are both pinned. ReattachmentTest's FAILED exclusion was only ever tested with pathless FAILED jobs, so a narrow regression ranking a FAILED job that carries a file like a result passed all 257 tests. The live shape is the 2 MB orphan the device pass found: a job killed mid-write leaves a partial, and under that regression the user is offered a truncated file with a Save button. One fixture with outputPath and outputExists set closes it. save() re-checks the staged file, in both ViewModels. The check reattachment made ran inside a tag query that can be hours older than the tap, and cacheDir is what the OS empties when it wants space and what the sweep collects after a day. The file's absence used to arrive as staged.inputStream() throwing, and e.message put "/data/user/0/.../4b4882....mp4: open failed: ENOENT" on screen -- a true statement about a path the user has never seen and cannot act on. It now reads as a sentence with an action in it. The message is one constant next to OutputPublisher because both ViewModels need it and staging is what it is about. Reattachment's KDoc claimed a defect that was fixed in the commit before it -- that "Start over" keeps its staged file -- which would send a maintainer to re-fix D2. Rewritten to say what is actually true: the delete happens, and the gap it leaves is the reset() whose delete is cancelled with the Activity, which is the sweep's job and is named in the sweep's own KDoc. R1 / #10, R2 / #11, R24 / #33, R25 / #34 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
da6f2807e9 |
Stop an interrupted sweep leaking the emulator, the AVD and the port
Four corrections to the harness, none of which changes what a successful sweep does. R17 / #26 -- no trap. Ctrl-C during a sweep (now up to five boots long) left headless qemu on console port 5560 and an lmc_e2e_apiNN AVD behind. The next run's `emulator -port` then collides with the orphan and `emu_adb` can resolve to it -- on a workstation with the Pixel plugged in, exactly the ambiguity the ANDROID_SERIAL pinning exists to prevent. `cleanup` (stop_emulator + delete_created_avds, KEEP_AVD honoured) is now on EXIT, INT and TERM. It is idempotent and the normal path calls it explicitly before the summary, so cleanup output cannot land after the summary and the EXIT trap finds nothing to redo. The interrupt path passes a 6-second grace rather than 30: Ctrl-C has already reached the emulator through the foreground process group, so that wait is only for it to finish writing, and `kill -9` follows regardless. `exit "$overall"` stays the last line, so the exit code an EXIT trap could have swallowed is still the one that escapes. The emulator logs in $LOG_DIR are deliberately kept -- they are the only evidence a failed boot leaves. R31 / #40 -- `kill -9 "${EMU_PID:-0}"`. EMU_PID is empty, not unset, if the background launch never produced a job, so `:-0` converted "nothing to kill" into pid 0, which POSIX reads as the sender's whole process group. The `kill -0` wait loop had the same shape and would have spent its full grace period testing the group. All three sites now take a bare `$EMU_PID` behind one `[ -n ... ] || return 0` guard. boot_emulator's own `kill -0` is left alone: it runs only after the assignment and cannot reach the group form. R33 / #42 -- ensure_avd wrote CI's RAM and disk pins to a hardcoded $HOME/.android/avd/... path and checked nothing. With ANDROID_AVD_HOME (or ANDROID_USER_HOME, or ANDROID_SDK_HOME) set, the sed failed and the level ran on at default RAM and userdata, which surfaces much later as "not enough space" and reads as a device problem. `avd_config_path` now looks in every directory avdmanager honours -- no precedence is asserted, the existence check decides -- and a level that cannot be found or written fails instead of running unpinned. R34 / #43 -- disable_region_sampling's "one blocking wait on the device" did not wait: `adb shell stop` does not clear sys.boot_completed, so the property still read 1 and the loop returned at once. Deleted, and the comment now names the service-check loop below it as the actual wait -- which polls the better thing anyway, since `Can't find service: package` is the failure it exists to prevent. That loop also says so when it gives up after 150 s instead of proceeding silently. Deliberately not doing the `setprop sys.boot_completed 0` variant: the loop tested for an empty value, so a 0 would not have made it wait either, and the `!= 1` form it would need is an unbounded loop inside `adb shell` with no timeout. Checked with `bash -n` and with two stub harnesses in place of a device (shellcheck is not installed here): one drives the extracted lifecycle functions against fake binaries and asserts pid 0 really does hit the sender's process group, that an empty EMU_PID now signals nothing and returns at once, that SIGINT cleans up once and exits 130 within seconds, that KEEP_AVD survives the trap path, and that an explicit exit status survives the EXIT trap; the other runs the real script end to end on the boot-failure path, which stops short of e2e-run.sh, and checks the pins land in config.ini, the created AVD is removed, a misplaced config.ini fails the level, and `set -u` is not tripped anywhere. No emulator was booted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
614af35647 |
Hold the corrections themselves to the standard they impose
Three defects in the three preceding commits, found on review. A commit set whose subject is stale dates and inferred status cannot carry either. Dates. Both correction blocks were stamped 2026-08-23. The commits are dated 2026-08-22, as is every other date in these two files and the review that produced them -- a day in the future, in the one place a reader checks to see how fresh a correction is. Corrected to the commit date, and the D6 note now carries one too. Coherence. The Status line was changed to say fix status "tracks main, re-checked at 18c53a3" while "Last verified: 2026-08-22, against main at 903b43c" stood two lines below it, unchanged. A reader would take the whole document as anchored to |
||
|
JMR-devandClaude Opus 5
|
01cbc94888 |
Say what the FFmpeg format tests have actually been run against
R18 / #27. The front-page status line said the FFmpeg format tests "have been written but not yet executed on a device". They have been executed, repeatedly and green, and this is the one line a contributor uses to decide whether the FFmpeg path is trustworthy -- understating it costs more than a stale detail elsewhere would. FFmpegEngineTest's nine format tests -- mp3, gif, matroska, flac, wav, opus, H.264, H.265, and the one asserting a failure surfaces as an exception rather than a silent empty file -- were present at every commit cited below, checked by counting @Test in that file at each: - Physical Pixel 10 Pro XL, API 37, 2026-08-21 at |
||
|
JMR-devandClaude Opus 5
|
7e7f1301a7 |
Re-date the audit's testing section, which its own follow-up falsified
R14 / #23, R15 / #24. "On testing these" proposed a plan; the twelve fixes then executed it, so the section describes a state that no longer exists. It is re-dated rather than deleted -- the reasoning is why the test stack looks the way it does -- with each stale claim marked where it stands. R15 / #24, four statements, each checked against this checkout rather than against another document: - "exactly one dependency, testImplementation(libs.junit)". There are five: junit, robolectric, androidx.work.testing, the Compose BOM platform and compose-ui-test-junit4. - "a testOptions { unitTests.isIncludeAndroidResources = true } block, which this module does not currently have at all". app/build.gradle.kts:105-110. - "work-testing, compose-ui-test-junit4 and espresso-core ... have zero users." By import, work-testing has seven files under app/src/test and androidx.compose.ui.test has one. espresso-core really is still at zero, so that third is kept as the only part still standing. - The preamble's "OutputPublisher, both ViewModels, both Workers and MainActivity have no JVM unit tests at all". 25 JVM test files were added over that set, 180 tests to 257. That last one is also the derivation of CLAUDE.md's ~31% coverage figure, so the reasoning is kept verbatim and only its tense and scope are fixed: the ~31% is what those ~1,200 untested lines produced at |
||
|
JMR-devandClaude Opus 5
|
9f0bc9d19b |
Correct the defect audit's status metadata, which went stale in hours
R3 / #12, R12 / #21, R13 / #22. Three status claims in the audit were false against `main` at |