Compare commits

...
Author SHA1 Message Date
JMR-devandClaude Opus 5 c5b2dc0f55 Record what working the e2e tickets found (E7, and #238)
Two results from #223-#230 that belong with the read rather than only in
their own tickets.

E7 re-scoped its own ticket. #226 split into a cheap headless half and an
expensive picker-driven one, on the premise that a real DocumentsProvider
can be reached without DocumentsUI. It cannot: an unprotected one is
refused at install, instrumentation runs in the app's uid so the test APK's
own identity is no help, and adopting shell identity is denied too -- each
denial naming ACTION_OPEN_DOCUMENT as the only way in. Measured three ways.
So #226 is one item at the picker's cost, not two.

The useful half of that distinction is that the input bridge needs no
documents provider at all. getSafParameterForRead opens a descriptor
through the resolver, so any readable content:// URI exercises it, which is
what kept #225 headless.

And that is how the read's one production defect surfaced. #238: joining
files picked through the system picker failed outright on the stream-copy
path, because the concat demuxer whitelists protocols separately from
-safe 0 and ffkitsaf was not on the list. Only STREAM_COPY feeds the
demuxer a list file, and every existing join test passed Uri.fromFile, so
the one broken combination was the only one a user could reach.

Worth stating plainly next to the coverage entry: it was not a missed line
and not an unasserted value, but two covered things no test put together --
the gap shape a coverage number is worst at, and the reason the read
happened.

E4 is marked fixed; #243 made that KDoc name the constant rather than
restate it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-06 02:50:37 -05:00
Jason Ross 8db9a6f52f Merge pull request #243 from JMR-dev/test/cancelling-a-running-export
Cancel a running Media3 export, completing the third engine
2026-09-06 02:48:22 -05:00
2 changed files with 82 additions and 12 deletions
+16 -1
View File
@@ -332,9 +332,24 @@ install for code that can never run — and on API 37 the full APK does not fit
without the pin they would fail loudly; `HardwareFallbackTest`'s are about the *output*, so it without the pin they would fail loudly; `HardwareFallbackTest`'s are about the *output*, so it
passes quietly. **Prefer asserting the path over asserting the artefact** where the two differ. passes quietly. **Prefer asserting the path over asserting the artefact** where the two differ.
The read was a triage, not a test push, and five of its six findings are prose rather than code — The read was a triage, not a test push, and six of its seven findings are prose rather than code —
the suite itself is in good shape. What had drifted is its self-description. the suite itself is in good shape. What had drifted is its self-description.
**Working the tickets then found the thing the read could not: one production defect.** #238 —
joining files picked through the system picker failed outright on the stream-copy path. The
concat demuxer whitelists protocols separately from `-safe 0`, and `ffkitsaf` was not on the
list; only `STREAM_COPY` feeds it a list file, and every existing join test passed
`Uri.fromFile`, so **the one broken combination was the only one a user could reach**. Not a
missed line and not an unasserted value — two covered things no test put together, which is the
gap shape a coverage number is worst at.
**E7 is the other reusable result**, because it re-scoped its own ticket. A real
`DocumentsProvider` cannot be reached without the picker: an unprotected one is refused at
install, instrumentation runs in the app's uid so the test APK's identity is no help, and shell
identity is denied too — each denial naming `ACTION_OPEN_DOCUMENT`. So #226 has no cheap headless
half. But the *input* bridge needs no documents provider at all, which is what kept #225 headless
and is how #238 surfaced.
- **Testable code is not done until it is tested.** If a piece is unit testable, it gets unit - **Testable code is not done until it is tested.** If a piece is unit testable, it gets unit
tests before it counts as done. If it is e2e testable, it gets e2e tests. Both clauses apply — tests before it counts as done. If it is e2e testable, it gets e2e tests. Both clauses apply —
a change that is both needs both. a change that is both needs both.
+66 -11
View File
@@ -1,6 +1,6 @@
# E2E-read findings # E2E-read findings
**Status:** six findings, none fixed, none urgent — **plus one confirmed vacuous test, which is a **Status:** seven findings; E4 fixed, the rest standing, none urgent — **plus one confirmed vacuous test, which is a
ticket rather than an entry here** (see [Not covered here](#not-covered-here)). `E1`–`E6` came from ticket rather than an entry here** (see [Not covered here](#not-covered-here)). `E1`–`E6` came from
the 2026-09-05 read of the instrumented suite. Every entry here is a *test-suite* observation — the 2026-09-05 read of the instrumented suite. Every entry here is a *test-suite* observation —
something a new test would not fix, because the test already exists and the problem is what it something a new test would not fix, because the test already exists and the problem is what it
@@ -242,6 +242,49 @@ visible skip or a red test, and that decision is the ticket's.
--- ---
## E7 — a real `DocumentsProvider` cannot be reached without the picker, so there is no cheap SAF test
**Severity: n/a · Confirmed by measurement · this is a platform rule, not a gap**
Added 2026-09-06, from doing #225 and #226 rather than from reading.
`OutputPublisher.publish`'s destination side is asserted only against Robolectric fakes —
`FakeSafProvider`, registered with `asDocumentsProvider = true`, which is the flag that *makes*
`DocumentsContract.isDocumentUri` answer true. #226 split that into a cheap headless half (drive a
real `DocumentsProvider` directly) and an expensive picker-driven half.
**The cheap half does not exist.** Three approaches, all measured on an API 34 emulator:
| approach | result |
|---|---|
| a second `DOCUMENTS_PROVIDER` declared **without** `MANAGE_DOCUMENTS` | refused at install: `SecurityException: Provider must be protected by MANAGE_DOCUMENTS` |
| create the document as the **test APK**, which owns the provider | denied — instrumentation runs *in the target app's process*, so it carries the app's uid whatever `Context` is asked |
| `uiAutomation.adoptShellPermissionIdentity(MANAGE_DOCUMENTS)` | denied identically |
The denial names the only way in:
> `Permission Denial: opening provider …FixtureDocumentsProvider from
> ProcessRecord{… org.libremediaconverter/u0a192} requires that you obtain access using
> ACTION_OPEN_DOCUMENT or related APIs`
And the intent filter is not optional: without it `isDocumentUri` returns false, which is exactly
the branch guarding `deletePartialOutput` — so a provider without the filter tests nothing the
ticket is about.
**So any test of `publish` against a real `DocumentsProvider` must drive DocumentsUI**, and pays
#190's flake tax. The work is one item at that cost, not two, and #226 was updated to say so.
### What this does *not* block, which is the useful half
`FFmpegKitConfig.getSafParameterForRead` — the bridge on every real conversion and join — needs no
documents provider. It opens a descriptor through the resolver, so **any readable `content://` URI
exercises it**, and an ordinary `ContentProvider` may be exported without a permission. That is what
`FixtureContentProvider` is, and it made #225 headless.
**That distinction was worth the trouble**: the first test ever to hand the join path a real
`content://` input found #238, a defect that broke joining for every user who picks matched files.
The expensive gate protects the *destination* side; the *input* side never needed it.
## Summary ## Summary
| ID | Finding | Severity | Evidence | Action | | ID | Finding | Severity | Evidence | Action |
@@ -249,11 +292,12 @@ visible skip or a red test, and that decision is the ticket's.
| E1 | `RemuxTest`'s KDoc claims engine assertions three of its tests correctly omit | low | confirmed by inspection; traced through `MEDIA3_CONTAINERS` | **one line of KDoc** — the tests are right | | E1 | `RemuxTest`'s KDoc claims engine assertions three of its tests correctly omit | low | confirmed by inspection; traced through `MEDIA3_CONTAINERS` | **one line of KDoc** — the tests are right |
| E2 | Three of the 60 instrumented tests assert nothing; two never run | n/a | confirmed by inspection; `docs/local-emulator.md:305` | **no action** — deliberate; but 60 ≠ 60 | | E2 | Three of the 60 instrumented tests assert nothing; two never run | n/a | confirmed by inspection; `docs/local-emulator.md:305` | **no action** — deliberate; but 60 ≠ 60 |
| E3 | `…AndReportsProgress` does not assert progress fired | low | confirmed by inspection; reason inline | **no action** — the name overstates, the KDoc corrects it | | E3 | `…AndReportsProgress` does not assert progress fired | low | confirmed by inspection; reason inline | **no action** — the name overstates, the KDoc corrects it |
| E4 | The API 37 marker's KDoc says "two"; three tests carry it | low | confirmed by inspection; baseline const says 3 | **fix the sentence** | | E4 | The API 37 marker's KDoc says "two"; three tests carry it | low | confirmed by inspection; baseline const says 3 | **fixed** in #243 — it names the constant now |
| E5 | `coverage-read-findings.md` F7's "uncovered" half is stale | low | confirmed by inspection; `RemuxTest.kt:111` drives it | **amend F7** — "device-only" stands, "uncovered" does not | | E5 | `coverage-read-findings.md` F7's "uncovered" half is stale | low | confirmed by inspection; `RemuxTest.kt:111` drives it | **amend F7** — "device-only" stands, "uncovered" does not |
| E6 | The device-capability assertion asks the class under test what to expect | low | confirmed by inspection; no third oracle exists on a device | **no action** — read with **#223** | | E6 | The device-capability assertion asks the class under test what to expect | low | confirmed by inspection; no third oracle exists on a device | **no action** — read with **#223** |
| E7 | A real `DocumentsProvider` is unreachable without the picker, so #226 has no cheap half | n/a | measured three ways on API 34; each denial names `ACTION_OPEN_DOCUMENT` | **no action** — it re-scoped #226 |
**Five of the six are prose, not code**, and that is the shape of this read. The instrumented suite **Six of the seven are prose, not code**, and that is the shape of this read. The instrumented suite
is in good condition: 57 of its 60 tests bite, the fixtures are committed with their generation is in good condition: 57 of its 60 tests bite, the fixtures are committed with their generation
recipes, and the one class that asserts nothing says so in its first line. What this read found is recipes, and the one class that asserts nothing says so in its first line. What this read found is
that **the suite's self-description has drifted from the suite** in five small places and one large that **the suite's self-description has drifted from the suite** in five small places and one large
@@ -312,14 +356,25 @@ decision, not a detail — see **E6** for why no third option exists — and **#
| # | Gap | | # | Gap |
|---|---| |---|---|
| **#223** | `HardwareFallbackTest` never attempts the hardware path on any emulator leg | | # | Gap | Outcome |
| **#224** | Cancelling a *running* native session, in any of the three engines | |---|---|---|
| **#225** | No `content://` input has reached a *successful* conversion — the ffkitsaf bridge | | **#223** | `HardwareFallbackTest` never attempts the hardware path on any emulator leg | closed — it skips instead of passing vacuously |
| **#226** | `OutputPublisher.publish` against a real `DocumentsProvider`, and the SAF premise it rests on | | **#224** | Cancelling a *running* native session, in any of the three engines | closed — all three engines |
| **#227** | The notification's Cancel action has never been fired | | **#225** | No `content://` input has reached a *successful* conversion — the ffkitsaf bridge | closed, and it found **#238** |
| **#228** | `encodesFlacLosslessAudio` and `encodesOpus` pass on any non-empty file | | **#226** | `OutputPublisher.publish` against a real `DocumentsProvider` | **open** — re-scoped by E7; one picker-driven item, not two |
| **#229** | FFmpeg's progress percentage is computed everywhere and asserted nowhere | | **#227** | The notification's Cancel action has never been fired | closed |
| **#230** | *(spike)* whether a running conversion's process can be killed under instrumentation | | **#228** | `encodesFlacLosslessAudio` and `encodesOpus` pass on any non-empty file | closed |
| **#229** | FFmpeg's progress percentage is computed everywhere and asserted nowhere | closed |
| **#230** | *(spike)* whether a running conversion's process can be killed | closed — it cannot; the runner shares the app's process |
**The read's own result, once the tickets were worked: one production defect.** #238 — joining files
picked through the system picker failed outright on the stream-copy path, because the concat demuxer
whitelists protocols separately from `-safe 0` and `ffkitsaf` was not on the list. Only `STREAM_COPY`
feeds the demuxer a list file, and every existing join test passed `Uri.fromFile`, so the one broken
combination was the only one a user could reach.
That is the argument for this kind of read in one line: the gap was not a missed line or an
unasserted value, it was **a combination of two covered things that no test put together**.
**Nothing here was filed as a coverage delta.** Each names the mutation that has to go red, which is **Nothing here was filed as a coverage delta.** Each names the mutation that has to go red, which is
the acceptance criterion wave 4 established and which caught two vacuous tests in that wave before the acceptance criterion wave 4 established and which caught two vacuous tests in that wave before