Re-derive the API 37 carrier counts, and correct what #226 left behind #251
@@ -272,13 +272,15 @@ jobs:
|
||||
# has to begin after them, not between them. The name is stale and kept:
|
||||
# read .github/scripts/e2e-run.sh's header, which carries the measurements.
|
||||
#
|
||||
# notAnnotation below keeps five tests off this row, and one of
|
||||
# them is new. SafPickerRoundTripTest's PICKER test was measured on
|
||||
# 2026-08-24 as passing here and was left on the leg; four gating logcats
|
||||
# read on 2026-09-05 show it aborting system_server from the task-snapshot
|
||||
# path on every single run, pass or fail, which is what had been failing
|
||||
# unrelated PRs (#108). Both of that class's tests now carry the marker.
|
||||
# docs/api-37-emulator-crash.md has the timings and the correction.
|
||||
# notAnnotation below keeps six tests off this row. SafPickerRoundTripTest's
|
||||
# PICKER test was measured on 2026-08-24 as passing here and was left on the
|
||||
# leg; four gating logcats read on 2026-09-05 show it aborting system_server
|
||||
# from the task-snapshot path on every single run, pass or fail, which is what
|
||||
# had been failing unrelated PRs (#108). All THREE of that class's tests now
|
||||
# carry the marker -- the save through the picker (#226) joined on 2026-09-06
|
||||
# by inheritance rather than measurement, since it opens the same picker.
|
||||
# docs/api-37-emulator-crash.md has the timings and the correction, and
|
||||
# FailsOnEmulatorApi37.kt has why the third one cannot be measured here.
|
||||
#
|
||||
# api-level must be a POINT release. A bare 37 is not an SDK package and
|
||||
# fails during setup, which cost a run to discover. `37.0` is the choice
|
||||
@@ -288,9 +290,12 @@ jobs:
|
||||
# docs/api-37-emulator-crash.md measures 37.0 rev 6 and 37.1 rev 8 side
|
||||
# by side, so pinning 37.0 is a decision, not a constraint.
|
||||
#
|
||||
# notAnnotation removes the three tests that do not pass on this image; they
|
||||
# notAnnotation removes the six tests that cannot be RUN on this image; they
|
||||
# run in the advisory job below, off the same marker so they cannot end up
|
||||
# in both or neither. docs/api-37-emulator-crash.md has the measurements.
|
||||
# in both or neither. "Cannot be run" rather than "do not pass" is deliberate:
|
||||
# four fail outright, one of those aborts the framework on its way down, and on
|
||||
# the advisory leg the two picker tests behind it never report at all.
|
||||
# docs/api-37-emulator-crash.md has the measurements.
|
||||
- label: "37"
|
||||
api-level: "37.0"
|
||||
disable-system-ui: "1"
|
||||
|
||||
@@ -76,12 +76,20 @@ days. Read it as the current answer, and see the git history if you need the old
|
||||
`angle_indirect` and `swangle_indirect` all boot, while `auto`, `off`, `guest` and
|
||||
`swiftshader_indirect` do not. `docs/local-emulator.md` has the evidence and the per-API renderer
|
||||
table.
|
||||
- **CI runs API 37, and it gates.** The matrix is 33/34/35/36/37. **Five** of the 69 instrumented
|
||||
tests cannot be *run* on that image, for three unrelated reasons: three Media3 tests fail inside
|
||||
the emulator's own `c2.goldfish.h264.decoder`, one SAF test takes the framework down when it
|
||||
rotates the display, and its sibling — the SAF picker round trip — aborts `system_server` from
|
||||
the task-snapshot path whether it passes or not. All five carry `@FailsOnEmulatorApi37` and run
|
||||
in a separate `continue-on-error` job; the gating leg runs the other 64.
|
||||
- **CI runs API 37, and it gates.** The matrix is 33/34/35/36/37. **Six** of the 70 instrumented
|
||||
tests cannot be *run* on that image, for three measured reasons and one inherited: three Media3
|
||||
tests fail inside the emulator's own `c2.goldfish.h264.decoder`, one SAF test takes the framework
|
||||
down when it rotates the display, and its sibling — the SAF picker round trip — aborts
|
||||
`system_server` from the task-snapshot path whether it passes or not. The sixth, that class's
|
||||
save through the picker (#226), carries the marker because it opens the same picker and a second
|
||||
DocumentsUI dialog on top of it — **not** because it has ever been observed here. It cannot be:
|
||||
the rotation test runs first and takes the framework down, so **all four** advisory runs at this
|
||||
baseline report `expected: 6, received: 4, failed: 4`, and the four are the three Media3 tests
|
||||
plus the rotation — runs 34041156680, 34041593697, 34042397320 and 34043502322. **Neither picker
|
||||
test has ever reported on the advisory leg**, which is a correction to what the marker's own KDoc
|
||||
says. All six carry
|
||||
`@FailsOnEmulatorApi37` and run in a separate `continue-on-error` job; the gating leg runs the
|
||||
other 64 — **the same 64 as before**, which is exactly how this paragraph went stale unnoticed.
|
||||
|
||||
**These two numbers move with the suite and are derived, not remembered.** `grep -cE
|
||||
'^\s*@Test' ` over `app/src/androidTest` is the first; the second is that minus the marker
|
||||
@@ -114,7 +122,7 @@ days. Read it as the current answer, and see the git history if you need the old
|
||||
describes everything in it. The name is kept deliberately — it is not a required context and
|
||||
people have learned to look for it — so **read the marker, not the name**, for what it holds.
|
||||
**It is red on every PR, by design**: do not read it as your change breaking something, and do
|
||||
not read a green run as evidence those five tests pass.
|
||||
not read a green run as evidence those six tests pass.
|
||||
`docs/api-37-emulator-crash.md` has the measurements.
|
||||
|
||||
**That instruction is also why nobody looks, so the job now reports its own shape** — expected,
|
||||
@@ -134,7 +142,7 @@ days. Read it as the current answer, and see the git history if you need the old
|
||||
is gradle never returning, so the log it left says nothing about it.
|
||||
|
||||
Still true, and the reason the advisory job is not simply deleted: **API 37 needs a manual check on
|
||||
the Pixel 10 Pro XL before each release.** Those five tests are the one thing CI cannot answer
|
||||
the Pixel 10 Pro XL before each release.** Those six tests are the one thing CI cannot answer
|
||||
for.
|
||||
|
||||
On a device or emulator, build only the ABI it can execute:
|
||||
@@ -339,7 +347,7 @@ install for code that can never run — and on API 37 the full APK does not fit
|
||||
when a fix is for something intermittent.
|
||||
|
||||
**Every number above is `testDebugUnitTest` only, and on 2026-09-05 the instrumented suite got its
|
||||
first read for that reason** — `docs/e2e-read-findings.md`, entries **E1-E6**, tickets
|
||||
first read for that reason** — `docs/e2e-read-findings.md`, entries **E1-E7**, tickets
|
||||
**#223-#230**. Four waves had been steered by a figure that **cannot see `app/src/androidTest` at
|
||||
all**, so nothing had ever asked what those 60 device tests pin, only that they were green.
|
||||
|
||||
@@ -378,6 +386,18 @@ install for code that can never run — and on API 37 the full APK does not fit
|
||||
half. But the *input* bridge needs no documents provider at all, which is what kept #225 headless
|
||||
and is how #238 surfaced.
|
||||
|
||||
**The 2026-09-06 re-check found that the read's own last PR had re-introduced the drift the read
|
||||
was about**, and that is the entry worth carrying forward. #226 moved the suite 69 -> 70 and the
|
||||
markers 5 -> 6 and changed neither the count in this file, the marker's KDoc, nor the two
|
||||
comments in `status_check.yml`. **The gating figure is what hid it**: 69 - 5 and 70 - 6 are both
|
||||
64, so the one number a reader checks against a run had not moved — which is precisely why the
|
||||
paragraph above says to derive these rather than remember them. Worse, two KDoc claims in the new
|
||||
test described a draft rather than the code: it says MP3 was chosen so the setup could not depend
|
||||
on the device's codecs, while the code converts at the default `MP4_H265`/`FAST` and therefore
|
||||
routes on `canEncode(H265)` — the *negation* of the stated reason. **That is E1 and E3's failure
|
||||
mode, committed by the wave that found it.** All of it is fixed; the standing item is **#250**,
|
||||
because #226 proved D4's premise and never drove its delete arm.
|
||||
|
||||
- **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 —
|
||||
a change that is both needs both.
|
||||
|
||||
@@ -9,14 +9,22 @@ package org.libremediaconverter
|
||||
* That is the whole reason there is one annotation rather than a pair of test lists: two lists
|
||||
* drift, and the drift is silent in both directions (a test that runs nowhere reads as green).
|
||||
*
|
||||
* **"Cannot be run" covers two things, and it said only the first until 2026-09-05.** Four of the
|
||||
* five carriers simply fail: three Media3 tests die in the image's own `c2.goldfish.h264.decoder`,
|
||||
* and the SAF rotation test takes the framework down with it. The fifth —
|
||||
* `SafPickerRoundTripTest.pickingAFileThroughTheSystemPickerFillsInTheFileCard` — **passes about
|
||||
* half the time and aborts `system_server` every time**, which is worse for a gating leg than an
|
||||
* honest failure: it fails the leg from the teardown, with no failing test to point at (#108).
|
||||
* The wording was widened rather than the test excused; that test's own KDoc has the four-run
|
||||
* measurement.
|
||||
* **"Cannot be run" covers three things now, and it covered only the first until 2026-09-05.**
|
||||
* Four of the six carriers simply fail: three Media3 tests die in the image's own
|
||||
* `c2.goldfish.h264.decoder`, and the SAF rotation test takes the framework down with it. The
|
||||
* fifth — `SafPickerRoundTripTest.pickingAFileThroughTheSystemPickerFillsInTheFileCard` —
|
||||
* **passes about half the time and aborts `system_server` every time**, which is worse for a
|
||||
* gating leg than an honest failure: it fails the leg from the teardown, with no failing test to
|
||||
* point at (#108). The wording was widened rather than the test excused; that test's own KDoc has
|
||||
* the four-run measurement.
|
||||
*
|
||||
* **The sixth is the new third thing: it is marked by inheritance, not by measurement.**
|
||||
* `SafPickerRoundTripTest.aSaveWritesToTheDocumentTheSystemPickerCreated` (#226) opens the same
|
||||
* picker and then a second DocumentsUI dialog on top of it, so it sits on the same task-snapshot
|
||||
* path its sibling was marked for. It has never been observed at API 37 either way — see the
|
||||
* measurement under [FAILS_ON_EMULATOR_API37_BASELINE], which is why it cannot be. Marking it was
|
||||
* the conservative choice, and **the trigger for revisiting it is the rotation test, not itself**:
|
||||
* while that one truncates the advisory run, nothing downstream of it can report.
|
||||
*
|
||||
* It says only what has been measured: **on the emulator, at API 37.** The same tests pass on a
|
||||
* physical Pixel 10 Pro XL at API 37 and at API 33–36 on the same runner under the same renderer,
|
||||
@@ -46,19 +54,21 @@ annotation class FailsOnEmulatorApi37
|
||||
* keep printing with nothing to compare to, so it announces that it could not read the baseline
|
||||
* rather than falling quiet. If you see that notice, this line is what it means.
|
||||
*
|
||||
* **One number, both checks, and that is what the marker means.** A test carrying it cannot be run
|
||||
* on this image, so the count is simultaneously how many the advisory leg runs and how many fail.
|
||||
* A *smaller* failure count is the interesting direction: it means one of them now passes, which
|
||||
* is the trigger the KDoc above names for deleting the annotation.
|
||||
* **One number, both checks, and that is what the marker was meant to mean.** A test carrying it
|
||||
* cannot be run on this image, so the count is meant to be simultaneously how many the advisory
|
||||
* leg runs and how many fail. A *smaller* failure count is the interesting direction: it means one
|
||||
* of them now passes, which is the trigger the KDoc above names for deleting the annotation.
|
||||
* **Since 2026-09-06 the second half no longer holds in practice** — the run truncates before two
|
||||
* of the six start, which the last paragraph below measures. `expected` still holds, and it is the
|
||||
* field that catches a marker added without changing this number.
|
||||
*
|
||||
* **The picker test is the one to read that sentence carefully for.**
|
||||
* `pickingAFileThroughTheSystemPickerFillsInTheFileCard` was marked on 2026-09-05 for aborting
|
||||
* `system_server` rather than for failing (#108), and on the gating leg it passed two runs of
|
||||
* four. It fails on the advisory leg because the rotation test runs before it and takes the
|
||||
* framework down first — measured, `api37-debug.yml` run 34008889182, which reports
|
||||
* `expected: 4, received: 4, failed: 4` with the four in the order Media3, Media3, rotation,
|
||||
* picker. (Those dispatches predate the third Media3 marker landing on `main`, so their totals
|
||||
* are four rather than five; the ordering they establish is what matters here.)
|
||||
* **The picker tests are the ones to read that sentence carefully for, and the reason changed
|
||||
* on 2026-09-06.** `pickingAFileThroughTheSystemPickerFillsInTheFileCard` was marked on
|
||||
* 2026-09-05 for aborting `system_server` rather than for failing (#108), and on the gating leg
|
||||
* it passed two runs of four. It was recorded here as *failing* on the advisory leg, behind the
|
||||
* rotation test — measured, `api37-debug.yml` run 34008889182, `expected: 4, received: 4,
|
||||
* failed: 4`, in the order Media3, Media3, rotation, picker. (Those dispatches predate the third
|
||||
* Media3 marker, so their totals are four rather than six.)
|
||||
*
|
||||
* **But a second dispatch of the identical configuration reported 4/3/3**, having lost the last
|
||||
* test to the abort rather than to anything about the test list, and that is why
|
||||
@@ -68,6 +78,14 @@ annotation class FailsOnEmulatorApi37
|
||||
* reporting fewer failures than this as one of them now passing; read a truncated one as the
|
||||
* framework having died, which is this job's normal.
|
||||
*
|
||||
* **That is no longer what happens, and the difference is that neither picker test reports at
|
||||
* all.** With six carriers the rotation test truncates the run before them: **all four** advisory
|
||||
* runs at this baseline — 34041156680, 34041593697, 34042397320 and 34043502322 — report
|
||||
* `expected: 6, received: 4, failed: 4`, and the four are the three Media3 tests plus the
|
||||
* rotation. So the advisory leg currently answers for
|
||||
* four of its six, and the comparison below is unaffected only because `failed` is not compared
|
||||
* on a truncated run. Read it as **unmeasured**, not as passing or failing.
|
||||
*
|
||||
* So: adding or removing a [FailsOnEmulatorApi37] means changing this number, in this file, in
|
||||
* the same diff. The report says so on the run itself if you forget — it prints the tree's own
|
||||
* `grep` count beside this one.
|
||||
|
||||
@@ -245,7 +245,17 @@ public final class FixtureDocumentsProvider extends DocumentsProvider {
|
||||
destinationFile(documentId).delete();
|
||||
}
|
||||
|
||||
/** Document ids {@link #deleteDocument} was called with, newest last. */
|
||||
/**
|
||||
* Document ids {@link #deleteDocument} was called with, newest last.
|
||||
*
|
||||
* <p><b>Nothing reads this yet, and that is recorded rather than hidden (#250).</b> It was
|
||||
* added with #226 to assert {@code OutputPublisher.deletePartialOutput} — D4's cleanup — against
|
||||
* a real {@code DocumentsProvider}. #226 only reached the <i>success</i> path, so the
|
||||
* {@code catch} that calls it is still asserted only against {@code FakeSafProvider} under
|
||||
* Robolectric. It is kept because the forcing condition is one {@code openDestination} override
|
||||
* away and #250 says exactly what to add; if that ticket is closed any other way, delete this
|
||||
* and {@link #DELETED} with it rather than leaving an accessor implying coverage.
|
||||
*/
|
||||
public static List<String> deletedDocumentIds() {
|
||||
synchronized (DELETED) {
|
||||
return new ArrayList<>(DELETED);
|
||||
|
||||
@@ -515,9 +515,28 @@ class SafPickerRoundTripTest {
|
||||
*
|
||||
* ## The conversion is setup, not subject
|
||||
*
|
||||
* Save is only offered on `Converted`, so the test converts first. MP3 is chosen because the
|
||||
* router sends it to FFmpeg unconditionally at every API level, so the setup cannot depend on
|
||||
* the device's codecs — #223 is what that costs.
|
||||
* Save is only offered on `Converted`, so the test converts first, at the screen's default
|
||||
* `MP4_H265` / `FAST`. That is **not** codec-independent, and this KDoc claimed the opposite
|
||||
* until 2026-09-06: an earlier draft used MP3 for exactly that reason, and the format had to
|
||||
* move for a different constraint the picker imposes — [convertToTheDefaultFormat] has it.
|
||||
* `MP4_H265` at `FAST` reaches `ConversionRouter`'s `canEncode(H265)` gate, so it runs on
|
||||
* FFmpeg on the emulators (no hardware H265) and on Media3 on the Pixel.
|
||||
*
|
||||
* **That is tolerable here, and #223 is the reason it needs saying.** There, the routing
|
||||
* decided whether the *subject* was reached, so a route to FFmpeg made the test pass while
|
||||
* proving nothing. Here the conversion is setup: if it goes the other way and fails, this test
|
||||
* fails loudly on the setup rather than quietly on the assertion. The subject is what `publish`
|
||||
* was handed, which the engine that produced the file does not touch.
|
||||
*
|
||||
* ## Why it carries [FailsOnEmulatorApi37]
|
||||
*
|
||||
* By inheritance, not measurement. It opens the same picker as
|
||||
* [pickingAFileThroughTheSystemPickerFillsInTheFileCard], which was marked for aborting
|
||||
* `system_server` from the task-snapshot path (#108), and then a second DocumentsUI dialog on
|
||||
* top of it. It has never been observed at API 37 either way: the rotation test truncates the
|
||||
* advisory run first, so all four advisory runs at this baseline report
|
||||
* `expected: 6, received: 4` without reaching either picker test. Marking it was the conservative choice and it is
|
||||
* recorded as unmeasured in `FailsOnEmulatorApi37.kt` rather than dressed up as a measurement.
|
||||
*/
|
||||
@Test
|
||||
@FailsOnEmulatorApi37
|
||||
@@ -1060,7 +1079,13 @@ class SafPickerRoundTripTest {
|
||||
/** Short: either the dialog is up almost immediately, or the permission was already held. */
|
||||
const val PERMISSION_DIALOG_MS = 5_000L
|
||||
|
||||
/** A 3 s clip to MP3 on an emulator is about a second; this only bounds a hang. */
|
||||
/**
|
||||
* Only bounds a hang, and it is an order of magnitude clear of the real cost: the whole
|
||||
* test — pick, convert, save — takes **11.8 s** on the API 34 CI leg (run 34043502322).
|
||||
* Deliberately generous because the engine is not fixed: the default `MP4_H265` at `FAST`
|
||||
* lands on FFmpeg on an emulator and on Media3 on real hardware, which is faster rather
|
||||
* than slower — see [convertToTheDefaultFormat].
|
||||
*/
|
||||
const val CONVERSION_TIMEOUT_MS = 120_000L
|
||||
|
||||
/** The copy is a few kilobytes, but it crosses a provider. */
|
||||
|
||||
@@ -382,7 +382,7 @@ decision, not a detail — see **E6** for why no third option exists — and **#
|
||||
| **#223** | `HardwareFallbackTest` never attempts the hardware path on any emulator leg | closed — it skips instead of passing vacuously |
|
||||
| **#224** | Cancelling a *running* native session, in any of the three engines | closed — all three engines |
|
||||
| **#225** | No `content://` input has reached a *successful* conversion — the ffkitsaf bridge | closed, and it found **#238** |
|
||||
| **#226** | `OutputPublisher.publish` against a real `DocumentsProvider` | closed — the premise holds; see E7 |
|
||||
| **#226** | `OutputPublisher.publish` against a real `DocumentsProvider` | closed — the *premise* holds; see E7. The delete **arm** is still unrun: **#250** |
|
||||
| **#227** | The notification's Cancel action has never been fired | closed |
|
||||
| **#228** | `encodesFlacLosslessAudio` and `encodesOpus` pass on any non-empty file | closed |
|
||||
| **#229** | FFmpeg's progress percentage is computed everywhere and asserted nowhere | closed |
|
||||
@@ -401,3 +401,39 @@ unasserted value, it was **a combination of two covered things that no test put
|
||||
the acceptance criterion wave 4 established and which caught two vacuous tests in that wave before
|
||||
they shipped. #223 is the one that shows why the criterion matters: it has two passing assertions and
|
||||
still tests nothing.
|
||||
|
||||
## The 2026-09-06 re-check
|
||||
|
||||
Run after the last ticket landed, to ask whether the suite's self-description had drifted again. It
|
||||
had, and **every drifted line came from #226 — the last PR of this read's own wave.**
|
||||
|
||||
The suite is 70 tests in 14 classes, 6 carrying `@FailsOnEmulatorApi37`, gating leg 64; the
|
||||
committed baseline says 6 and the advisory job agrees (`baseline: matches`). Every gating leg is
|
||||
green on `main`.
|
||||
|
||||
- **The counts had gone stale in four places** — `CLAUDE.md` (three sites),
|
||||
`FailsOnEmulatorApi37.kt`'s KDoc, and two comments in `status_check.yml` — all still saying five
|
||||
carriers of 69. **The gating figure is what hid it**: 69 − 5 and 70 − 6 are both 64, so the one
|
||||
number a reader would check against a run had not moved. CLAUDE.md's own instruction to derive
|
||||
these rather than remember them is what caught it.
|
||||
- **Two KDoc claims in `SafPickerRoundTripTest` described a draft rather than the code.** The save
|
||||
test says MP3 was chosen so the setup could not depend on device codecs; the code converts at the
|
||||
default `MP4_H265` / `FAST`, which routes by `canEncode(H265)`. The *negation* of the stated
|
||||
reason was true. This is **E1 and E3's failure mode landing in a test written by the read that
|
||||
found it** — a passing test with a wrong explanation.
|
||||
- **Neither picker test has ever reported on the advisory leg.** The marker's KDoc said the picker
|
||||
test *fails* there behind the rotation test; with six carriers the rotation test truncates the run
|
||||
first, and all four advisory runs at this baseline (`34041156680`, `34041593697`,
|
||||
`34042397320`, `34043502322`) report `expected: 6, received: 4, failed: 4` — the three Media3
|
||||
tests plus the rotation. The save test is therefore
|
||||
marked by **inheritance, not measurement**, which is now what both KDocs say.
|
||||
- **One substantive gap, filed as #250.** `FixtureDocumentsProvider.deletedDocumentIds()` has no
|
||||
callers. #226 proved D4's *premise* — SAF hands back a document of exactly zero bytes — but drove
|
||||
only the success path, so `deletePartialOutput` against a real `DocumentsProvider` is still
|
||||
asserted nowhere. `openDestination` is `protected open` precisely to force the failure, so the
|
||||
test is cheap; it costs another marked picker test and a baseline of 7.
|
||||
|
||||
**The reusable part is the second bullet.** A read that fixes documentation drift can introduce it in
|
||||
the same wave, and the tests it writes are no more self-describing than the ones it audited. The
|
||||
check that found it is the one this document already recommends: **read the KDoc against the code,
|
||||
not against the ticket.**
|
||||
|
||||
Reference in New Issue
Block a user