From a3c835b7c9ebe100ee2fec3fb7a6441154dd80e8 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 24 Aug 2026 20:58:34 -0500 Subject: [PATCH] Keep the picker test off the API 37 gating leg, having measured why The API 37 emulator images abort surfaceflinger inside the guest's Gralloc5 mapper, init SIGKILLs zygote with it, and the framework restarts under the run. run-e2e.sh and the CI leg disable SystemUI to remove the trigger -- but that removes the IDLE one, RegionSamplingThread's nav-bar luma sampling. Driving DocumentsUI and rotating the display are not idle. They are the first things in this suite that generate surface traffic of their own. Both tests were measured on android-37.0 under swangle_indirect with SystemUI disabled and verified quiet, and measured SEPARATELY -- inferring the second from the first is the mistake docs/api-37-emulator-crash.md opens by correcting. They fail in the two shapes a framework restart produces: thePickedInputSurvivesARealRotation INSTRUMENTATION_ABORTED: System has crashed. Expected 59 tests, received 50 (5 hasReadColorBufferDma aborts; the framework dies DURING the test, so six later tests never run and the XML carries a failure with no text at all) pickingAFileThroughTheSystemPickerFillsInTheFileCard androidx.test.uiautomator.StaleObjectException at androidx.test.uiautomator.UiObject2.click(UiObject2.java:526) (3 aborts; the picker's root node was rebuilt between finding it and tapping it) Both pass on API 33 and API 36 locally -- whole suite, 59/0/0/2 on each -- which is the same evidence pattern that made the Media3EngineTest pair the image rather than the app. So the class carries @FailsOnEmulatorApi37 and runs on the advisory leg. THREE PLACES SAID "nothing in this suite touches system UI", and that is what makes the SystemUI-disable deviation defensible. It is no longer true of the suite, and all three are corrected rather than left to rot -- the workflow comment, run-e2e.sh's header, and the doc. The rule they state is being APPLIED, not broken: the thing that depends on system UI is excluded from the leg that cannot be trusted for it. Two consequences stated rather than left to be discovered: - run-e2e.sh applies no annotation filter, unlike CI, so a local `run-e2e.sh 37` reports these two on top of the Media3 pair AND DOES NOT FINISH. Its totals come back short and which later tests ran is arbitrary. The summary row now says so; it previously promised "exactly two failures", which would have read as a regression in someone else's diff. - The advisory job is still named "E2E API 37 Media3 hardware transcode", and half of what it now runs is neither. Renaming a check touches branch protection, so it is deliberately not done here; the doc records the staleness and the revisit trigger now says the marker covers two unrelated bugs that can go green apart. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/status_check.yml | 11 ++- .../saf/SafPickerRoundTripTest.kt | 33 ++++++++- docs/api-37-emulator-crash.md | 70 +++++++++++++++---- tools/local-emulator/run-e2e.sh | 39 ++++++++--- 4 files changed, 130 insertions(+), 23 deletions(-) diff --git a/.github/workflows/status_check.yml b/.github/workflows/status_check.yml index 1c95fff..19ccdab 100644 --- a/.github/workflows/status_check.yml +++ b/.github/workflows/status_check.yml @@ -240,13 +240,22 @@ jobs: # CAVEAT, read this before trusting a green here: this leg runs with # SystemUI disabled and the framework restarted under it. No other leg # and no Pixel run uses that configuration. It is defensible only because - # nothing in this suite touches system UI -- these are Media3, FFmpeg and + # nothing THIS LEG RUNS touches system UI -- Media3, FFmpeg and # WorkManager tests -- and because the alternative is no CI coverage of # the level this app targets. **Anything that ever does depend on system # UI must not trust this row.** E2E_DISABLE_SYSTEM_UI is what does it; # .github/scripts/e2e-run.sh explains the mechanism and why every step of # it is verified rather than assumed. # + # "this leg" and not "this suite", since 2026-08-24: the suite now has a + # test that DOES touch system UI. SafPickerRoundTripTest drives DocumentsUI + # and rotates the display, both of which reach the gralloc mapper this image + # aborts in -- disabling SystemUI removes the idle trigger, not that one. It + # was measured failing here, per test, and carries @FailsOnEmulatorApi37, so + # notAnnotation below keeps it off this row. The rule the caveat states is + # doing its job rather than being violated; docs/api-37-emulator-crash.md + # has both failures. + # # api-level must be "37.0". A bare 37 is not an SDK package and fails # during setup, which cost a run to discover. # diff --git a/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt b/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt index c245bee..51d122b 100644 --- a/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt +++ b/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt @@ -17,6 +17,7 @@ import org.junit.Assert.assertNotEquals import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith +import org.libremediaconverter.FailsOnEmulatorApi37 import org.libremediaconverter.MainActivity import org.libremediaconverter.ui.TestTags @@ -70,9 +71,39 @@ import org.libremediaconverter.ui.TestTags * * The Pixel 10 Pro XL is secure-locked and cannot be unlocked from a shell, so the picker cannot be * driven there at all. That is why this gap survived as long as it did. - * `tools/local-emulator/run-e2e.sh` runs API 33-36 on the development host. + * `tools/local-emulator/run-e2e.sh` runs API 33-36 on the development host, and both tests pass + * there: **59 / 0 / 0 / 2 at API 33 and again at API 36**, whole suite, 2026-08-24. + * + * ### Why [FailsOnEmulatorApi37] is on this class + * + * Measured, per that annotation's own rule, and measured **per test** rather than inferred from + * one of them — see `docs/api-37-emulator-crash.md`, which this is the first entry in that is not + * a codec. + * + * This is the first thing in the suite that touches system UI, and the android-37.x images are + * where that stops being free: surfaceflinger aborts inside the guest's Gralloc5 mapper, init + * SIGKILLs zygote with it, and the framework restarts underneath the run. Disabling SystemUI -- + * the deviation the API 37 leg already makes -- removes the *idle* trigger, not this one. Driving + * DocumentsUI and rotating the display generate exactly the surface traffic that reaches the + * mapper. Both tests fail on `android-37.0` under `swangle_indirect` with SystemUI disabled, and + * they fail in the two shapes a framework restart produces: + * + * ``` + * thePickedInputSurvivesARealRotation + * INSTRUMENTATION_ABORTED: System has crashed. (5 hasReadColorBufferDma aborts; the run + * Expected 59 tests, received 50 never finished, taking 6 later tests out) + * + * pickingAFileThroughTheSystemPickerFillsInTheFileCard + * androidx.test.uiautomator.StaleObjectException (3 aborts; the picker's root node was + * at UiObject2.click(UiObject2.java:526) rebuilt between finding it and tapping it) + * ``` + * + * The annotation says only that, and CI reads it twice, so this class runs on the advisory API 37 + * leg and not on the gating one. **Do not read it as "a rotation is allowed to lose the file".** + * That is what API 33 through 36 are for, and they answer it. */ @UnstableApi +@FailsOnEmulatorApi37 @RunWith(AndroidJUnit4::class) class SafPickerRoundTripTest { diff --git a/docs/api-37-emulator-crash.md b/docs/api-37-emulator-crash.md index ff93b26..3e53fdd 100644 --- a/docs/api-37-emulator-crash.md +++ b/docs/api-37-emulator-crash.md @@ -230,11 +230,17 @@ booted (`emulator_alive=yes`). The host emulator is fine; the guest is not. ## Can the suite run on it? -**Almost.** `tools/local-emulator/run-e2e.sh 37` now runs the whole suite locally, and all of it -passes except two tests. Measured at `22c7914`: **49 tests, 2 failures, 0 errors, 2 skipped** — 45 -passed, the two `Media3EngineTest` failures dissected below, and the two `assumeTrue` skips every -level has. It costs two deviations from how every other level is run, and both are worth -understanding before trusting the leg. +**Almost, and less so than it was.** `tools/local-emulator/run-e2e.sh 37` runs the whole suite +locally. Measured at `22c7914`: **49 tests, 2 failures, 0 errors, 2 skipped** — 45 passed, the two +`Media3EngineTest` failures dissected below, and the two `assumeTrue` skips every level has. It +costs two deviations from how every other level is run, and both are worth understanding before +trusting the leg. + +**That was the high-water mark.** On 2026-08-24 a test that touches system UI joined the suite, +and the level stopped *finishing* rather than merely failing two — +[see below](#something-does-depend-on-system-ui-now-and-it-is-excluded-rather-than-trusted). +Two `Media3EngineTest` failures is what **CI's gating leg** expects, because it filters on +`notAnnotation`; a local `run-e2e.sh 37` does not filter and sees more. Two things about that total before it is compared with anything. It is the size of the suite on the checkout that ran, not a property of API 37 — `app/src/androidTest` held 49 `@Test` methods at @@ -320,10 +326,44 @@ and proceeding straight to the tests fails exactly as before. The harness theref 1. **The renderer is ANGLE, not the host GPU.** Shared with nothing else in the matrix — API 33–36 run `-gpu host` locally, and CI runs `swiftshader_indirect`. 2. **SystemUI is disabled.** The API 37 leg does not run the same device configuration as any - other leg or as the Pixel. It is defensible here only because nothing in this suite touches - system UI — these are Media3, FFmpeg and WorkManager tests — and because the alternative is no - local API 37 coverage at all. **Anything that ever does depend on system UI must not trust - this leg.** + other leg or as the Pixel. It was defensible here because nothing in this suite touched + system UI — Media3, FFmpeg and WorkManager tests — and because the alternative is no local + API 37 coverage at all. **Anything that ever does depend on system UI must not trust this + leg.** Something now does; see the section below. + +### Something does depend on system UI now, and it is excluded rather than trusted + +Added 2026-08-24, and it is the first entry on this page that is not a codec. + +`SafPickerRoundTripTest` drives the real system file picker and rotates the display. Both reach +the gralloc mapper — DocumentsUI is another app's windows, and a rotation rebuilds every surface +on screen — and **disabling SystemUI does not help**, because it removes the *idle* trigger +(RegionSamplingThread's nav-bar luma sampling) and not this one. The two tests fail on +`android-37.0` under `swangle_indirect` with SystemUI disabled and verified quiet, and they were +measured **separately**, because inferring the second from the first would have been the same +mistake this page's opening correction is about: + +| test | how it fails | `hasReadColorBufferDma` aborts in the window | +|---|---|---| +| `thePickedInputSurvivesARealRotation` | `INSTRUMENTATION_ABORTED: System has crashed.` — `Expected 59 tests, received 50`. The framework dies **during** it, so six later tests never run and the JUnit XML carries a failure with no text at all. | 5 | +| `pickingAFileThroughTheSystemPickerFillsInTheFileCard` | `androidx.test.uiautomator.StaleObjectException` at `UiObject2.click`, tapping the fixture root the previous line had just found. A restart rebuilt the window between the two. | 3 | + +Both pass on API 33 and API 36 locally, whole suite, `59 / 0 / 0 / 2` on each — so this is the +image, on the same evidence pattern as the codec failures above. + +The class therefore carries `@FailsOnEmulatorApi37` and runs on the advisory leg. **The rule the +deviation states is being applied, not broken:** the thing that depends on system UI does not +trust this leg. + +Two consequences worth stating rather than discovering: + +- **`run-e2e.sh 37` applies no annotation filter**, so a local API 37 run reports these on top of + the two `Media3EngineTest` ones, and — new — **does not finish**. Its totals come back short, + and which later tests ran is arbitrary. The summary row says so. +- **The advisory job is still named `E2E API 37 Media3 hardware transcode (advisory)`**, and it + now carries two tests that are neither Media3 nor a transcode. Renaming a check is a branch- + protection change and was deliberately not made in the same PR; the name is stale, the + behaviour is correct. ### The two remaining failures are the same bug, one layer down @@ -650,9 +690,15 @@ though a new API level shipped. Watch for these instead: - **`E2E API 37 Media3 hardware transcode (advisory)` going green.** Nothing announces this: the job is `continue-on-error`, so it fixing itself looks exactly like a check nobody reads quietly ceasing to be red. It is listed here because that makes it the *least* likely of these - triggers to be noticed, not the most. When it happens, delete `@FailsOnEmulatorApi37` from the - two tests rather than the job — the gating leg picks them back up on its own, and the advisory - job then runs nothing and can go. + triggers to be noticed, not the most. When it happens, delete `@FailsOnEmulatorApi37` from + everything carrying it rather than deleting the job — the gating leg picks them back up on its + own, and the advisory job then runs nothing and can go. + + **It is not two tests any more.** As of 2026-08-24 the marker is on `Media3EngineTest`'s two + methods *and* on `SafPickerRoundTripTest` as a class, and the two groups fail for unrelated + reasons — a codec and the gralloc mapper. They can go green independently, so check both before + concluding the marker is done; and the job's name still says "Media3 hardware transcode", which + half of what it runs is not. ## Correction owed to `CLAUDE.md` diff --git a/tools/local-emulator/run-e2e.sh b/tools/local-emulator/run-e2e.sh index ed34159..417a8dd 100755 --- a/tools/local-emulator/run-e2e.sh +++ b/tools/local-emulator/run-e2e.sh @@ -320,9 +320,18 @@ boot_emulator() { # # THIS IS A DEVIATION, and it is deliberately loud rather than silent. The API 37 leg does not # run the same device configuration as API 33-36 or as the Pixel. It is defensible only -# because nothing in this suite touches SystemUI -- these are Media3, FFmpeg and WorkManager -# tests -- and because the alternative is no API 37 coverage at all. Anything that ever does -# depend on system UI must not trust this leg. docs/api-37-emulator-crash.md explains why. +# because nothing in this suite touched SystemUI -- Media3, FFmpeg and WorkManager tests -- +# and because the alternative is no API 37 coverage at all. Anything that ever does depend on +# system UI must not trust this leg. docs/api-37-emulator-crash.md explains why. +# +# "Touched", past tense, since 2026-08-24. SafPickerRoundTripTest drives DocumentsUI and rotates +# the display, and both reach the gralloc mapper these images abort in -- disabling SystemUI +# removes the IDLE trigger, not that one. Unlike CI, THIS SCRIPT APPLIES NO ANNOTATION FILTER, so +# a local `run-e2e.sh 37` runs it and reports a third and fourth failure on top of the two +# Media3EngineTest ones the summary names. Measured 2026-08-24: the rotation test takes the +# framework down outright (INSTRUMENTATION_ABORTED, and six later tests never run), the picker +# test dies on a StaleObjectException. CI's gating leg does not see either -- they carry +# @FailsOnEmulatorApi37 and it filters on notAnnotation. # # The retry loop is not defensive padding: at the moment boot_completed flips, the framework # may be in one of its restarts and `pm` is simply not published yet. The first attempt at this @@ -564,13 +573,24 @@ for api in "${APIS[@]}"; do guest_forensics "$api" # API 37 is in the default list on purpose, and it is expected to be red. Leaving it out would # put the level back where this whole exercise found it -- untested and unlooked-at -- but a - # summary that just says "2 failures" with no explanation trains people to ignore the exit - # code. So the row says which two, and a THIRD failure is then obviously new. + # summary that just says "N failures" with no explanation trains people to ignore the exit + # code. So the row NAMES the expected ones, and anything else is then obviously new. + # + # The list grew on 2026-08-24 and the shape of the row changed with it. The two + # Media3EngineTest failures are a codec; SafPickerRoundTripTest is the gralloc bug reached + # through system UI, and its rotation test takes the framework down rather than merely + # failing -- so that level does not finish, and the totals come back SHORT (50 of 59 at the + # time of writing) with the later tests never run. A run whose totals do not add up is + # therefore expected here too, which it never was before, and is the reason this says + # "at least". case "$api" in 37 | 37.*) line="$line - expected here: 2 failures, both Media3EngineTest, on c2.goldfish.h264.decoder. - A third is new -- docs/api-37-emulator-crash.md" + expected here, at least: 2 Media3EngineTest failures on c2.goldfish.h264.decoder, plus + both SafPickerRoundTripTest tests -- and the run ABORTS partway, so the total is short + and which later tests ran is arbitrary. Anything else is new. + CI's gating leg sees only the first two: the SAF class carries @FailsOnEmulatorApi37 and + this script, unlike CI, applies no annotation filter. docs/api-37-emulator-crash.md" ;; esac if [ "$rc" -ne 0 ]; then @@ -596,8 +616,9 @@ echo "==============================================================" # worse than no note at all. if [ "$overall" -ne 0 ] && [ "$NON37_RED" -eq 0 ]; then echo "note: the only level that went red is API 37, which exits non-zero by design -- it is" - echo " permanently 2 failures short of green. Confirm its row above shows exactly those" - echo " two and nothing else; docs/api-37-emulator-crash.md says why they are the image." + echo " permanently short of green, and since 2026-08-24 it does not even finish. Confirm" + echo " its row above names every failure it shows; docs/api-37-emulator-crash.md says why" + echo " each of them is the image rather than this app." fi exit "$overall"