9f06eb99884c27bc5a71a08dc9032f7edde9caea
30
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  JMR-devandClaude Opus 5
|
c757565d64 |
Read the instrumented suite, and find the test that proves nothing
Four coverage waves have been steered by JaCoCo, which measures testDebugUnitTest only and cannot see app/src/androidTest at all. So nothing had ever asked what the 60 device tests pin, only that they were green. This is that read: a triage, not a test push, in the shape of docs/coverage-read-findings.md. Six findings (E1-E6) are things a test would not fix, and five of those six are prose rather than code — the suite itself is in good condition. Eight tickets carry the rest (#223-#230), each naming the mutation that has to go red rather than a coverage delta. The one that matters is #223. HardwareFallbackTest is the only automated check of the hardware->software fallback against a real codec failure, and it has never attempted the hardware path. Measured on run 34004304566: the API 33, 34, 35 and 37 legs each log Routing sample_h264_444.mp4 -> ... via FFMPEG (NO_HARDWARE_ENCODER) because emulators expose no hardware encoder, so the router sends the job straight to FFmpeg and runMedia3OrFallBack's catch is never entered. Its two assertions — succeeded, output non-empty — are true anyway. It finishes in 448 ms, which is not long enough to fail a hardware export and then software-encode a three-second clip. Deleting that catch reddens nothing anywhere. Two things generalise. A test can assert and still not reach, which neither a coverage number nor a "does it assert something" review can see; the filter that works is whether the test's premise holds on the machine running it. And the codebase already knew — ForcedFailureTest pins DeviceCodecs.PERMISSIVE against this exact hazard and says why, as does ConversionWorkerTest. Their assertions are about the path, so without the pin they fail loudly; HardwareFallbackTest's are about the output, so it passes quietly. That asymmetry is why nobody noticed. E5 records the structural reason this document is separate: F7 in the coverage findings calls probeWithExtractor's catch uncovered when RemuxTest drives it on a device every leg. A JaCoCo-derived document cannot see androidTest, so it will keep re-deriving that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
79097a0256 |
Record wave 4's landed coverage, and that the hang watchdog has fired
Numbers re-measured on
|
||
|
JMR-devandClaude Opus 5
|
e7caeeac43 |
Finish the startup sweep before onCreate returns, in the JVM suite
Robolectric builds an Application per test class that asks for one, and each onCreate launched a staging sweep on Dispatchers.IO over the shared <cacheDir>/conversions/. Nothing joined them, so a test asserting about a staged file was racing every sweep the classes before it had left in flight (#159). It was CI-only until wave 4 added ten Robolectric classes, at which point OutputPublisherStagingTest started failing locally too. LibreMediaConverterApp gains a protected open sweepScope and publishes the Job onCreate started; the JVM suite substitutes TestLibreMediaConverterApp, whose scope is Dispatchers.Unconfined so the sweep -- a plain function that never suspends -- runs to completion inline. The SupervisorJob is kept so this differs from production in the dispatcher alone. Suite-wide rather than per-test: 27 of the 58 Robolectric classes touch that directory, so opt-in was not a real option. It costs one assertion, knowingly. AppStartSweepTest opened by asserting that the manifest's android:name is what Robolectric instantiated. An application= override replaces the manifest rather than being checked against it, and applicationInfo.className reports the override too, so that claim is now unobservable from this source set and a rewritten version would assert the override against itself. The manifest link is device-only; the cast in setUp still catches the test app ceasing to extend the real one. AppStartSweepTest also joins the published Job instead of polling for ten seconds -- a poll cannot tell "swept" from "not started yet" -- and gains a test pinning that the sweep is complete when onCreate returns, which is the property the substitution exists for and the only place it is checked. Verified by mutation rather than by repetition. Putting the test app back on Dispatchers.IO reddens that test 5 times out of 5, while running the whole suite six times per arm caught nothing either way: at the rate #159 was observed at, a clean six-run arm is roughly a coin flip, so the comparison was underpowered and is not offered as evidence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
223fe6deea |
Record what the wave-4 coverage read found, and correct the filter that missed the biggest gap
Five findings (F6-F10) and a methodology correction. The twelve test tickets the same read produced are #192-#203, with #204 for four candidates whose cost was not obviously worth paying; nothing here is work, by this document's standing rule. The correction is the part worth carrying forward. Wave 3 filtered candidates on `mi > 0` and CLAUDE.md recommended it. That filter fails in both directions. It over-reports on Compose: JoinScreen.kt:222 reads mi=10 and also ci=38, and JoinStateAffordancesTest already clicks that Save button and asserts save:joined.mp4 -- the missed instructions are the synthesized $changed/$dirty recomposition-skip path, the same codegen this repo already knew inflated the branch count, showing up in the instruction count too. Every onClick lambda flagged that way turned out to be covered at method level. It under-reports on the case that mattered more. ConversionViewModel.cancel() and JoinViewModel.cancel() miss no line at all, so no line-level filter can see them -- yet only the null arm of activeWorkId?.let(workManager::cancelWorkById) had ever been entered, and nothing in 584 tests connected the Cancel button to WorkManager. That is #192, and it needs `ci > 0 && mb > 0` at method level to surface. Use both filters; `ci == 0` alone is JaCoCo's own missed-line definition and needs no judgement, which is why it is the first. The five findings are what a test would not fix. F6: four more unreachable arms, each traced to the upstream guard that makes it so, one of which (ConversionRouter:214-217) carries a KDoc describing a hazard :117 already removed. F7: probeWithExtractor's catch is unreachable for the same reason probeForConcat's is -- the measurement was on record for one site and not the other, three lines apart in the same file. F8: three more dead members and six unused defaults. F9: both getForegroundInfo overrides are dead because getForegroundInfoAsync is only called for expedited work and nothing sets it -- which sharpens #88's close rather than reopening it. F10: three arms that ARE reachable and still cannot be made to bite, recorded because all three were picked up as candidates and put down again. Six of the ten findings are now "no action" or "not a test gap", and that shape is the honest summary of what is left: arms nothing can reach, members nothing calls, and arms a test can reach but not pin. A coverage number tells none of them apart. One close is qualified rather than overturned. #86 and #133 ruled AndroidDeviceCodecs.probe() out through ShadowMediaCodecList, on the grounds that MediaCodecInfoBuilder cannot set isAlias or canonicalName. A pure seam does not have that constraint and #133 did not evaluate one, so #194 is a different mechanism, not a third run of the same spike -- and its argument is not coverage but that the runCatching fallback logs "assuming permissive" while returning empty sets, which makes canEncode and canDecode answer no for everything. Documentation only: no Kotlin, Gradle or shell file is touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
5a5a680b4c |
Re-measure after wave 3, and write down the two kinds of gap it had to separate
92.8% line (2183/2352), 81.3% branch (1091/1342), 584 JVM tests in 87 classes, measured 2026-09-02 on the tree this branch creates rather than quoted from a PR body. The shape of the wave is worth more than the number, and it is different from the two before it. Waves 1 and 2 were finding uncovered code; by wave 3 there was little of that left, so the gaps had to be sorted before any test was written. Coverage gaps -- filtered to sites where JaCoCo reports mi > 0, which is what separates a real gap from a partial branch on a compound condition, and which cut the candidate list roughly in half. And assertion gaps, where JaCoCo is green and nothing checks the answer: MainActivity's rail and bottom bar were both executed and transposing them passed the entire suite, as did swapping the two progress-notification strings and swapping Content's two destinations. No coverage number would have found any of the three. Naming the required mutation per ticket earned its keep three times, each recorded with what the weak assertion actually was. Also recorded: a green mutation is only evidence when the mutation is a real change -- one classify reordering was semantically equivalent for every reachable input, and a bad mutation and a weak test look identical in the output. Two entries came back as not gaps, which is a result rather than a shortfall: ContainerCapabilities:282's exclude filter cannot drop anything, and probeForConcat's catch arm is unreachable on this runtime -- Robolectric's MediaExtractor never throws from setDataSource, measured across four input shapes. Both denominators moved, in opposite directions and for different reasons, so they are stated rather than folded into the percentage: 1340 -> 1342 branches from MediaProbe.merge, 2348 -> 2352 lines from the ConcatJoiner interface. Neither is new untested code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Recovered onto main after hitting #160's trap for real. #189 was opened against test/concat-engine-seam and, unlike #184-#188, never retargeted to main before merging -- so it merged into a branch that had already been merged and left behind. GitHub reported `merged`, the PR shows MERGED, and none of it was on main: `git merge-base --is-ancestor` is what said so, one line, immediately. That check is the entire reason this was a five-minute recovery rather than a coverage entry that silently stayed three points stale. The failure mode is exactly what CLAUDE.md warns about; what it did not say, and now would, is that the auto-retarget it describes belongs to GitHub's stacking feature, so a stack opened with plain `gh pr create --base` has to be retargeted by hand for every single PR -- and missing one is invisible until you check ancestry. Numbers re-measured on this tree with --rerun-tasks rather than inherited from the branch they were taken on: 584 tests, 0 failures, 2183/2352 line, 1091/1342 branch. Same figures, earned again. |
||
|
JMR-devandClaude Opus 5
|
dbedfb4708 |
Re-measure coverage after wave 2, and write down how a stacked PR merges
COVERAGE. 87.1% line / 69.1% branch, 502 tests -> 88.9% line (2087/2348), 75.4% branch (1011/1340), 546 tests in 76 classes, as #153's five children land. The branch figure moved for two reasons and the entry now says so, because only one of them is new tests. The numerator rose 974 -> 1011; the denominator *fell* 1410 -> 1340. Both are the seam work: pulling a `when` out of a lambda inside a `collect` deletes the coroutine state machine's synthesized branches around it, and leaves a plain function whose branches a test can choose. `ConversionViewModel$observe$1$1` went from carrying the whole mapping to six branches, while the extracted `ConversionViewModelKt` covers 41 of 42 and `JoinViewModelKt` 38 of 39. That is worth stating rather than quoting the percentage alone. A number that rises because the denominator shrank is a different claim from one that rises because more branches are tested, and this entry has a documented history of explaining its own movements wrongly. STACKED PRS. A new Conventions entry, from two traps measured on 2026-08-27 while landing #144-#151. `gh pr merge` refuses a stacked PR outright -- "must be merged using the asynchronous merge REST API" -- and so does the plain `/merge` endpoint. The one that works is `PUT .../pulls/N/merge-async`, which returns a uuid to poll. The second is worse because nothing looks wrong. GitHub retargets a stacked PR's base to main when the one below it merges, but asynchronously. Merging five about thirty seconds apart outran it, so each merged into its own already-merged base branch. Every call returned `status: merged`, every PR read MERGED, `gh pr list --state open` was empty, and none of the content was on main. What caught it was a coverage re-measure two points below what the same tree had produced an hour earlier -- a fresh `git pull` changed nothing, which is what made it a question rather than a stale checkout. `git merge-base --is-ancestor` answers it in one line. #160 is what the recovery cost. Also recorded: the auto-retarget belongs to the stacking feature. A PR opened with a plain `--base some-branch` does not retarget when that branch merges, and has to be moved by hand -- which is what #163 needed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
2d4898ad44 |
Re-measure the coverage entry against the tree this branch creates
84.9% line / 63.8% branch, 454 tests -> 87.1% line (2025/2324), 69.1% branch (974/1410), 502 tests in 71 classes, as #132 and #133's ten children land. The entry already instructs re-measuring before quoting, and that is why this is here rather than in the batch: quoting these numbers before the work merged would have described a tree that did not exist. It nearly went wrong the other way too -- the first measurement for this commit was taken against a main that was three merges stale and read 85.1%. Also says something the bare numbers do not. Branch moved 5.3 points against line's 2.2, and that asymmetry is the expected shape of this kind of work rather than a curiosity: those children targeted decision code -- enum fallbacks, refusal arms, cursor shapes, a `when` over container rules -- where one test chooses a branch the suite had never taken. Line coverage barely notices that. Branch coverage is the whole point. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
27d7cc0a86 | Merge remote-tracking branch 'origin/main' into m-127b-tmp | ||
|
JMR-dev
|
c27881ab64 | Merge remote-tracking branch 'origin/main' into m-127-tmp | ||
|
JMR-devandClaude Opus 5
|
81ad102f2a |
Stop a deadlocked unit-test run, and make it say what it deadlocked on
The JVM suite had no timeout of any kind, so #125's Room/WorkManager lock-order inversion ran until something outside it gave up: 47 minutes locally, and on CI it would burn the Unit tests job's 30-minute cap and report as a job timeout with no cause. The deadlock is monitor contention, which no interrupt breaks, so nothing inside the JVM could have ended it either. The obvious fix does not work here. A JUnit `Timeout` -- as a rule or as `@Test(timeout = ...)` -- runs the test body on a separate thread, and every Compose test in this source set goes through Robolectric's paused main looper. Both forms fail with "main looper can only be controlled from main thread"; the same tests with the timeout removed pass, so it is the mechanism and not the probe. So the bound comes from outside the test JVM, where it moves no threads: `timeout` on the Test tasks kills the forked worker, and a watchdog jstacks that worker two minutes earlier. The jstack is the point. Gradle's timeout on its own kills silently, a timed-out run writes no XML for the class that hung, and the JVM's own "Found one Java-level deadlock" section naming both monitors is the only reason #125 could be described at all -- so it goes to stdout as well as to a file, because the Unit tests job uploads only reports/tests/. Ten minutes is against the slowest observed passing run, not the typical one: eight CI samples of the whole invocation ranged 62-90s, so this is ~6.7x that and a third of the job cap. A timeout that fires on a healthy slow runner turns a real signal into noise. Both numbers live in a build script that nothing compiles, so HangBoundTest reads them back and the build script joins build.yml as a declared input -- without that the guard would go stale on exactly the edit it exists to catch. Refs #125. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
4f1a007b58 |
Quote the settled figure, now that the work it was waiting on has landed
This PR was opened quoting 81.4%, measured on |
||
|
JMR-dev
|
bf78e06969 | Merge remote-tracking branch 'origin/main' into m-115-tmp | ||
|
JMR-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-dev
|
2e0c6737d6 | Merge remote-tracking branch 'origin/main' into merge-113-tmp | ||
|
JMR-devandClaude Opus 5
|
68f841e84f |
Re-measure coverage, because the figure here was quoted from before the test push
CLAUDE.md's own rule is "re-measure before quoting", and the figure it carried
was measured on 2026-08-24 -- before the #52 children, the MediaProbe and codec
tests, and the guards from #100/#107 landed. Quoting it now would understate the
suite by twelve points, which is the same failure the bullet directly below it
was written to describe.
Measured on
|
||
|
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-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
|
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-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-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
|
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-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
|
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-devandClaude Opus 5
|
fd6e5325cf |
Raise Kotlin to 2.4.10 so the bytecode can join the toolchain on Java 25
The previous commit settled for Java 24 everywhere because Kotlin 2.2.10 refuses jvmTarget 25. That was the wrong constraint to accept, for two reasons. The first is that 24 turned out to be unbuyable. Adoptium's repository carries 8, 11, 17, 21, 25 and 26 -- no 24, because it is a non-LTS that went end of life in July 2025. The builds passed only because Gradle quietly auto-provisioned 24.0.2+12 through foojay, and .idea/misc.xml had been pointed at a temurin-24 that cannot be installed. A toolchain nobody can install is not pinned, it is lucky. The second is that the cap was never on the toolchain at all. Kotlin's ceiling applies to jvmTarget -- the bytecode -- and the JDK running the build is a separate axis. Conflating them is what steered this at 24 in the first place. So the fix is the one the sibling repo already uses: put KGP on the root buildscript classpath, where AGP's built-in Kotlin picks it up instead of the 2.2.10 it bundles. Kotlin 2.4.10 supports jvmTarget through 26, which lifts the ceiling above the toolchain rather than under it. The Compose compiler plugin is versioned in lockstep and reads the same catalog entry, so the two cannot drift, and the module now applies both by id() because they come from the classpath rather than from plugin resolution. Checked rather than assumed, since a silent downgrade would look identical to success: compiled classes report major version 69, which is Java 25. D8 dexes them, R8 minifies them, and ktlint, detekt, lint, the unit tests and the androidTest compile are all green on top. 25 is the right landing place independent of all this: it is LTS, it is in the Adoptium repository, and temurin-25-jdk is already installed here -- so the daemon runs on a real system JDK rather than a provisioned copy of an unpatched one. Two catalog plugin aliases went with it. android-application and kotlin-compose now resolve from the buildscript classpath, so leaving aliases behind would have left two entries that read like the source of truth and control nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
e9542d2223 |
Let the libraries float on minor and patch
Library versions now read "1.+" instead of "1.19.0". Three groups stay pinned, and the reasons differ: agp/kotlin/ksp are version-locked to each other -- AGP 9.3.1's POM declares kotlin-gradle-plugin 2.2.10, so a float that picked up Kotlin 2.4.x would put the Compose compiler ahead of the Kotlin AGP actually compiles with. ktlint/detekt/jacoco because a linter is not a library. A library bump that misbehaves usually still compiles; a new lint rule makes files nobody touched stop passing, turning a PR red for something absent from its diff. Upgrading those is worth a commit that reads the new findings. The FFmpeg AAR is a committed file, not a coordinate. The componentSelection block is the part that makes this safe rather than the part that makes it work. Gradle resolves "+" to the highest version it can find and does not skip prereleases, and androidx routinely publishes alphas numbered above the current stable: lifecycle 2.12.0-alpha01, work 2.12.0-rc01, navigation 2.10.0-rc01, datastore 1.3.0-alpha10, annotation 1.11.0-alpha01 all outrank the releases this app uses. Without the guard, five dependencies would have moved onto unreleased code on the next build with nothing in the diff to say so. With it, every float resolves to exactly the version that was pinned before -- checked against :app:dependencies, not assumed. So this changes nothing today. Every library was already at its newest stable when the catalog was audited; floating is about what happens next month, not this commit. Trying a prerelease is still possible: name the exact version, which pins it rather than floating it. That is the right way round -- an alpha should be a deliberate act with a version number attached to it. Verified: ktlint, detekt, lint, unit tests, androidTest compile, assembleDebug all green, and the configuration cache still reuses across runs of the same task set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
f4962913e1 |
Correct the JDK claim, and finish the @UnstableApi propagation
Two things the first pass got wrong. CLAUDE.md said "use a JDK 17-21, AGP 9 does not support 25+". That was carried over from the sibling repo and is not true here: gradle-daemon-jvm.properties pins toolchainVersion=25, so Gradle provisions and runs the daemon on Java 25 whatever JAVA_HOME says -- JAVA_HOME only picks the launcher. `gradlew --version` prints both, and shows them differing on this machine right now. It also means CI's java-version: '17' is not the JDK that compiles anything, and that the daemon JVM is the same on a laptop as on a runner, which is a better guarantee than the one the file claimed. Marking ConversionDependencies @UnstableApi propagates to its callers, and FakeFailures in androidTest calls it. That is a warning rather than an error in Kotlin, and lint does not read the androidTest source set, so the previous commit compiled clean while leaving one file inconsistent with the very pattern it described. Marked now. Left alone deliberately: gradlew.bat. The new `*.bat text eol=crlf` attribute governs how it is checked out from here on, which is the point of adding it, and the file already has CRLF in both the tree and the index. Rewriting the stored bytes of the wrapper script to prove the attribute works is not this branch's business. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
65a94b4ec1 |
Clear the 35 findings the new tools reported
detekt found 29 and Android lint 6, on a codebase neither had ever seen. Each
one was either fixed or relaxed with the reason written next to it; nothing was
suppressed to make the build quiet.
Fixed, because the tool was right:
- ConversionDependencies constructs Media3Engine, which is @UnstableApi, and
was not marked. Every other type here that touches Media3 propagates the
marker rather than swallowing it with @OptIn, so this one does too. Lint
was the only thing that had ever noticed.
- MediaProbe converted microseconds to milliseconds with a bare 1000, twice,
in a file that also handles a seconds-based duration from a different API.
US_PER_MS and MS_PER_SECOND now say which is which -- that confusion is a
real bug source in media code, not a style question.
- Foreground service types compared SDK_INT against 34 and 35 as raw ints
while the doc comment above spelled the version names out. VERSION_CODES
says it in the code.
- take(3) is a product decision about how many alternatives an error offers.
It means nothing until it is named; MAX_SUGGESTIONS does.
- setProgress(100, ...) is a percentage max, now PERCENT_MAX.
Relaxed, because the rule did not fit:
- The model package is excluded from ReturnCount and CyclomaticComplexMethod
ONLY. It is the decision layer: ConversionRouter.route scores 17 because
the app can give 17 distinct answers to "which engine, and why", each with
its own user-visible reason, and route's own comment records that their
ORDER decides which message is shown. Counting those as complexity measures
how many answers exist, not how hard the code is to follow. Everything else
-- LongMethod, NestedBlockDepth, ComplexCondition -- still applies there.
- A flat `when` used as a lookup table scores a point per entry, so
MediaProbe's demuxer-name to Container map read as complexity 21 with no
nesting and no state. ignoreSingleWhenExpression is the rule's own answer.
- TooGenericExceptionCaught off. MediaProbe, ConversionWorker and ConcatWorker
sit in front of native code that reports a malformed file as anything from
IllegalArgumentException to a bare RuntimeException, undocumented.
Enumerating that list means guessing, and a wrong guess crashes the app on a
file it could have reported as unreadable. SwallowedException stays on, so
these still have to log and handle.
- SI thresholds in the byte formatter, via ignoreNumbers. Each literal sits on
the line with the unit string it belongs to; BYTES_PER_MB would need a Long
and a Double and say nothing the line does not.
- allowedFunctionsPerObject, which the first pass simply missed.
Lint's three version-freshness nags are off. They do not describe this code --
they go red the day someone else publishes a release, which turns a PR red for
something its author cannot see in their diff, and they want the network at
lint time. Upgrades here are deliberate; Kotlin in particular is pinned to AGP's
bundled KGP and is not free to follow the newest release.
UsableSpace is informational rather than disabled, because it is a real finding
that this commit is choosing not to act on. hasSpaceFor reads File.usableSpace,
which ignores reclaimable cache, so the app can refuse a conversion it had room
for. StorageManager.getAllocatableBytes is the better answer, but it changes
when a job is rejected and can throw -- a behaviour change to a safety check,
which deserves its own commit and its own test rather than a drive-by here.
informational keeps it in every lint report instead of hiding it.
Also adds the CI gate and a CLAUDE.md. Coverage is reported and not gated: the
measured baseline is 31% of lines, which is exactly why LibreMail's 0.84 floor
was evidence about LibreMail and not a number to copy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|