diff --git a/.github/workflows/status_check.yml b/.github/workflows/status_check.yml index 19ccdab..5b1aaa7 100644 --- a/.github/workflows/status_check.yml +++ b/.github/workflows/status_check.yml @@ -247,14 +247,21 @@ jobs: # .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. + # "this leg" and not "this suite", since 2026-08-24, and the difference is + # now load-bearing: SafPickerRoundTripTest DOES touch system UI. It drives + # DocumentsUI and rotates the display, and both reach the gralloc mapper + # this image aborts in -- disabling SystemUI removes the IDLE trigger, not + # those. Measured per method on android-37.0: the ROTATION test takes the + # framework down (INSTRUMENTATION_ABORTED) and carries + # @FailsOnEmulatorApi37, so notAnnotation below keeps it off this row; the + # PICKER test passes and runs here like anything else. A rotation rebuilds + # every surface at once, and starting another app's activity does not. + # + # So this row does now run one test that depends on system UI, and the + # caveat above still applies to it: a green here is not evidence the picker + # works on a device with SystemUI running -- the Pixel release check is. + # docs/api-37-emulator-crash.md has the per-method measurements, and the + # correction that produced them. # # 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 51d122b..ed9bc91 100644 --- a/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt +++ b/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt @@ -10,6 +10,7 @@ import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry import androidx.test.uiautomator.By import androidx.test.uiautomator.BySelector +import androidx.test.uiautomator.StaleObjectException import androidx.test.uiautomator.UiDevice import androidx.test.uiautomator.Until import org.junit.After @@ -74,36 +75,37 @@ import org.libremediaconverter.ui.TestTags * `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 + * ### Why only the rotation test carries [FailsOnEmulatorApi37] * - * 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 + * This class 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: + * the deviation the API 37 leg already makes -- removes the *idle* trigger, not this one. + * + * The marker is on one method and not on the class, because that is what was measured, one method + * per fresh emulator, on `android-37.0` under `swangle_indirect`: * * ``` - * 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) + * thePickedInputSurvivesARealRotation INSTRUMENTATION_ABORTED: System has crashed. + * Expected 1 tests, received 0 + * pickingAFileThroughTheSystemPickerFillsInTheFileCard PASSED * ``` * - * 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. + * A rotation rebuilds every surface on screen at once, which the mapper does not survive; merely + * starting DocumentsUI does not. + * + * **The first version of this said the class, and it was wrong.** The picker test had failed at + * API 37 too -- with a `StaleObjectException` that turned out to be this file's own bug rather + * than the image's, and which CI then reproduced deterministically at API 33, 34 and 35. Fixing + * it ([tapPickerNode]) and re-measuring is what separated the two. An annotation is a claim about + * an image, and a broken test makes every image look broken; **re-measure after fixing a test + * before deciding what the platform did.** + * + * The annotation says only that, and CI reads it twice, so the rotation test runs on the advisory + * API 37 leg and not 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 { @@ -162,6 +164,7 @@ class SafPickerRoundTripTest { } @Test + @FailsOnEmulatorApi37 fun thePickedInputSurvivesARealRotation() { pickTheFixture() // The identity hash rather than the Activity itself, so nothing here keeps a destroyed @@ -213,20 +216,56 @@ class SafPickerRoundTripTest { // types against Root.COLUMN_MIME_TYPES and drops the roots that cannot answer, so a filter // the fixture root does not satisfy takes the root out of the picker altogether -- along // with "Images", "Audio", "Videos" and "Documents", measured on API 34. - val root = awaitPickerNode(By.text(FixtureDocumentsProvider.ROOT_TITLE)) { + tapPickerNode(By.text(FixtureDocumentsProvider.ROOT_TITLE)) { // Which screen the picker opens on is its own business: it lands on Recent, where the // roots are a strip at the bottom, but a device with a populated Recent may need the // drawer. Looking in the second place widens where the root is searched for; it does // not weaken what has to be found, which is still this root. device.findObject(By.desc(SHOW_ROOTS_DESCRIPTION))?.click() } - root.click() - awaitPickerNode(By.text(FixtureDocumentsProvider.FIXTURE_DISPLAY_NAME)).click() + tapPickerNode(By.text(FixtureDocumentsProvider.FIXTURE_DISPLAY_NAME)) awaitNode(TestTags.Converter.FILE_CARD_NAME) } + /** + * Finds the picker node [selector] names and taps it, re-finding it if it goes stale. + * + * **The re-finding is not padding, and this is not a retry of the assertion.** A `UiObject2` + * holds an `AccessibilityNodeInfo` captured when it was found, and DocumentsUI is still + * settling when the node first appears — its list rebinds, the roots strip lays out, a window + * animates. If the node is replaced in that gap, `click()` throws `StaleObjectException` + * against the handle rather than missing the target. Measured on a cold API 34 emulator: + * + * ``` + * androidx.test.uiautomator.StaleObjectException + * at androidx.test.uiautomator.UiObject2.getAccessibilityNodeInfo(UiObject2.java:1042) + * at androidx.test.uiautomator.UiObject2.click(UiObject2.java:526) + * ``` + * + * So what is retried is *acquiring a handle to a node that has to be there anyway* — every + * attempt still goes through [awaitPickerNode], which fails outright if the node is absent. + * The MIME mutation's bite is untouched: a root that is not in the picker is not found on any + * attempt, and the failure is still "the system picker never showed" rather than a stale one. + */ + private fun tapPickerNode(selector: BySelector, ifAbsent: () -> Unit = {}) { + var stale: StaleObjectException? = null + repeat(TAP_ATTEMPTS) { attempt -> + // ifAbsent only on the first attempt: it navigates, and re-navigating from a screen it + // already reached would walk away from the node. + val node = awaitPickerNode(selector, if (attempt == 0) ifAbsent else ({})) + device.waitForIdle() + try { + node.click() + return + } catch (e: StaleObjectException) { + stale = e + } + } + throw AssertionError("$selector kept going stale between finding it and tapping it", stale) + } + /** * The picker node [selector] names, or a failure that says which one was missing. * @@ -270,6 +309,16 @@ class SafPickerRoundTripTest { /** `Surface.ROTATION_0`, named rather than `0` so the comparison reads. */ const val NATURAL_ROTATION = 0 + /** + * How many times a picker node may be re-found before its staleness is the finding. + * + * Three, not "until the timeout". Each attempt already waits up to [PICKER_TIMEOUT_MS] for + * the node to exist, so this bounds only the settling window after it does; a node that is + * still being replaced after three of those is telling you something about the device, and + * a loop that hid it would be the flake rather than the fix. + */ + const val TAP_ATTEMPTS = 3 + /** DocumentsUI's drawer button. It carries no text, only this description. */ const val SHOW_ROOTS_DESCRIPTION = "Show roots" diff --git a/docs/api-37-emulator-crash.md b/docs/api-37-emulator-crash.md index 3e53fdd..5051964 100644 --- a/docs/api-37-emulator-crash.md +++ b/docs/api-37-emulator-crash.md @@ -331,39 +331,55 @@ and proceeding straight to the tests fails exactly as before. The harness theref 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 +### Something does depend on system UI now, and half of it is excluded -Added 2026-08-24, and it is the first entry on this page that is not a codec. +Added 2026-08-24, and 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: +(RegionSamplingThread's nav-bar luma sampling) and not this one. -| test | how it fails | `hasReadColorBufferDma` aborts in the window | +Measured one method per fresh emulator, `android-37.0`, `swangle_indirect`, SystemUI disabled and +verified quiet — separately, because inferring the second from the first is the mistake this +page's opening correction is about: + +| test | result on android-37.0 | `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 | +| `thePickedInputSurvivesARealRotation` | **fails**: `INSTRUMENTATION_ABORTED: System has crashed.`, `Expected 1 tests, received 0`. The framework dies **during** it, so the JUnit XML carries a failure with no text at all. | 3 | +| `pickingAFileThroughTheSystemPickerFillsInTheFileCard` | **passes** | 4 | -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. +So a rotation, which rebuilds every surface at once, is what the mapper does not survive. Merely +starting DocumentsUI is not. Only the rotation test carries `@FailsOnEmulatorApi37`; the picker +test runs on the gating leg like anything else. -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. +#### The correction that produced that table -Two consequences worth stating rather than discovering: +**The first version of this section said both tests failed, and put the marker on the class.** The +picker test had indeed failed at API 37 — with `androidx.test.uiautomator.StaleObjectException`, +which looked like a framework restart invalidating an accessibility node, because that is exactly +what it looks like. -- **`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. +It was the test's own bug. `UiObject2` caches the `AccessibilityNodeInfo` it was found with, and +DocumentsUI is still settling when a node first appears; the handle went stale before `click()`. +CI then reproduced it **deterministically** at API 33, 34 and 35 — every cold runner emulator, not +intermittently — which is what made it obviously not an API 37 property. It had passed locally +only because the emulator was warm. + +The lesson is worth more than the measurement: **an annotation is a claim about an image, and a +broken test makes every image look broken.** Re-measure after fixing a test before deciding what +the platform did. Both the abort and the stale node produce "the run fell over", and only one of +them was the image. + +#### Two consequences worth stating rather than discovering + +- **`run-e2e.sh 37` applies no annotation filter**, unlike CI, so a local API 37 run includes the + rotation test and therefore **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 now + carries a test that is 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 diff --git a/tools/local-emulator/run-e2e.sh b/tools/local-emulator/run-e2e.sh index 417a8dd..095f042 100755 --- a/tools/local-emulator/run-e2e.sh +++ b/tools/local-emulator/run-e2e.sh @@ -15,11 +15,13 @@ # EXIT CODE: 0 only if every level was green; 1 if any level failed, wedged or could not be # set up; 2 if it refused to start at all. **A bare `run-e2e.sh` therefore exits 1 by design.** # API 37 is in the default list on purpose -- leaving it out is what left the level unlooked-at -# for as long as it was -- and it is permanently two failures short of green, on the emulator's -# own c2.goldfish.h264.decoder rather than on anything this app does. The summary names the two, -# so a third is visibly new, and the last line printed says the same thing. Anything that reads a -# non-zero exit as breakage should name the levels it wants: `run-e2e.sh 33 34 35 36` is the -# sweep that can be green. docs/api-37-emulator-crash.md has the measurements. +# for as long as it was -- and it is permanently short of green, on the emulator image rather +# than on anything this app does. Since 2026-08-24 it does not even FINISH: one of its expected +# failures kills the framework, so the totals come back short with an arbitrary tail. The summary +# names every failure it expects, so an unnamed one is visibly new, and the last line printed +# says the same thing. Anything that reads a non-zero exit as breakage should name the levels it +# wants: `run-e2e.sh 33 34 35 36` is the sweep that can be green. +# docs/api-37-emulator-crash.md has the measurements. # # WHY THIS EXISTS, AND WHAT IT DELIBERATELY DOES NOT DO # @@ -326,12 +328,14 @@ boot_emulator() { # # "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. +# removes the IDLE trigger, not those. Measured per method on android-37.0: the ROTATION test +# takes the framework down (INSTRUMENTATION_ABORTED) and carries @FailsOnEmulatorApi37; the +# picker test passes. +# +# THIS SCRIPT APPLIES NO ANNOTATION FILTER, unlike CI, so a local `run-e2e.sh 37` runs the +# rotation test anyway -- and because that test kills the framework rather than merely failing, +# THE LEVEL DOES NOT FINISH. Its totals come back short and which later tests ran is arbitrary. +# CI's gating leg never sees it. # # 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 @@ -577,20 +581,21 @@ for api in "${APIS[@]}"; do # 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". + # Media3EngineTest failures are a codec; the third is the gralloc bug reached through system + # UI, and it takes the framework DOWN rather than merely failing -- so the level does not + # finish, and the totals come back SHORT (50 of 59 when this was written) with the later + # tests never run. A run whose totals do not add up is expected here now, which it never + # was before. case "$api" in 37 | 37.*) line="$line - 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" + expected here: 2 Media3EngineTest failures on c2.goldfish.h264.decoder, plus + SafPickerRoundTripTest.thePickedInputSurvivesARealRotation -- which kills the framework + rather than merely failing, so the run ABORTS partway and the total comes back SHORT with + an arbitrary tail. That is expected here too, and never was before. Anything else is new. + CI's gating leg sees only the first two: the rotation test carries @FailsOnEmulatorApi37 + and this script, unlike CI, applies no annotation filter. + docs/api-37-emulator-crash.md" ;; esac if [ "$rc" -ne 0 ]; then