From 232cbd19494bb1182b99b11804d7c05c8eee8e4b Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 26 Aug 2026 21:18:30 -0500 Subject: [PATCH 1/6] Record the code findings from the 2026-08-26 coverage read Four things came out of re-measuring coverage that a test would document rather than repair, so they go in a doc rather than a ticket: - F1 FFmpegCommandBuilder emits a Vorbis encoder ContainerCapabilities' own comment says nothing emits. Traced unreachable through four call sites, but the interesting reading is the other one: FFmpeg can encode Vorbis, WebM and OGG carry it, and the picker never offers it. - F2 ConversionRequest.hardwareEncodeAvailable is written once and read by nothing; its KDoc describes a Fast-tier preset choice that was removed, and the router computes the same answer itself. - F3 ConversionRequest.videoCodec/.audioCodec have no callers anywhere. Named as NOT a test gap: asserting a delegation restates it. - F4 Two private guards reachable only by direct call. No action, per the judgement #88 reached about getForegroundInfo. Also records two things the read makes look like gaps and are not: the Compose screens' branch numbers (inflated by compiler-synthesised recomposition checks; the line figures are 34/383 and 20/143), and ConversionForegroundType, where #88's premise was re-checked against #122's wedge and holds -- the API 33 leg completes 60/60 cleanly. Entry ids are F1-F4 so they cannot be confused with defect-audit.md's D1-D16, and the confidence vocabulary is deliberately that document's. Co-Authored-By: Claude Opus 5 (1M context) --- docs/coverage-read-findings.md | 259 +++++++++++++++++++++++++++++++++ 1 file changed, 259 insertions(+) create mode 100644 docs/coverage-read-findings.md diff --git a/docs/coverage-read-findings.md b/docs/coverage-read-findings.md new file mode 100644 index 0000000..fb225f4 --- /dev/null +++ b/docs/coverage-read-findings.md @@ -0,0 +1,259 @@ +# Coverage-read findings + +**Status:** four findings, none fixed, none urgent. Every entry here is a *code* observation — +something a test would document rather than repair. The test gaps found in the same read are +tickets, not entries here; see [Not covered here](#not-covered-here). +**Scope:** what a JaCoCo read on 2026-08-26 turned up that writing a test would not fix. This is +a survey, not a work order. Acting on any entry is a separate decision and would be its own commit. +**Last verified:** `main` at `dc8b7c3`, 2026-08-26. Coverage re-measured that day with +`./gradlew :app:jacocoTestReport`: **84.9% line (1971/2321), 63.8% branch (900/1410)**, against +**456 JVM tests in 68 classes**. `CLAUDE.md` quotes 454 in 67 from four hours earlier; the +percentages are unchanged, so no figure there is stale. + +## Why this document is separate from `defect-audit.md` + +`defect-audit.md` is the record of the 2026-08-22 defect sweep: sixteen entries, each a thing that +is *wrong at runtime*. Nothing here is wrong at runtime today. These are arms that cannot be +reached, accessors nobody calls, and one KDoc that contradicts the code beside it — the category +`defect-audit.md` calls **latent**, plus one that is not a defect at all and is recorded so the +next coverage read does not re-file it. + +They are here rather than in that document because folding them in would inflate a sixteen-entry +audit whose status metadata has already gone stale once, and because they share a provenance: +all four fell out of reading a coverage report, and all four are the kind of thing a coverage +report is *good* at surfacing and a test is bad at fixing. + +Entry ids are `F1`–`F4` so they cannot be confused with `defect-audit.md`'s `D1`–`D16`. + +## How to read the confidence labels + +Same vocabulary as `defect-audit.md`, deliberately, so the two read alike: + +- **Confirmed by inspection** — the control flow is fully readable and the finding follows from it. +- **Latent** — not reachable through today's UI, but wrong, and one change away from being live. +- **No action** — recorded because it looks like a finding and is not. + +Nothing below was observed on a device, and nothing below needs to be: every entry is a claim about +what the code says, checkable by reading it. + +--- + +## F1 — `FFmpegCommandBuilder` emits a Vorbis encoder that `ContainerCapabilities` says does not exist + +**Severity: low · Latent · the more interesting reading is a missing feature, not dead code** + +``` +app/src/main/java/org/libremediaconverter/ffmpeg/FFmpegCommandBuilder.kt:188 +app/src/main/java/org/libremediaconverter/model/ContainerCapabilities.kt:84-91 +``` + +`FFmpegCommandBuilder.audioArgs` carries a live Vorbis arm: + +```kotlin +AudioCodec.VORBIS -> listOf("-c:a", "libvorbis", "-q:a", "5") +``` + +`ContainerCapabilities` states, immediately above the set that governs it, that no such thing +exists: + +> `/** Vorbis is absent for the same reason: nothing here emits a Vorbis encoder. */` +> `private val ENCODABLE_AUDIO = setOf(AAC, OPUS, MP3, FLAC, PCM)` + +One of those two is wrong. The comment is the one that is wrong as written — something here does +emit a Vorbis encoder, twelve lines of `FFmpegCommandBuilder`. + +### Why the arm is unreachable today + +Traced, not assumed: + +| step | where | effect | +|---|---|---| +| `validate` runs before routing | `ConversionWorker.kt:123` | a spec is checked on every job, however it was enqueued | +| `validateAudio` refuses non-encodable | `ContainerCapabilities.kt:246-251` | `VORBIS !in ENCODABLE_AUDIO` → `Invalid("This app cannot encode Vorbis audio.")` | +| the only spec→plan encode path | `CopyPlanner.kt:104` | `AudioPlan.Encode(requested)` — but `requested` cannot be Vorbis by the row above | +| the fallback encode path | `CopyPlanner.kt:112-115` | draws from `encodableAudio(container)`, itself filtered by `ENCODABLE_AUDIO` | + +So `AudioPlan.Encode(VORBIS)` is not constructible through the app, and line 188 is dead. + +### The reading that matters more + +`CARRIES_AUDIO` lists Vorbis for WebM (`ContainerCapabilities.kt:62`) and OGG (`:67`). Because +`encodableAudio` filters through `ENCODABLE_AUDIO`, the picker offers **Opus and nothing else** for +WebM, and Opus/FLAC for OGG. FFmpeg on this device can encode Vorbis — the command is written and +correct — and the app declines to offer it. + +So the honest framing is not "delete a dead arm". It is: **is `ENCODABLE_AUDIO`'s omission of +Vorbis a deliberate product call, or an accident that has been costing WebM/OGG users a format the +app already supports?** Nothing in the repo records that decision. + +### The precedent for whichever way it goes + +`Media3Engine.audioMimeTypeFor` has the *same* Vorbis arm, and handles it exactly right +(`Media3Engine.kt:221-233`): the KDoc names it dead, says why the arm stays anyway ("deleting a +right answer out of unreachable code buys nothing"), and points at `Media3EngineMimeTypesTest`, +which asserts which three of six codecs actually arrive — so the set moving fails a test rather +than surprising someone. + +`FFmpegCommandBuilder`'s arm has none of that. Whatever is decided, the fix is to make the two +files agree and to say so in one place. + +### What a fix has to decide + +1. Whether Vorbis belongs in `ENCODABLE_AUDIO`. If yes, this is a feature and needs an e2e test + that produces a playable Vorbis file; if no, go to 2. +2. Correct the `ContainerCapabilities.kt:84` comment, which is false as written, and give the + `FFmpegCommandBuilder` arm the treatment `Media3Engine.kt:221-233` already models. + +--- + +## F2 — `ConversionRequest.hardwareEncodeAvailable` is written, read by nothing, and its KDoc describes behaviour that was removed + +**Severity: low · Confirmed by inspection** + +``` +app/src/main/java/org/libremediaconverter/model/OutputFormat.kt:211-219 +app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt:117 +``` + +The property is set on every request: + +```kotlin +hardwareEncodeAvailable = devices.canEncode(spec.videoCodec), +``` + +`grep -rn 'hardwareEncodeAvailable' app/src/main` returns **that line and nothing else**. No +production code reads it. Its getter is one of three uncovered methods in `OutputFormat.kt`, which +is what surfaced it. + +Its KDoc (`OutputFormat.kt:211-218`) explains at length what it is for: + +> Knowing this lets the Fast tier choose a genuinely fast software preset instead of a mislabelled +> slow one. + +`FFmpegCommandBuilder` no longer does that, and its own test says so — +`FFmpegCommandBuilderTest.kt:132`, `the encoder choice no longer depends on hardware availability`: + +> Once FFmpeg stopped selecting MediaCodec encoders, this flag only affects whether the router sends +> the job to Media3 at all — not what FFmpeg does. + +That second clause is also not true. `ConversionRouter` decides hardware encodability by calling +`device.canEncode(videoEncode)` itself (`ConversionRouter.kt:153`); it never reads +`request.hardwareEncodeAvailable`. The flag is computed from the same source the router +independently consults, carried through the request, and dropped. + +This is the shape of open issue **#68** — a KDoc promising a switch that does not exist. + +**Not harmful.** It costs one `canEncode` call per job and a field on a data class. It is recorded +because the KDoc actively misleads: a reader changing the Fast-tier preset logic would look here +first, and this is not where that decision lives. + +### What a fix has to decide + +Whether to delete the property (and the constructor parameter, and the four +`FFmpegCommandBuilderTest` call sites that pass it) or to keep it and rewrite the KDoc to say it is +vestigial. Deleting is cleaner; the test at `:132` is worth keeping either way, since it pins the +"FFmpeg does not select MediaCodec encoders" rule that the deletion would otherwise erase. + +--- + +## F3 — `ConversionRequest.videoCodec` and `.audioCodec` have no callers anywhere + +**Severity: low · Confirmed by inspection** + +``` +app/src/main/java/org/libremediaconverter/model/OutputFormat.kt:222-223 +``` + +```kotlin +val container: Container get() = spec.container // used: FFmpegConcatCommand.kt:42, :80 +val videoCodec: VideoCodec get() = spec.videoCodec // no callers +val audioCodec: AudioCodec get() = spec.audioCodec // no callers +``` + +Three delegating accessors on `ConversionRequest`; the first is used twice, the other two are used +nowhere in `main`, `test` or `androidTest`. Everything that wants those values reads +`request.spec.videoCodec` or takes the `OutputSpec` directly. + +**This is not a test gap and must not be filed as one.** A test asserting +`request.videoCodec == request.spec.videoCodec` is vacuous by construction — it restates the +implementation and would pass against any delegation, right or wrong. That is precisely the failure +mode `CLAUDE.md` records from the mutation review (9 of 46 mutations vacuous, five over completely +unguarded paths). + +The two accessors are either convenience worth keeping for symmetry with `container`, or two lines +to delete. Deleting them costs nothing and removes two uncovered methods that will otherwise be +re-found by every future coverage read. + +--- + +## F4 — Two guards are reachable only by direct call, and that is correct + +**Severity: n/a · No action** + +``` +app/src/main/java/org/libremediaconverter/ffmpeg/FFmpegCommandBuilder.kt:167-168 +app/src/main/java/org/libremediaconverter/model/ConversionRouter.kt:175-176 +``` + +```kotlin +VideoCodec.COPY, VideoCodec.NONE -> error("encodeVideo called for $codec, which is not an encode") +``` + +```kotlin +if (plan.video == VideoPlan.Copy && video == null) return false +if (plan.audio == AudioPlan.Copy && audio == null) return false +``` + +Both sit in private functions (`encodeVideo`, `media3CanMux`), and both are unreachable because a +caller upstream already excluded the case — which each says in its own comment. `ConversionRouter`'s +is labelled "the second line of defence"; `CopyPlanner` is the first. + +**Recorded so the next coverage read does not treat them as gaps.** A second line of defence that +can be provoked is not a second line of defence. Making these reachable from a test would mean +widening the functions to `internal`, which buys a test that asserts an `error()` fires when called +in a way production cannot call it. This is the same judgement issue **#88** reached about +`getForegroundInfo` and closed on: naming the exemption rather than covering it. + +Neither should change unless the upstream guard does. If `CopyPlanner` ever stops resolving `COPY` +before the builder sees it, `FFmpegCommandBuilder.kt:167` becomes live and wants a test that day. + +--- + +## Summary + +| ID | Finding | Severity | Evidence | Action | +|---|---|---|---|---| +| F1 | `FFmpegCommandBuilder` emits a Vorbis encoder `ContainerCapabilities` says does not exist | low | confirmed by inspection; unreachability traced through four call sites | **decide**: feature or dead arm — the comment is false either way | +| F2 | `hardwareEncodeAvailable` written, never read; KDoc describes removed behaviour | low | confirmed by inspection; `FFmpegCommandBuilderTest:132` corroborates | **decide**: delete or mark vestigial | +| F3 | `ConversionRequest.videoCodec` / `.audioCodec` have no callers | low | confirmed by inspection | delete, or keep for symmetry — **not** a test gap | +| F4 | Two private guards reachable only by direct call | n/a | confirmed by inspection | **no action** — named exemption, per #88 | + +Order, if these are acted on: **F1 first and alone.** It is the only one with a possible +user-visible answer, and answering it may make its own comment fix unnecessary. F2 and F3 are +tidying and belong in one commit with each other, not with F1. F4 is finished by being written down. + +## Not covered here + +**The test gaps from the same read.** Ten JVM-side gaps and three seam questions came out of this +coverage read and are tracked as tickets, because they are work rather than observations. This +document holds only what a test would not fix. + +**`ConversionForegroundType.current()`**, which looked like the sharpest gap in the read and is not. +Its API 33 and 34 arms are cold on the JVM, but issue **#88** already established that the class is +covered by `ConversionWorkerTest.foregroundTypeMatchesTheRunningApiLevel` across the CI matrix, and +that its 0% is the `testDebugUnitTest`-only measurement boundary. The one premise worth re-checking +was whether the 33/34 legs still complete, given #122's wedge — they do: the API 33 leg on the most +recent `status_check` run reports **expected 60, received 60, failed 0, completed cleanly: yes**, +with no `wedged:` row. Recorded because that check is the whole reason this is not a ticket. + +**The Compose screens' branch coverage.** `ConverterScreenKt` reports 110 of 200 branches missed and +`JoinScreenKt` 60 of 82, which looks alarming and is not a signal: the Compose compiler synthesises +`$changed`/`$dirty` recomposition-skip tests that JaCoCo counts as branches. The line figures are +the real ones — **34 of 383** and **20 of 143** missed — and the screens are among the +better-covered files in the repo, which is what #52, #57 and #61 were for. **Do not chase the +branch number here.** If a future read wants a screen metric, use lines. + +**Anything requiring a device.** `MediaProbe`'s FFprobe half (`MediaProbe.kt:151, 156-158, 173-188`) +and `FFmpegEngine` in full report 0% on the JVM and are covered by `androidTest`. JaCoCo measures +`testDebugUnitTest` only; their zeroes are a boundary, as #84, #85, #86 and #88 each recorded +before this. From 25992863e6b088396ac2b384bafeb173f45511d4 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 26 Aug 2026 21:21:34 -0500 Subject: [PATCH 2/6] Name the ticket numbers the findings doc defers to #132 holds the seven JVM test gaps from the same read, #133 the three seam questions. The doc drew the line between them in prose already; this makes it followable. Co-Authored-By: Claude Opus 5 (1M context) --- docs/coverage-read-findings.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/docs/coverage-read-findings.md b/docs/coverage-read-findings.md index fb225f4..cfb9414 100644 --- a/docs/coverage-read-findings.md +++ b/docs/coverage-read-findings.md @@ -2,7 +2,7 @@ **Status:** four findings, none fixed, none urgent. Every entry here is a *code* observation — something a test would document rather than repair. The test gaps found in the same read are -tickets, not entries here; see [Not covered here](#not-covered-here). +tickets #132 and #133, not entries here; see [Not covered here](#not-covered-here). **Scope:** what a JaCoCo read on 2026-08-26 turned up that writing a test would not fix. This is a survey, not a work order. Acting on any entry is a separate decision and would be its own commit. **Last verified:** `main` at `dc8b7c3`, 2026-08-26. Coverage re-measured that day with @@ -234,9 +234,10 @@ tidying and belong in one commit with each other, not with F1. F4 is finished by ## Not covered here -**The test gaps from the same read.** Ten JVM-side gaps and three seam questions came out of this -coverage read and are tracked as tickets, because they are work rather than observations. This -document holds only what a test would not fix. +**The test gaps from the same read.** Seven JVM-side gaps (**#132**) and three seam questions +(**#133**) came out of this coverage read and are tracked there, because they are work rather than +observations. This document holds only what a test would not fix. #133 also records why +`AndroidDeviceCodecs.probe()` was considered and left out, so that spike is not run a third time. **`ConversionForegroundType.current()`**, which looked like the sharpest gap in the read and is not. Its API 33 and 34 arms are cold on the JVM, but issue **#88** already established that the class is From 8a23f2a0b85494c64fbd09287df0fa5762aed5da Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 26 Aug 2026 21:47:17 -0500 Subject: [PATCH 3/6] Correct the #122 claim this document got wrong from one green run The ConversionForegroundType note asserted that #122's wedge no longer kills the API 33 leg, on the evidence of a single run. The PR carrying this document then wedged that exact leg: 23m08s, "wedged: yes -- gradle was killed after 1200s and never returned", failed: unknown. Corrected to what the runs actually show: intermittent, not resolved -- five of the last six completed legs passed in ~7 minutes. And the distinction the wedge row exists to draw is now stated, because it is what keeps #88's reasoning intact: received: 60 means all sixty tests still reported, so the API 33 regime was exercised; it is the failed count that reads "unknown", so the leg could not have reported a break. Also names what that changes -- a @Config(sdk = 33/34) JVM test is worth three lines as insurance against a leg that cannot be trusted to go red, which is a different and much smaller claim than the uncovered behaviour this first looked like. Co-Authored-By: Claude Opus 5 (1M context) --- docs/coverage-read-findings.md | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/docs/coverage-read-findings.md b/docs/coverage-read-findings.md index cfb9414..1393a73 100644 --- a/docs/coverage-read-findings.md +++ b/docs/coverage-read-findings.md @@ -242,10 +242,27 @@ observations. This document holds only what a test would not fix. #133 also reco **`ConversionForegroundType.current()`**, which looked like the sharpest gap in the read and is not. Its API 33 and 34 arms are cold on the JVM, but issue **#88** already established that the class is covered by `ConversionWorkerTest.foregroundTypeMatchesTheRunningApiLevel` across the CI matrix, and -that its 0% is the `testDebugUnitTest`-only measurement boundary. The one premise worth re-checking -was whether the 33/34 legs still complete, given #122's wedge — they do: the API 33 leg on the most -recent `status_check` run reports **expected 60, received 60, failed 0, completed cleanly: yes**, -with no `wedged:` row. Recorded because that check is the whole reason this is not a ticket. +that its 0% is the `testDebugUnitTest`-only measurement boundary. + +The premise worth re-checking was whether the 33/34 legs still complete, given #122's wedge. +**They mostly do, and #122 is not resolved** — this entry said "they do" on first writing, from a +single green run, and the PR carrying this very document proved that wrong: + +| run | API 33 leg | shape | +|---|---|---| +| `32933262839` (#127) | success, 7m16s | `expected 60, received 60, failed 0, completed cleanly: yes` | +| `33033036857` (PR #131, docs-only) | **failure, 23m08s** | `expected 60, received 60, failed unknown, wedged: yes — gradle killed after 1200s` | + +Five of the last six completed API 33 legs passed in about seven minutes, so the wedge is +intermittent rather than systematic. **What it costs is the verdict, not the execution**: `received: +60` on the wedged run means all sixty tests still reported, so the API 33 regime *was* exercised — +but `failed:` reads `unknown`, so that leg could not have told anyone if it had broken. + +That is why this stays a note and not a ticket, and also why it is not simply deleted: #88's +reasoning holds, but the leg it rests on cannot be relied on to report a failure. A +`@Config(sdk = 33)` / `@Config(sdk = 34)` JVM test would pin all three arms deterministically in one +run for about three lines. Small, and worth doing the next time this file is opened — but it is +insurance against a flaky leg, not the uncovered behaviour it first looked like. **The Compose screens' branch coverage.** `ConverterScreenKt` reports 110 of 200 branches missed and `JoinScreenKt` 60 of 82, which looks alarming and is not a signal: the Compose compiler synthesises From d9c32c6ce5a9d2fe83cf2bde4ef588fb818fde40 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 26 Aug 2026 22:02:39 -0500 Subject: [PATCH 4/6] Add F5: areEnabled() is never called, so it is not a test gap Found while decomposing #132 into children. It was item 6 there, and it looked like the cheapest item on the list: three cold lines, a KDoc with real user-visible stakes, and a permission Robolectric can flip in one line. grep -rn 'areEnabled' app/src returns the declaration and nothing else. Both workers construct ConversionNotifications and only ever call build(). So the behaviour the KDoc describes -- warning when progress will be invisible -- does not happen, and a test would assert that a function nobody calls returns what the platform told it. Green, vacuous, and worse than nothing, because it would imply the disabled-notification case is handled. Recorded rather than tested, and the summary now names what F1 and F5 have in common: a comment describing behaviour the code lacks, where the tempting fix freezes the wrong answer. Co-Authored-By: Claude Opus 5 (1M context) --- docs/coverage-read-findings.md | 64 ++++++++++++++++++++++++++++++---- 1 file changed, 57 insertions(+), 7 deletions(-) diff --git a/docs/coverage-read-findings.md b/docs/coverage-read-findings.md index 1393a73..5a54e45 100644 --- a/docs/coverage-read-findings.md +++ b/docs/coverage-read-findings.md @@ -1,6 +1,6 @@ # Coverage-read findings -**Status:** four findings, none fixed, none urgent. Every entry here is a *code* observation — +**Status:** five findings, none fixed, none urgent. F5 was added on 2026-08-27, found while decomposing #132 into children — it had been listed there as a test gap, and is not one. Every entry here is a *code* observation — something a test would document rather than repair. The test gaps found in the same read are tickets #132 and #133, not entries here; see [Not covered here](#not-covered-here). **Scope:** what a JaCoCo read on 2026-08-26 turned up that writing a test would not fix. This is @@ -20,10 +20,11 @@ next coverage read does not re-file it. They are here rather than in that document because folding them in would inflate a sixteen-entry audit whose status metadata has already gone stale once, and because they share a provenance: -all four fell out of reading a coverage report, and all four are the kind of thing a coverage -report is *good* at surfacing and a test is bad at fixing. +every one fell out of reading a coverage report, and every one is the kind of thing a coverage +report is *good* at surfacing and a test is bad at fixing. F5 is the clearest case — it was filed +as a test gap first, and only stopped being one when someone went looking for its callers. -Entry ids are `F1`–`F4` so they cannot be confused with `defect-audit.md`'s `D1`–`D16`. +Entry ids are `F1`–`F5` so they cannot be confused with `defect-audit.md`'s `D1`–`D16`. ## How to read the confidence labels @@ -219,6 +220,48 @@ before the builder sees it, `FFmpegCommandBuilder.kt:167` becomes live and wants --- +## F5 — `ConversionNotifications.areEnabled()` is never called + +**Severity: low · Confirmed by inspection · found while decomposing the test-gap ticket** + +``` +app/src/main/java/org/libremediaconverter/work/ConversionNotifications.kt:60-62 +``` + +```kotlin +fun areEnabled(): Boolean = context.getSystemService(NotificationManager::class.java) + .areNotificationsEnabled() + .also { if (!it) Log.i(TAG, "Notifications disabled; progress will not be visible.") } +``` + +`grep -rn 'areEnabled' app/src` returns **that declaration and nothing else**. `ConversionNotifications` +is constructed in both workers (`ConversionWorker.kt:55`, `ConcatWorker.kt:35`) and only `build()` is +ever called on it. + +**This entry exists because it was very nearly filed as a test gap.** Its three lines are cold on the +JVM, it has a KDoc explaining real user-visible stakes — a foreground service without +`POST_NOTIFICATIONS` shows only in the Task Manager, so progress silently vanishes — and Robolectric +can flip that permission in one line. Everything about it reads like a cheap, worthwhile test. + +It is not, because **the behaviour the KDoc describes does not happen**. Nothing consults +`areEnabled()`, so nothing warns, degrades, or logs when notifications are off. A test would assert +that a function nobody calls returns what the platform told it — green, vacuous, and actively +misleading, since it would imply the app handles the disabled-notification case. That is the failure +mode `CLAUDE.md` records from the mutation review, reached from the opposite direction: not a test +that fails to bite, but a test with nothing to bite. + +### What a fix has to decide + +Whether the app should act on this at all. The KDoc argues it should — a conversion whose progress is +invisible is a real complaint, and `ConversionViewModel` or the worker's foreground start is where a +check would go. If yes, that is a **feature** with a test; if no, delete the method and the KDoc's +claim with it. What must not happen is a test that makes the current state look handled. + +Related: **#16** is open on an adjacent gap — a user who *can* unblock a foreground-denied retry has +no way to make it happen now. + +--- + ## Summary | ID | Finding | Severity | Evidence | Action | @@ -227,10 +270,17 @@ before the builder sees it, `FFmpegCommandBuilder.kt:167` becomes live and wants | F2 | `hardwareEncodeAvailable` written, never read; KDoc describes removed behaviour | low | confirmed by inspection; `FFmpegCommandBuilderTest:132` corroborates | **decide**: delete or mark vestigial | | F3 | `ConversionRequest.videoCodec` / `.audioCodec` have no callers | low | confirmed by inspection | delete, or keep for symmetry — **not** a test gap | | F4 | Two private guards reachable only by direct call | n/a | confirmed by inspection | **no action** — named exemption, per #88 | +| F5 | `ConversionNotifications.areEnabled()` is never called | low | confirmed by inspection; grep returns the declaration only | **decide**: act on it or delete it — **not** a test gap | -Order, if these are acted on: **F1 first and alone.** It is the only one with a possible -user-visible answer, and answering it may make its own comment fix unnecessary. F2 and F3 are -tidying and belong in one commit with each other, not with F1. F4 is finished by being written down. +Order, if these are acted on: **F1 and F5 first, separately.** They are the two with a possible +user-visible answer — a format the app can produce and does not offer, and a warning the app +documents and does not give — and either answer changes what the tidying should look like. F2 and F3 +are tidying and belong in one commit with each other, not with F1 or F5. F4 is finished by being +written down. + +**F1 and F5 share a shape worth naming:** both are places where a comment describes behaviour the +code does not have, and in both the tempting fix (delete the dead arm, test the dead method) would +freeze the wrong answer in place. The decision comes first. ## Not covered here From 44d4c61738a814ae683a54df8af55efce3325088 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 26 Aug 2026 22:13:36 -0500 Subject: [PATCH 5/6] C0 (#134): move the fake providers to scaffolding, let them answer wrongly Two of #132's items are cursor-shaped -- InputQuery's row reads (#137) and OutputPublisher.destinationIsKnownEmpty's short-circuits (#140) -- and the provider that could drive them lived inside OutputPublisherPublishTest and could only answer correctly. Its row was always (file.name, file.length()). Moved FakeSafProvider, FakePlainProvider and the registration helper to FakeProviders.kt, same package, following StagingCleanupSupport.kt and ParkedPickDispatcher.kt. UnreliableOutputStream stays behind: it serves one test, which is the line WorkerStubs.kt draws. Added RowShape, seven ways a provider can answer a metadata query. Column granularity is deliberate -- OutputPublisher reads only SIZE, InputQuery reads both and reaches different answers depending on which is bad -- and so is keeping null, missing, negative and no-row distinct rather than folding them into one "bad" case. That distinction is the whole reason InputQuery exists: hasSpaceFor(0) is only "is there 128 MB free", so a size nobody could determine must not arrive as 0. No production change. OutputPublisherPublishTest, OutputPublisherStagingTest and UnknownInputSizeTest pass unchanged. Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/FakeProviders.kt | 223 ++++++++++++++++++ .../convert/OutputPublisherPublishTest.kt | 124 +--------- 2 files changed, 225 insertions(+), 122 deletions(-) create mode 100644 app/src/test/java/org/libremediaconverter/convert/FakeProviders.kt diff --git a/app/src/test/java/org/libremediaconverter/convert/FakeProviders.kt b/app/src/test/java/org/libremediaconverter/convert/FakeProviders.kt new file mode 100644 index 0000000..cfa32c3 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/FakeProviders.kt @@ -0,0 +1,223 @@ +package org.libremediaconverter.convert + +import android.content.ComponentName +import android.content.ContentProvider +import android.content.ContentValues +import android.content.Context +import android.content.IntentFilter +import android.content.pm.ProviderInfo +import android.database.Cursor +import android.database.MatrixCursor +import android.net.Uri +import android.os.Bundle +import android.provider.DocumentsContract +import android.provider.OpenableColumns +import org.robolectric.Robolectric +import org.robolectric.Shadows.shadowOf +import java.io.File + +/** + * Content providers more than one test needs, and the registration dance they all repeat. + * + * Only that. A stub that serves one test stays in that test, next to the assertion it exists for — + * the rule `work/WorkerStubs.kt` states, and the reason `UnreliableOutputStream` is still private to + * `OutputPublisherPublishTest`. + * + * These started life inside `OutputPublisherPublishTest`, which is the only thing that needed a + * provider at all. They moved here when `InputQuery`'s cursor reads turned out to need the same + * provider answering *badly* — see [RowShape]. + */ + +internal const val DOCUMENTS_AUTHORITY = "org.libremediaconverter.test.documents" +internal const val PLAIN_AUTHORITY = "org.libremediaconverter.test.plain" + +/** + * How [FakeSafProvider] answers a metadata query. + * + * A provider is another app. It can be uninstalled, revoke its grant, crash, or simply answer + * something the caller did not expect — and "answered something unexpected" is not one case but + * several, which is why this is an enum rather than a boolean. + * + * The distinction that matters most to callers is **null versus missing versus zero versus + * negative**. `InputQuery` exists to stop the last three being conflated: `hasSpaceFor(0)` is only + * "is there 128 MB free", so a size nobody could determine must not arrive as `0`, and + * `OutputPublisher.destinationIsKnownEmpty` must answer `false` — never "empty, go ahead and + * delete" — for every one of them. + * + * Column-level granularity is deliberate. `OutputPublisher` reads only `SIZE`; `InputQuery` reads + * both, and reaches a different answer depending on which one is bad. + */ +internal enum class RowShape { + /** What a healthy provider answers: the file's real name and real length. */ + NORMAL, + + /** A row is present and its `DISPLAY_NAME` cell is null. */ + NULL_DISPLAY_NAME, + + /** A row is present and its `SIZE` cell is null. */ + NULL_SIZE, + + /** The cursor carries no `DISPLAY_NAME` column at all — `getColumnIndex` gives `-1`. */ + NO_DISPLAY_NAME_COLUMN, + + /** The cursor carries no `SIZE` column at all — `getColumnIndex` gives `-1`. */ + NO_SIZE_COLUMN, + + /** + * A size of `-1`. + * + * Not a corrupt provider: it is what anything without a fixed length reports — a pipe, or a + * provider streaming its answer — and it is a third way of saying "unknown", distinct from a + * null cell and from a missing column. + */ + NEGATIVE_SIZE, + + /** + * A cursor with the right columns and no rows in it. + * + * Distinct from returning `null`, which is what a provider that does not recognise the URI + * does. Both mean "no answer", and code that treats one as an answer and the other as an + * absence is wrong about one of them. + */ + NO_ROWS, +} + +/** + * A stand-in for the provider behind a SAF destination. + * + * It answers only what its callers ask of a document -- how many bytes are already there, what it + * is called, and delete it -- backed by a real file so the assertions are about the filesystem + * rather than about a mock's call log alone. The rest of the `ContentProvider` surface is stubbed. + * + * Writing is deliberately NOT routed through it. Robolectric's `ShadowContentResolver` + * consults its registered-stream map before it reaches any provider, which is what lets a + * test hand out a stream that writes some bytes and then fails -- a condition a real provider + * cannot be asked to produce on demand. + */ +internal open class FakeSafProvider : ContentProvider() { + + override fun onCreate() = true + + override fun query( + uri: Uri, + projection: Array?, + selection: String?, + selectionArgs: Array?, + sortOrder: String?, + ): Cursor? { + val file = backingFile(uri) + if (!file.exists()) return null + return MatrixCursor(columnsFor(rowShape)).apply { + if (rowShape != RowShape.NO_ROWS) addRow(cellsFor(rowShape, file)) + } + } + + override fun call(method: String, arg: String?, extras: Bundle?): Bundle? { + if (method != METHOD_DELETE_DOCUMENT) return null + val target = extras?.getParcelable(EXTRA_URI, Uri::class.java) ?: return null + deleteRequests += target + deleteFailure?.let { throw it } + backingFile(target).delete() + return Bundle() + } + + override fun getType(uri: Uri) = "video/mp4" + + override fun insert(uri: Uri, values: ContentValues?): Uri? = null + + override fun delete(uri: Uri, selection: String?, selectionArgs: Array?) = 0 + + override fun update(uri: Uri, values: ContentValues?, selection: String?, selectionArgs: Array?) = 0 + + companion object { + // DocumentsContract.METHOD_DELETE_DOCUMENT and EXTRA_URI are hidden from the public + // SDK, so they cannot be referenced. These are the wire names + // DocumentsContract.deleteDocument() actually sends, which is what a provider sees. + const val METHOD_DELETE_DOCUMENT = "android:deleteDocument" + const val EXTRA_URI = "uri" + + /** Where the "documents" really live. Set per test to a Robolectric temp path. */ + lateinit var root: File + + /** Every delete this provider was asked for, in order. Empty is an assertion too. */ + val deleteRequests = mutableListOf() + + /** Armed by the test that needs the cleanup itself to fail. */ + var deleteFailure: RuntimeException? = null + + /** + * How the next query answers. [reset] puts it back to [RowShape.NORMAL], so a test that + * does not care never has to think about it. + */ + var rowShape: RowShape = RowShape.NORMAL + + fun backingFile(uri: Uri) = File(root, uri.lastPathSegment.orEmpty()) + + fun reset(directory: File) { + root = directory + deleteRequests.clear() + deleteFailure = null + rowShape = RowShape.NORMAL + } + + private fun columnsFor(shape: RowShape): Array = when (shape) { + RowShape.NO_DISPLAY_NAME_COLUMN -> arrayOf(OpenableColumns.SIZE) + RowShape.NO_SIZE_COLUMN -> arrayOf(OpenableColumns.DISPLAY_NAME) + else -> arrayOf(OpenableColumns.DISPLAY_NAME, OpenableColumns.SIZE) + } + + private fun cellsFor(shape: RowShape, file: File): Array = when (shape) { + RowShape.NO_DISPLAY_NAME_COLUMN -> arrayOf(file.length()) + RowShape.NO_SIZE_COLUMN -> arrayOf(file.name) + RowShape.NULL_DISPLAY_NAME -> arrayOf(null, file.length()) + RowShape.NULL_SIZE -> arrayOf(file.name, null) + RowShape.NEGATIVE_SIZE -> arrayOf(file.name, UNKNOWN_LENGTH) + else -> arrayOf(file.name, file.length()) + } + + /** What `statSize` reports for anything without a fixed length. See [RowShape.NEGATIVE_SIZE]. */ + private const val UNKNOWN_LENGTH = -1L + } +} + +/** + * The same provider, registered WITHOUT the documents-provider intent filter. + * + * A separate class because the package manager keys providers by component name, so two + * authorities need two components. It exists to prove the guard is a guard: a content URI + * from something that is not a documents provider must not be handed to `deleteDocument`. + */ +internal class FakePlainProvider : FakeSafProvider() + +/** + * Stands [provider] up on [authority] so `contentResolver` and the package manager both know it. + * + * `isDocumentUri()` does not look at the URI alone: it asks the package manager whether anything + * answers `ACTION_DOCUMENTS_PROVIDER` for that authority. Registering the provider with the + * resolver is not enough, which is the whole reason [asDocumentsProvider] is a parameter rather + * than always true — the negative case is a test. + */ +internal fun registerProvider( + context: Context, + provider: Class, + authority: String, + asDocumentsProvider: Boolean, +) { + val info = ProviderInfo().apply { + this.authority = authority + packageName = context.packageName + name = provider.name + exported = true + grantUriPermissions = true + } + Robolectric.buildContentProvider(provider).create(info) + + val packageManager = shadowOf(context.packageManager) + packageManager.addOrUpdateProvider(info) + if (asDocumentsProvider) { + packageManager.addIntentFilterForProvider( + ComponentName(context.packageName, provider.name), + IntentFilter(DocumentsContract.PROVIDER_INTERFACE), + ) + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt index 88431f6..c670a67 100644 --- a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt @@ -1,17 +1,7 @@ package org.libremediaconverter.convert -import android.content.ComponentName -import android.content.ContentProvider -import android.content.ContentValues import android.content.Context -import android.content.IntentFilter -import android.content.pm.ProviderInfo -import android.database.Cursor -import android.database.MatrixCursor import android.net.Uri -import android.os.Bundle -import android.provider.DocumentsContract -import android.provider.OpenableColumns import org.junit.Assert.assertArrayEquals import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse @@ -20,7 +10,6 @@ import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test import org.junit.runner.RunWith -import org.robolectric.Robolectric import org.robolectric.RobolectricTestRunner import org.robolectric.RuntimeEnvironment import org.robolectric.Shadows.shadowOf @@ -31,91 +20,6 @@ import java.io.OutputStream /** What a destination volume says when it fills up mid-write. */ private const val NO_SPACE = "No space left on device" -private const val DOCUMENTS_AUTHORITY = "org.libremediaconverter.test.documents" -private const val PLAIN_AUTHORITY = "org.libremediaconverter.test.plain" - -/** - * A stand-in for the provider behind a SAF destination. - * - * It answers only what `publish()` asks of a destination -- how many bytes are already there, - * and delete it -- backed by a real file so the assertions are about the filesystem rather - * than about a mock's call log alone. The rest of the `ContentProvider` surface is stubbed. - * - * Writing is deliberately NOT routed through it. Robolectric's `ShadowContentResolver` - * consults its registered-stream map before it reaches any provider, which is what lets a - * test hand out a stream that writes some bytes and then fails -- the condition this whole - * file exists for, and one a real provider cannot be asked to produce on demand. - */ -internal open class FakeSafProvider : ContentProvider() { - - override fun onCreate() = true - - override fun query( - uri: Uri, - projection: Array?, - selection: String?, - selectionArgs: Array?, - sortOrder: String?, - ): Cursor? { - val file = backingFile(uri) - if (!file.exists()) return null - return MatrixCursor(arrayOf(OpenableColumns.DISPLAY_NAME, OpenableColumns.SIZE)).apply { - addRow(arrayOf(file.name, file.length())) - } - } - - override fun call(method: String, arg: String?, extras: Bundle?): Bundle? { - if (method != METHOD_DELETE_DOCUMENT) return null - val target = extras?.getParcelable(EXTRA_URI, Uri::class.java) ?: return null - deleteRequests += target - deleteFailure?.let { throw it } - backingFile(target).delete() - return Bundle() - } - - override fun getType(uri: Uri) = "video/mp4" - - override fun insert(uri: Uri, values: ContentValues?): Uri? = null - - override fun delete(uri: Uri, selection: String?, selectionArgs: Array?) = 0 - - override fun update(uri: Uri, values: ContentValues?, selection: String?, selectionArgs: Array?) = 0 - - companion object { - // DocumentsContract.METHOD_DELETE_DOCUMENT and EXTRA_URI are hidden from the public - // SDK, so they cannot be referenced. These are the wire names - // DocumentsContract.deleteDocument() actually sends, which is what a provider sees. - const val METHOD_DELETE_DOCUMENT = "android:deleteDocument" - const val EXTRA_URI = "uri" - - /** Where the "documents" really live. Set per test to a Robolectric temp path. */ - lateinit var root: File - - /** Every delete this provider was asked for, in order. Empty is an assertion too. */ - val deleteRequests = mutableListOf() - - /** Armed by the test that needs the cleanup itself to fail. */ - var deleteFailure: RuntimeException? = null - - fun backingFile(uri: Uri) = File(root, uri.lastPathSegment.orEmpty()) - - fun reset(directory: File) { - root = directory - deleteRequests.clear() - deleteFailure = null - } - } -} - -/** - * The same provider, registered WITHOUT the documents-provider intent filter. - * - * A separate class because the package manager keys providers by component name, so two - * authorities need two components. It exists to prove the guard is a guard: a content URI - * from something that is not a documents provider must not be handed to `deleteDocument`. - */ -internal class FakePlainProvider : FakeSafProvider() - /** * A sink that behaves like a volume filling up. * @@ -179,8 +83,8 @@ class OutputPublisherPublishTest { fun setUp() { context = RuntimeEnvironment.getApplication() FakeSafProvider.reset(File(context.cacheDir, "destinations").apply { mkdirs() }) - register(FakeSafProvider::class.java, DOCUMENTS_AUTHORITY, asDocumentsProvider = true) - register(FakePlainProvider::class.java, PLAIN_AUTHORITY, asDocumentsProvider = false) + registerProvider(context, FakeSafProvider::class.java, DOCUMENTS_AUTHORITY, asDocumentsProvider = true) + registerProvider(context, FakePlainProvider::class.java, PLAIN_AUTHORITY, asDocumentsProvider = false) // SAF's CreateDocument contract hands back a document that already exists and is // empty, so that is the state every destination starts in here. @@ -326,28 +230,4 @@ class OutputPublisherPublishTest { ) } } - - private fun register(provider: Class, authority: String, asDocumentsProvider: Boolean) { - val info = ProviderInfo().apply { - this.authority = authority - packageName = context.packageName - name = provider.name - exported = true - grantUriPermissions = true - } - Robolectric.buildContentProvider(provider).create(info) - - // isDocumentUri() does not look at the URI alone: it asks the package manager whether - // anything answers ACTION_DOCUMENTS_PROVIDER for that authority. Registering the - // provider with the resolver is not enough, which is the whole reason the negative - // case above can exist. - val packageManager = shadowOf(context.packageManager) - packageManager.addOrUpdateProvider(info) - if (asDocumentsProvider) { - packageManager.addIntentFilterForProvider( - ComponentName(context.packageName, provider.name), - IntentFilter(DocumentsContract.PROVIDER_INTERFACE), - ) - } - } } From 8a88fc4ae78a74cde86e7e6e1a5f49233078b091 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 26 Aug 2026 22:19:55 -0500 Subject: [PATCH 6/6] C3 (#137): pin what InputQuery makes of a metadata row Nothing had ever handed InputQuery a cursor row. UnknownInputSizeTest drives the no-provider case thoroughly -- query returns null, measure() answers -- so firstRow's body, displayNameOrNull and sizeOrNull had never executed at all. Nine tests over FakeSafProvider's RowShape states. What they pin is not "reads a cursor" but the rule the class exists for: a size nobody could determine must arrive as null, never 0. Four separate ways a provider fails to give one -- a null cell, a missing column, a negative value, an empty cursor -- plus a provider that throws outright, which is the guard firstRow's KDoc is written for. Mutations run, all four bite: drop `takeIf { it >= 0 }` from sizeOrNull -> negative-size test red drop `!isNull(it)` from sizeOrNull -> null-size test red drop the runCatching in firstRow -> throwing-provider test red drop `!isNull(it)` from displayNameOrNull -> GREEN, does not bite That last one is recorded in the test's KDoc as a named exemption rather than papered over. Measured: MatrixCursor.getString on a null cell returns null while getLong returns 0. So the guard is load-bearing on the size path -- it is what stops a null becoming a real number -- and unfalsifiable on the name path, where getString already yields null. It stays regardless: Cursor.getString's contract makes throwing on null implementation-defined, and a real provider may do what MatrixCursor does not. InputQuery.kt now has no never-executed lines. Suite 456 -> 465 tests, branch coverage 63.8% -> 65.6%. Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/FakeProviders.kt | 11 ++ .../convert/InputQueryCursorTest.kt | 187 ++++++++++++++++++ 2 files changed, 198 insertions(+) create mode 100644 app/src/test/java/org/libremediaconverter/convert/InputQueryCursorTest.kt diff --git a/app/src/test/java/org/libremediaconverter/convert/FakeProviders.kt b/app/src/test/java/org/libremediaconverter/convert/FakeProviders.kt index cfa32c3..2d6b125 100644 --- a/app/src/test/java/org/libremediaconverter/convert/FakeProviders.kt +++ b/app/src/test/java/org/libremediaconverter/convert/FakeProviders.kt @@ -80,6 +80,16 @@ internal enum class RowShape { * absence is wrong about one of them. */ NO_ROWS, + + /** + * The query itself throws. + * + * A resolver call is a call into another app, and that app can have been uninstalled, revoked + * its grant, or simply crashed. `InputQuery.firstRow`'s KDoc is explicit that "a file picker is + * not a place to bring the process down from", so this is the shape that proves the guard is + * one. + */ + QUERY_THROWS, } /** @@ -105,6 +115,7 @@ internal open class FakeSafProvider : ContentProvider() { selectionArgs: Array?, sortOrder: String?, ): Cursor? { + if (rowShape == RowShape.QUERY_THROWS) throw SecurityException("provider revoked the grant") val file = backingFile(uri) if (!file.exists()) return null return MatrixCursor(columnsFor(rowShape)).apply { diff --git a/app/src/test/java/org/libremediaconverter/convert/InputQueryCursorTest.kt b/app/src/test/java/org/libremediaconverter/convert/InputQueryCursorTest.kt new file mode 100644 index 0000000..1b81616 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/InputQueryCursorTest.kt @@ -0,0 +1,187 @@ +package org.libremediaconverter.convert + +import android.content.Context +import android.net.Uri +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File + +/** + * What [InputQuery] makes of a metadata row. + * + * ## Why this is a separate file from `UnknownInputSizeTest` + * + * That test drives the case where **no provider is registered** — the query returns null and + * `measure()` answers instead — and it drives it thoroughly. What it never does is hand `InputQuery` + * a row. Before this file, nothing did: `firstRow`'s body, `displayNameOrNull` and `sizeOrNull` had + * never executed in the JVM suite, so every branch inside them was untested. + * + * ## What is actually being pinned + * + * Not "does it read a cursor" — that would pass against almost any implementation. The rule is that + * **a size nobody could determine must not arrive as a number**, and there are four separate ways a + * provider fails to determine one: a null cell, a missing column, a negative value, and no row at + * all. `InputQuery`'s KDoc states the stake: + * + * > a worker's input `Data` carries the size the *picker* found … `hasSpaceFor(0)` is only "is there + * > 128 MB free". + * + * So each of those four must produce `null`, and `null` specifically — not `0`, not `-1`. A test + * that asserted only "not the file's length" would pass on `0`, which is the exact conflation the + * class exists to end. + * + * ## Why every fall-through lands on null here + * + * [FakeSafProvider] does not implement `openFile`, so `measure()` cannot answer for these URIs + * either. That is deliberate: it isolates the cursor half. The other direction — the cursor says + * nothing and `measure()` succeeds — is `UnknownInputSizeTest`'s + * `a picked file no provider describes is measured rather than reported as empty`, and is not + * repeated here. + * + * ## What the mutations say, including the one that does not bite + * + * Measured against `MatrixCursor`, which is what these tests drive: + * + * | call on a null cell | result | + * |---|---| + * | `getString` | returns `null` | + * | `getLong` | returns **`0`** | + * + * That second row is why `sizeOrNull`'s `!isNull(it)` guard is load-bearing and why these tests + * bite: remove it and a null size arrives as `0`, a real number indistinguishable from an empty + * file, which is the precise conflation this class exists to end. Removing it reddens + * `a null size is unknown rather than zero`. Removing the trailing `takeIf { it >= 0 }` reddens + * `a negative size is unknown rather than reported`. + * + * **Named exemption: `displayNameOrNull`'s `!isNull(it)` guard is not pinned by anything here, and + * cannot be.** `getString` returns null for a null cell, so the fallback applies with or without + * the guard — removing it leaves every test in this file green. The guard is not redundant in + * production: `Cursor.getString`'s contract states that whether it throws on a null column is + * *implementation-defined*, and a real `ContentProvider` is free to throw where `MatrixCursor` + * returns null. It should stay. It simply cannot be falsified with this cursor, and saying so is + * better than implying `a null display name falls back without disturbing the size` covers it — + * that test pins the behaviour, not the guard. + */ +@RunWith(RobolectricTestRunner::class) +class InputQueryCursorTest { + + private lateinit var context: Context + private lateinit var uri: Uri + + @Before + fun setUp() { + context = RuntimeEnvironment.getApplication() + FakeSafProvider.reset(File(context.cacheDir, "picked").apply { mkdirs() }) + registerProvider(context, FakeSafProvider::class.java, DOCUMENTS_AUTHORITY, asDocumentsProvider = true) + uri = Uri.parse("content://$DOCUMENTS_AUTHORITY/document/holiday.mp4") + FakeSafProvider.backingFile(uri).writeBytes(ByteArray(PAYLOAD_BYTES)) + } + + @Test + fun `a provider that answers properly supplies both the name and the size`() { + val described = InputQuery.describe(context, uri) + + assertEquals("holiday.mp4", described.displayName) + assertEquals(PAYLOAD_BYTES.toLong(), described.sizeBytes) + } + + @Test + fun `a null display name falls back without disturbing the size`() { + FakeSafProvider.rowShape = RowShape.NULL_DISPLAY_NAME + + val described = InputQuery.describe(context, uri) + + assertEquals(InputQuery.FALLBACK_DISPLAY_NAME, described.displayName) + // The two columns are read independently. A provider that cannot name the file can still + // size it, and losing the size here would be a bug the name assertion alone would miss. + assertEquals(PAYLOAD_BYTES.toLong(), described.sizeBytes) + } + + @Test + fun `a cursor with no display name column falls back rather than throwing`() { + // getColumnIndex returns -1 rather than throwing, so the `it >= 0` guard is the only thing + // between this and an IllegalArgumentException out of getString. + FakeSafProvider.rowShape = RowShape.NO_DISPLAY_NAME_COLUMN + + val described = InputQuery.describe(context, uri) + + assertEquals(InputQuery.FALLBACK_DISPLAY_NAME, described.displayName) + assertEquals(PAYLOAD_BYTES.toLong(), described.sizeBytes) + } + + @Test + fun `a null size is unknown rather than zero`() { + FakeSafProvider.rowShape = RowShape.NULL_SIZE + + assertNull(unknownSizeMessage("a null cell"), InputQuery.sizeOf(context, uri)) + } + + @Test + fun `a cursor with no size column is unknown rather than zero`() { + FakeSafProvider.rowShape = RowShape.NO_SIZE_COLUMN + + assertNull(unknownSizeMessage("a missing column"), InputQuery.sizeOf(context, uri)) + } + + @Test + fun `a negative size is unknown rather than reported`() { + // What anything without a fixed length reports -- a pipe, or a provider streaming its + // answer. Passing -1 through would be worse than passing 0: hasSpaceFor compares it + // against free space, so it would read as "needs less than nothing". + FakeSafProvider.rowShape = RowShape.NEGATIVE_SIZE + + assertNull(unknownSizeMessage("a negative size"), InputQuery.sizeOf(context, uri)) + } + + @Test + fun `a cursor with no rows is unknown rather than zero`() { + // Distinct from the provider returning null, which UnknownInputSizeTest covers. A cursor + // that exists and holds nothing still has to reach the same answer. + FakeSafProvider.rowShape = RowShape.NO_ROWS + + val described = InputQuery.describe(context, uri) + + assertEquals(InputQuery.FALLBACK_DISPLAY_NAME, described.displayName) + assertNull(unknownSizeMessage("an empty cursor"), described.sizeBytes) + } + + @Test + fun `a provider that throws is survived rather than propagated`() { + // The guard firstRow's KDoc exists for: "a resolver call is a call into another app ... and + // a file picker is not a place to bring the process down from". Without the runCatching, + // this SecurityException reaches the caller and takes the pick with it. + FakeSafProvider.rowShape = RowShape.QUERY_THROWS + + val described = InputQuery.describe(context, uri) + + assertEquals(InputQuery.FALLBACK_DISPLAY_NAME, described.displayName) + assertNull(unknownSizeMessage("a provider that threw"), described.sizeBytes) + } + + @Test + fun `a join total is unknown when any one input could not be sized`() { + // The consequence the four cases above exist for, asserted once at the place it lands. + // Summing the inputs that did answer would produce a lower bound indistinguishable from a + // real total, which is what the space check cannot tell apart. + FakeSafProvider.rowShape = RowShape.NULL_SIZE + val unsizable = InputQuery.sizeOf(context, uri) + FakeSafProvider.rowShape = RowShape.NORMAL + val sizable = InputQuery.sizeOf(context, uri) + + assertEquals(PAYLOAD_BYTES.toLong(), sizable) + assertNull(unsizable) + assertNull("one unknown input makes the whole total unknown", InputQuery.total(listOf(sizable, unsizable))) + } + + private fun unknownSizeMessage(cause: String) = + "$cause means nobody could size the file; that must be null, not 0 -- hasSpaceFor(0) is only a headroom check" + + private companion object { + const val PAYLOAD_BYTES = 4096 + } +}