diff --git a/.github/workflows/status_check.yml b/.github/workflows/status_check.yml index 1c95fff..5b1aaa7 100644 --- a/.github/workflows/status_check.yml +++ b/.github/workflows/status_check.yml @@ -240,13 +240,29 @@ 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, 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/build.gradle.kts b/app/build.gradle.kts index 6ed9c72..a2565d9 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -235,8 +235,11 @@ val jacocoGeneratedExcludes = listOf( ) // AGP 9 compiles Kotlin through its built-in compiler, which writes here rather than to the -// classic `tmp/kotlin-classes/debug`. All hand-written code in this module is Kotlin, so the -// javac output (BuildConfig and R only) is not read at all. +// classic `tmp/kotlin-classes/debug`. All hand-written code in the MAIN source set is Kotlin, so +// the javac output (BuildConfig and R only) is not read at all. There is now one hand-written +// Java file in the module -- androidTest's FixtureDocumentsProvider, which cannot be Kotlin +// because the process it runs in has no Kotlin stdlib; its own header explains why. It is in +// androidTest, so it is not in this task's classDirectories and this stays accurate. val jacocoDebugKotlinClasses = layout.buildDirectory.dir( "intermediates/built_in_kotlinc/debug/compileDebugKotlin/classes", ) @@ -340,5 +343,11 @@ dependencies { androidTestImplementation(libs.androidx.espresso.core) androidTestImplementation(libs.compose.ui.test.junit4) androidTestImplementation(libs.androidx.work.testing) + // androidTest only, and it has to be: UiAutomator drives the whole device, including + // windows belonging to other packages. The system file picker is one -- DocumentsUI runs + // in its own process, so Compose's matchers cannot see it and Espresso's cannot either + // (both are scoped to this process's view hierarchy). Nothing on the JVM has a device to + // drive, so there is no unit-test counterpart to add it to. + androidTestImplementation(libs.androidx.uiautomator) debugImplementation(libs.compose.ui.test.manifest) } diff --git a/app/src/androidTest/AndroidManifest.xml b/app/src/androidTest/AndroidManifest.xml new file mode 100644 index 0000000..2ed15ba --- /dev/null +++ b/app/src/androidTest/AndroidManifest.xml @@ -0,0 +1,47 @@ + + + + + + + + + + + + + diff --git a/app/src/androidTest/java/org/libremediaconverter/saf/FixtureDocumentsProvider.java b/app/src/androidTest/java/org/libremediaconverter/saf/FixtureDocumentsProvider.java new file mode 100644 index 0000000..c5d7db9 --- /dev/null +++ b/app/src/androidTest/java/org/libremediaconverter/saf/FixtureDocumentsProvider.java @@ -0,0 +1,234 @@ +package org.libremediaconverter.saf; + +import android.database.Cursor; +import android.database.MatrixCursor; +import android.os.CancellationSignal; +import android.os.ParcelFileDescriptor; +import android.provider.DocumentsContract.Document; +import android.provider.DocumentsContract.Root; +import android.provider.DocumentsProvider; + +import java.io.File; +import java.io.FileNotFoundException; +import java.io.FileOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; + +/** + * One file, offered to the system file picker, so that picking one can be tested at all. + * + *

DocumentsUI does not browse a filesystem: it lists what {@link DocumentsProvider}s hand it. + * So a test that drives the real picker has to supply the thing being picked, and it has to + * supply it as a manifest-declared component, because a {@code ContentProvider} is instantiated + * by the system and cannot be registered from test code. {@code + * app/src/androidTest/AndroidManifest.xml} is that declaration and says why each of its + * attributes is load-bearing. + * + *

The only Java file in this module, and it has to be

+ * + *

Everything else here is Kotlin. This cannot be: the Kotlin standard library is not on + * this class's classpath at runtime. + * + *

Instrumentation code normally never notices. The test APK's dex is loaded into the app's + * process, where the app APK supplies {@code kotlin.jvm.internal.Intrinsics} — so the test APK is + * built without it, deliberately, since packaging a second copy is what {@code + * checkDebugAndroidTestDuplicateClasses} exists to prevent. A provider is different. It is a + * component of the instrumentation package, so when DocumentsUI queries it the system + * starts a plain {@code org.libremediaconverter.test} process with only the test APK on its dex + * path, and no app APK anywhere. The Kotlin version of this file crashed there on its first + * query, before returning a single row: + * + * 

+ * FATAL EXCEPTION: binder:6369_2
+ * Process: org.libremediaconverter.test
+ * java.lang.NoClassDefFoundError: Failed resolution of: Lkotlin/jvm/internal/Intrinsics;
+ *     at org.libremediaconverter.saf.FixtureDocumentsProvider.queryDocument
+ *
+ * + *

The compiler emits that reference for the null checks on almost every function, so there is + * no Kotlin dialect that avoids it. For the same reason nothing here imports {@code androidx.*}: + * those classes are absent from this process for exactly the same reason. Framework and JDK only. + * + *

Why a provider rather than a file in Downloads

+ * + *

That would have worked, and it would have tested less. Two properties are what {@code + * SafPickerRoundTripTest} actually needs: + * + *

+ * + *

The descriptor is opened on a real file rather than served through a pipe, deliberately. + * {@code InputQuery.sizeOf} falls back to {@code ParcelFileDescriptor.statSize} when a provider + * omits {@code OpenableColumns.SIZE}, and a pipe's {@code statSize} is {@code -1} — an unknown + * size, which is a different case with a screen of its own. This fixture is meant to be an + * ordinary, fully described file, so that the one thing under test is the round trip. + */ +public final class FixtureDocumentsProvider extends DocumentsProvider { + + /** + * What the picker calls this root. + * + *

Deliberately not a word any other root uses. The picker's own landing screen already + * offers "Images", "Audio", "Videos" and "Documents", and a UiAutomator selector that could + * match two things is not a selector. + */ + public static final String ROOT_TITLE = "LMC R38 fixtures"; + + /** + * What the file card has to end up showing. + * + *

The same string reaches the assertion two ways — as the picker row UiAutomator taps, and + * as {@code OpenableColumns.DISPLAY_NAME} on the URI the app is handed — which is exactly the + * round trip under test. + */ + public static final String FIXTURE_DISPLAY_NAME = "lmc-r38-fixture.mp4"; + + /** + * The type the root advertises, and the one the MIME mutation has to stop matching. + * + *

A real type rather than something invented, so the wildcard filter the screen passes + * today is not the only filter under which this test could pass. + */ + public static final String FIXTURE_MIME_TYPE = "video/mp4"; + + private static final String ROOT_ID = "lmc-r38-root"; + private static final String ROOT_DOCUMENT_ID = "root"; + private static final String FIXTURE_DOCUMENT_ID = "root/" + FIXTURE_DISPLAY_NAME; + + /** Already in this source set, and already a real H.264 MP4 the engines can open. */ + private static final String FIXTURE_ASSET = "sample_h264.mp4"; + + private static final String[] DEFAULT_ROOT_PROJECTION = { + Root.COLUMN_ROOT_ID, + Root.COLUMN_DOCUMENT_ID, + Root.COLUMN_TITLE, + Root.COLUMN_SUMMARY, + Root.COLUMN_MIME_TYPES, + Root.COLUMN_FLAGS, + Root.COLUMN_ICON, + }; + + private static final String[] DEFAULT_DOCUMENT_PROJECTION = { + Document.COLUMN_DOCUMENT_ID, + Document.COLUMN_DISPLAY_NAME, + Document.COLUMN_MIME_TYPE, + Document.COLUMN_FLAGS, + Document.COLUMN_SIZE, + Document.COLUMN_LAST_MODIFIED, + }; + + @Override + public boolean onCreate() { + return true; + } + + /** + * The single root. + * + *

{@link Root#COLUMN_MIME_TYPES} is the important column. Left null it would mean "this + * root supports everything", the picker would list it whatever was asked for, and the MIME + * mutation would have nothing to bite on. + */ + @Override + public Cursor queryRoots(String[] projection) { + MatrixCursor cursor = new MatrixCursor(projection != null ? projection : DEFAULT_ROOT_PROJECTION); + cursor.newRow() + .add(Root.COLUMN_ROOT_ID, ROOT_ID) + .add(Root.COLUMN_DOCUMENT_ID, ROOT_DOCUMENT_ID) + .add(Root.COLUMN_TITLE, ROOT_TITLE) + .add(Root.COLUMN_SUMMARY, "Instrumentation fixture") + .add(Root.COLUMN_MIME_TYPES, FIXTURE_MIME_TYPE) + .add(Root.COLUMN_FLAGS, Root.FLAG_LOCAL_ONLY) + .add(Root.COLUMN_ICON, android.R.drawable.ic_menu_gallery); + return cursor; + } + + @Override + public Cursor queryDocument(String documentId, String[] projection) throws FileNotFoundException { + MatrixCursor cursor = new MatrixCursor(projection != null ? projection : DEFAULT_DOCUMENT_PROJECTION); + if (ROOT_DOCUMENT_ID.equals(documentId)) { + addDirectoryRow(cursor); + } else if (FIXTURE_DOCUMENT_ID.equals(documentId)) { + addFixtureRow(cursor); + } else { + throw new FileNotFoundException("no such document: " + documentId); + } + return cursor; + } + + @Override + public Cursor queryChildDocuments(String parentDocumentId, String[] projection, String sortOrder) + throws FileNotFoundException { + MatrixCursor cursor = new MatrixCursor(projection != null ? projection : DEFAULT_DOCUMENT_PROJECTION); + if (ROOT_DOCUMENT_ID.equals(parentDocumentId)) { + addFixtureRow(cursor); + } + return cursor; + } + + @Override + public ParcelFileDescriptor openDocument(String documentId, String mode, CancellationSignal signal) + throws FileNotFoundException { + if (!FIXTURE_DOCUMENT_ID.equals(documentId)) { + throw new FileNotFoundException("no such document: " + documentId); + } + return ParcelFileDescriptor.open(fixtureFile(), ParcelFileDescriptor.MODE_READ_ONLY); + } + + private void addDirectoryRow(MatrixCursor cursor) { + cursor.newRow() + .add(Document.COLUMN_DOCUMENT_ID, ROOT_DOCUMENT_ID) + .add(Document.COLUMN_DISPLAY_NAME, ROOT_TITLE) + .add(Document.COLUMN_MIME_TYPE, Document.MIME_TYPE_DIR) + .add(Document.COLUMN_FLAGS, 0) + .add(Document.COLUMN_SIZE, null); + } + + private void addFixtureRow(MatrixCursor cursor) throws FileNotFoundException { + File file = fixtureFile(); + cursor.newRow() + .add(Document.COLUMN_DOCUMENT_ID, FIXTURE_DOCUMENT_ID) + .add(Document.COLUMN_DISPLAY_NAME, FIXTURE_DISPLAY_NAME) + .add(Document.COLUMN_MIME_TYPE, FIXTURE_MIME_TYPE) + .add(Document.COLUMN_FLAGS, 0) + .add(Document.COLUMN_SIZE, file.length()) + .add(Document.COLUMN_LAST_MODIFIED, file.lastModified()); + } + + /** + * The fixture on disk, unpacked from this APK's own assets the first time anything asks. + * + *

On demand rather than seeded once in {@link #onCreate()}, because this process is started + * by whoever queries the provider and can be killed between two queries of the same test. + * + *

A failure here is reported as {@link FileNotFoundException} rather than swallowed. A + * provider that answers with a zero-byte file would put the test on the "Size unknown" screen + * with nothing saying why. + */ + private File fixtureFile() throws FileNotFoundException { + File file = new File(getContext().getFilesDir(), FIXTURE_DISPLAY_NAME); + if (file.length() > 0L) { + return file; + } + try (InputStream source = getContext().getAssets().open(FIXTURE_ASSET); + OutputStream sink = new FileOutputStream(file)) { + byte[] buffer = new byte[8192]; + int read; + while ((read = source.read(buffer)) != -1) { + sink.write(buffer, 0, read); + } + } catch (IOException e) { + throw new FileNotFoundException("could not unpack " + FIXTURE_ASSET + ": " + e); + } + return file; + } +} diff --git a/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt b/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt new file mode 100644 index 0000000..ed9bc91 --- /dev/null +++ b/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt @@ -0,0 +1,328 @@ +package org.libremediaconverter.saf + +import androidx.compose.ui.test.assertTextEquals +import androidx.compose.ui.test.junit4.v2.createAndroidComposeRule +import androidx.compose.ui.test.onAllNodesWithTag +import androidx.compose.ui.test.onNodeWithTag +import androidx.compose.ui.test.performClick +import androidx.media3.common.util.UnstableApi +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 +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 + +/** + * Choosing a file, through the real system picker, and still having it after a rotation. + * + * Two defects, and neither is reachable from anywhere else in this repo. + * + * **The picker is opened with a filter, and a filter can hide the user's file.** `ConverterScreen` + * launches `ActivityResultContracts.OpenDocument` with a MIME array; DocumentsUI hides every root + * and every document that array does not match. Narrow it and the app still compiles, still + * renders, still passes every JVM test — and the user taps "Choose file" and is shown an empty + * picker. Nothing in either source set drove SAF **as a picker** before this: the only SAF coverage + * is the publish side, in `OutputPublisherPublishTest`, against hand-written `ContentProvider` + * fakes. The launcher wiring, the filter, and the read grant that comes back had never been + * executed by a test. + * + * **The picked file has to survive a rotation.** `MainActivity` declares no `configChanges`, so + * every rotation destroys and recreates it, and `ConversionViewModel` holds the picked file in a + * plain `MutableStateFlow` with no `SavedStateHandle` behind it. The only thing that carries it + * across is the retained `ViewModelStore` the Activity gets from resolving the ViewModel through + * `LocalViewModelStoreOwner`. Scope it to the composition instead and the file is gone. + * + * ### Why these two are one test class + * + * A rotation test alone has no bite of its own. `AppRootRestorationTest` already catches + * `rememberSaveable` -> `remember` on the JVM, and a second test whose only mutation is one an + * existing test catches is the vacuous test this whole decomposition exists to prevent. So the + * rotation here runs **from a real picked input**, which is a state no JVM test can produce: + * `AppRootRestorationTest` injects a stub `content` lambda specifically to avoid standing up + * either ViewModel, and `StateRestorationTester` saves into an in-memory map rather than a + * `Bundle`. + * + * ### The mutations, and what they printed + * + * Both were run, not asserted. Narrowing the wildcard array `ConverterScreen.kt` passes to + * `pickInput.launch` — to `arrayOf("application/x-lmc-no-such-type")` — empties the picker of the + * fixture root entirely, and [pickingAFileThroughTheSystemPickerFillsInTheFileCard] fails on the + * assertion that names it. + * Making the ViewModel composition-scoped leaves the picker test alone and fails + * [thePickedInputSurvivesARealRotation], with `:app:testDebugUnitTest` still BUILD SUCCESSFUL — + * which is the divergence this ticket was filed to establish, and which was doubted on it. It is + * `viewModel()` -> `viewModel(viewModelStoreOwner = remember { })`, + * **plus** `factory = ViewModelProvider.AndroidViewModelFactory()` and a `MutableCreationExtras` + * carrying `APPLICATION_KEY`. The factory half is not decoration: an owner that is not a + * `HasDefaultViewModelProviderFactory` contributes no creation extras, and the default factory + * cannot construct an `AndroidViewModel` without them — so the owner swap alone crashes on + * construction instead of demonstrating the scope. The PR body quotes both failures verbatim. + * + * ### It has to be an unlocked emulator + * + * 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, and both tests pass + * there: **59 / 0 / 0 / 2 at API 33 and again at API 36**, whole suite, 2026-08-24. + * + * ### Why only the rotation test carries [FailsOnEmulatorApi37] + * + * 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. + * + * 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. + * Expected 1 tests, received 0 + * pickingAFileThroughTheSystemPickerFillsInTheFileCard PASSED + * ``` + * + * 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 +@RunWith(AndroidJUnit4::class) +class SafPickerRoundTripTest { + + @get:Rule + val composeRule = createAndroidComposeRule() + + private val device: UiDevice = + UiDevice.getInstance(InstrumentationRegistry.getInstrumentation()) + + /** Set by the one test that rotates, read by [restoreOrientation]. See its KDoc. */ + private var rotated = false + + /** + * Leave the device the way it was found — and only if this test moved it. + * + * Two things are deliberate here, and both are about the *other* tests on the device rather + * than about these two. + * + * The flag, because this runs after every test in the class, not only the one that rotated. An + * unconditional restore issues a WindowManager rotation request after the picker test as well, + * which has nothing to undo; JUnit does not promise method order, so that is an interaction + * between two tests that no single-class run would ever show. Tracked as a flag rather than + * read back off `isNaturalOrientation`, because a device whose *natural* orientation is + * landscape would answer that question the wrong way round. + * + * And `unfreezeRotation`, because `setOrientationNatural` does not merely rotate: it freezes + * the rotation there. A run that stopped after it would hand the next test a device that + * cannot rotate at all. + */ + @After + fun restoreOrientation() { + if (!rotated) return + device.setOrientationNatural() + device.unfreezeRotation() + device.waitForIdle() + } + + @Test + fun pickingAFileThroughTheSystemPickerFillsInTheFileCard() { + pickTheFixture() + + composeRule.onNodeWithTag(TestTags.Converter.FILE_CARD_NAME) + .assertTextEquals(FixtureDocumentsProvider.FIXTURE_DISPLAY_NAME) + + // Not the same assertion twice. The name above comes from a metadata query, which a URI + // with no read grant answers just as well; this line only appears once something has + // opened the file and read its header. It is what says the picker handed back a URI the + // app can actually USE -- delete grantUriPermissions from the fixture's manifest entry and + // the name still arrives while this goes red. + // + // The whole "Container: MP4" and not "MP4": DetailRow renders the label and the value as + // one semantics node. + awaitNode(TestTags.Converter.detailRow(CONTAINER_LABEL)) + composeRule.onNodeWithTag(TestTags.Converter.detailRow(CONTAINER_LABEL)) + .assertTextEquals("$CONTAINER_LABEL: MP4") + } + + @Test + @FailsOnEmulatorApi37 + fun thePickedInputSurvivesARealRotation() { + pickTheFixture() + // The identity hash rather than the Activity itself, so nothing here keeps a destroyed + // Activity reachable across the recreation it is being used to detect. + val before = System.identityHashCode(composeRule.activity) + + device.setOrientationLandscape() + rotated = true + composeRule.waitForIdle() + + // Two guards before the assertion that matters, because both of the ways this test could + // pass while proving nothing are silent ones. + // + // A device that ignored the rotation request would leave the app exactly as it was, and + // "the file is still there" would then be a statement about a screen nothing happened to. + assertNotEquals( + "the device did not actually rotate, so nothing below is about a rotation", + NATURAL_ROTATION, + device.displayRotation, + ) + // And a rotation that did NOT recreate the Activity -- a configChanges attribute added to + // the manifest, an aspect-ratio or orientation lock -- would make this a recomposition + // test. The retained ViewModelStore is only interesting because the Activity around it + // really was destroyed and rebuilt. + assertNotEquals( + "the rotation did not recreate MainActivity, so the retained ViewModelStore was never used", + before, + System.identityHashCode(composeRule.activity), + ) + + awaitNode(TestTags.Converter.FILE_CARD_NAME) + composeRule.onNodeWithTag(TestTags.Converter.FILE_CARD_NAME) + .assertTextEquals(FixtureDocumentsProvider.FIXTURE_DISPLAY_NAME) + } + + // --- driving the picker --------------------------------------------------------------- + + /** + * Taps "Choose file", walks the system picker to the fixture, and returns once the app has it. + * + * Everything between the first tap and the last belongs to `com.google.android.documentsui`, + * which is why UiAutomator is here at all: Compose's matchers stop at this process's + * composition and Espresso's at its view hierarchy, and the picker is neither. + */ + private fun pickTheFixture() { + composeRule.onNodeWithTag(TestTags.Converter.CHOOSE_FILE).performClick() + + // THIS is the line the MIME filter mutation fails on. DocumentsUI matches the requested + // 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. + 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() + } + + 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. + * + * [ifAbsent] runs once, after the first wait comes up empty, and then the wait is repeated. A + * null return from `findObject` is deliberately not an error there: it is the "already on the + * right screen" case. + */ + private fun awaitPickerNode(selector: BySelector, ifAbsent: () -> Unit = {}) = + device.wait(Until.findObject(selector), PICKER_TIMEOUT_MS) + ?: run { + ifAbsent() + requireNotNull(device.wait(Until.findObject(selector), PICKER_TIMEOUT_MS)) { + "the system picker never showed $selector" + } + } + + /** + * Blocks until [tag] is in the composition, so an assertion cannot race the picker's result. + * + * The described overload of `waitUntil`, not the bare one. A timeout is how both of this + * class's mutations report themselves, and the bare overload's message is + * `Condition still not satisfied after 30000 ms` — which names neither the node nor the test. + * With the description it says which affordance never arrived, which is the whole finding. + */ + private fun awaitNode(tag: String) { + composeRule.waitUntil("a node tagged $tag exists", APP_TIMEOUT_MS) { + composeRule.onAllNodesWithTag(tag).fetchSemanticsNodes().isNotEmpty() + } + } + + private companion object { + + /** + * Generous on purpose. This waits on another app being started, and on FFprobe spawning a + * native process over a `content://` URI; a timeout that merely usually passes is a flaky + * gating leg on five API levels, which costs far more than the seconds it saves. + */ + const val PICKER_TIMEOUT_MS = 30_000L + const val APP_TIMEOUT_MS = 30_000L + + /** `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" + + /** The detail row `MediaProbe` fills in for anything it could open and identify. */ + const val CONTAINER_LABEL = "Container" + } +} diff --git a/docs/api-37-emulator-crash.md b/docs/api-37-emulator-crash.md index ff93b26..5051964 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,60 @@ 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 half of it is excluded + +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. + +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` | **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 | + +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 correction that produced that table + +**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. + +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 @@ -650,9 +706,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/gradle/libs.versions.toml b/gradle/libs.versions.toml index 8441472..69f21ea 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -42,6 +42,18 @@ annotation = "1.+" junit = "4.+" androidxJunit = "1.+" espressoCore = "3.+" +# UiAutomator. FLOATING, and the argument for it is the one the guard already makes: +# androidx.test.uiautomator is inside `floatedGroupPrefixes` ("androidx."), so `2.+` reads +# as "the newest RELEASED 2.x" exactly the way `work = "2.+"` does -- and this library does +# publish alphas above its stable, so without the guard it would be a pin. +# +# Not pinned like ktlint/detekt/robolectric, because it is not that kind of dependency. Those +# are pinned because a new *rule* or a new *runtime* makes untouched files fail -- the tool +# changes its verdict on code nobody edited. UiAutomator has no verdict: it clicks what a +# selector names, and a selector that stops matching is this repo's test to fix, in a diff +# that says so. `2.` and not bare `+` because 3.x does not exist yet and a major is where the +# selector API would be free to change under exactly that assumption. +uiautomator = "2.+" # PINNED, unlike its neighbours. Under semver a 0.x minor is allowed to break, and # this library is load-bearing exactly where breakage is hardest to see: the wrapper # reaches for smartexception.java.Exceptions only when an FFmpeg call FAILS, so a @@ -139,6 +151,11 @@ junit = { group = "junit", name = "junit", version.ref = "junit" } androidx-junit = { group = "androidx.test.ext", name = "junit", version.ref = "androidxJunit" } androidx-espresso-core = { group = "androidx.test.espresso", name = "espresso-core", version.ref = "espressoCore" } +# The only way to touch UI this app does not own. Compose's own matchers stop at this +# process's composition, and the system file picker is a DocumentsUI activity in another +# process -- so a SAF round trip is unreachable without it. +androidx-uiautomator = { group = "androidx.test.uiautomator", name = "uiautomator", version.ref = "uiautomator" } + # Robolectric — an Android runtime for the JVM test source set, so file-lifecycle behaviour # that needs a real Context can be verified without a device. The instrumented suite cannot # run on the development host at all (see CLAUDE.md), so an androidTest-only red test is not diff --git a/tools/local-emulator/run-e2e.sh b/tools/local-emulator/run-e2e.sh index ed34159..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 # @@ -320,9 +322,20 @@ 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 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 @@ -564,13 +577,25 @@ 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; 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: 2 failures, both Media3EngineTest, on c2.goldfish.h264.decoder. - A third is new -- 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 @@ -596,8 +621,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"