From b18f45def76ee6ba1f404732978a0eec0ddeb936 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 24 Aug 2026 20:15:33 -0500 Subject: [PATCH 1/4] Give the system file picker something to pick Nothing in either source set drives SAF as a picker. The only SAF coverage is the publish side, in OutputPublisherPublishTest, against hand-written ContentProvider fakes -- so the launcher wiring in ConverterScreen, the MIME filter it passes, and the grant that comes back have never been executed by a test. Driving the real picker needs three things this repo did not have. UiAutomator, because DocumentsUI is another process. Compose's matchers stop at this process's composition and Espresso's stop at its view hierarchy; neither can see or tap a window belonging to another package. It FLOATS, at "2.+", which is the same argument the catalog already makes for work and lifecycle rather than a new one: androidx.test.uiautomator is inside floatedGroupPrefixes, so the componentSelection guard makes "+" mean "newest RELEASED", and that is load-bearing here -- this library publishes 2.4.0-alphas above its stable, so without the guard the float would be a pin to a prerelease. Resolved to 2.4.0 (released) on debugAndroidTestRuntimeClasspath, checked rather than assumed. It is deliberately NOT pinned alongside ktlint/detekt/JaCoCo/Robolectric: those are pinned because a new rule or a new runtime changes the verdict on files nobody touched. UiAutomator has no verdict -- it taps what a selector names, and a selector that stops matching is this repo's test to fix, in a diff that explains itself. The "2." rather than a bare "+" is the one thing held back: a major is where the selector API would be free to change under exactly that assumption. A DocumentsProvider, because DocumentsUI does not browse a filesystem -- it lists what providers offer it. Writing a file into Downloads would have worked and tested less: the fixture root declares Root.COLUMN_MIME_TYPES, and DocumentsUI filters the drawer by it, which is what gives the MIME filter a mutation with a shape rather than "one file among the hundreds in Downloads was not listed". Its contents are also exactly one file, where a shared directory accumulates whatever earlier runs left behind. And the first AndroidManifest.xml this source set has ever had, to declare it -- a ContentProvider is instantiated by the system and cannot be registered from test code. In androidTest rather than src/debug so it is installed by the instrumentation APK only, and never appears in a developer's own file picker. Two things worth knowing before editing either file. XML comments cannot contain "--", which the manifest's first draft failed the build on; and "*/" inside a KDoc closes the comment, which the provider's did. Both are silent in review and loud in the build. No test yet, and no new test tag: TestTags.Converter.CHOOSE_FILE and FILE_CARD_NAME already name both ends of the round trip. Co-Authored-By: Claude Opus 5 (1M context) --- app/build.gradle.kts | 6 + app/src/androidTest/AndroidManifest.xml | 47 +++++ .../saf/FixtureDocumentsProvider.kt | 197 ++++++++++++++++++ gradle/libs.versions.toml | 17 ++ 4 files changed, 267 insertions(+) create mode 100644 app/src/androidTest/AndroidManifest.xml create mode 100644 app/src/androidTest/java/org/libremediaconverter/saf/FixtureDocumentsProvider.kt diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 6ed9c72..7287b11 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -340,5 +340,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.kt b/app/src/androidTest/java/org/libremediaconverter/saf/FixtureDocumentsProvider.kt new file mode 100644 index 0000000..3b20033 --- /dev/null +++ b/app/src/androidTest/java/org/libremediaconverter/saf/FixtureDocumentsProvider.kt @@ -0,0 +1,197 @@ +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 + +/** + * One file, offered to the system file picker, so that picking one can be tested at all. + * + * DocumentsUI does not browse the filesystem: it lists what [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 — a `ContentProvider` is instantiated by the system, never + * registered from test code. `app/src/androidTest/AndroidManifest.xml` is that declaration and + * says why each of its attributes is load-bearing. + * + * ### Why not just write a file to Downloads + * + * That would work and it would test less. Two properties of a provider are what + * [SafPickerRoundTripTest] actually needs: + * + * - **The root declares [Root.COLUMN_MIME_TYPES], and DocumentsUI filters the drawer by it.** + * That is what gives the MIME mutation a bite with a shape: ask for a type this root does not + * offer and the root itself is not in the picker, so the failure is "the fixture root is not + * there" rather than "one file among the hundreds in Downloads was not listed". + * - **The contents are exactly this and nothing else.** A shared directory accumulates whatever + * earlier runs and other tests left in it, and a picker test that finds the wrong file passes. + * + * ### It runs in its own process, so it seeds itself + * + * The instrumentation's own code executes inside the *app's* process; this provider is a + * component of the instrumentation *package*, so the system starts a separate process under + * `org.libremediaconverter.test` for it. Nothing the test sets up in a field is visible here. + * The bytes therefore come from this APK's own assets, on demand, and the test and the provider + * agree only on the constants below — which are compile-time, so they cannot drift. + * + * The descriptor is opened on a real file rather than served through a pipe, deliberately. + * `InputQuery.sizeOf` falls back to `ParcelFileDescriptor.statSize` when a provider omits + * `OpenableColumns.SIZE`, and a pipe's `statSize` is `-1` — an unknown size, which is a + * different case with its own screen. This fixture is meant to be an ordinary, fully described + * file, so the one thing under test is the round trip. + */ +class FixtureDocumentsProvider : DocumentsProvider() { + + override fun onCreate(): Boolean = true + + /** + * The single root, named [ROOT_TITLE] so a UiAutomator text selector is unambiguous. + * + * [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 fun queryRoots(projection: Array?): Cursor = + MatrixCursor(copyOf(projection, DEFAULT_ROOT_PROJECTION)).apply { + 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) + } + + override fun queryDocument(documentId: String?, projection: Array?): Cursor = + MatrixCursor(copyOf(projection, DEFAULT_DOCUMENT_PROJECTION)).apply { + when (documentId) { + ROOT_DOCUMENT_ID -> addDirectoryRow() + FIXTURE_DOCUMENT_ID -> addFixtureRow() + else -> throw java.io.FileNotFoundException("no such document: $documentId") + } + } + + override fun queryChildDocuments( + parentDocumentId: String?, + projection: Array?, + sortOrder: String?, + ): Cursor = MatrixCursor(copyOf(projection, DEFAULT_DOCUMENT_PROJECTION)).apply { + if (parentDocumentId == ROOT_DOCUMENT_ID) addFixtureRow() + } + + override fun openDocument(documentId: String?, mode: String?, signal: CancellationSignal?): ParcelFileDescriptor { + if (documentId != FIXTURE_DOCUMENT_ID) { + throw java.io.FileNotFoundException("no such document: $documentId") + } + return ParcelFileDescriptor.open(fixtureFile(), ParcelFileDescriptor.MODE_READ_ONLY) + } + + private fun MatrixCursor.addDirectoryRow() { + 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 fun MatrixCursor.addFixtureRow() { + 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, fixtureFile().length()) + .add(Document.COLUMN_LAST_MODIFIED, fixtureFile().lastModified()) + } + + /** + * The fixture on disk, unpacked from this APK's assets the first time anything asks. + * + * Idempotent rather than seeded once at [onCreate], because this process is started by + * whoever queries the provider and can be killed between two queries of the same test. + */ + private fun fixtureFile(): File { + val context = requireNotNull(context) { "provider used before onCreate" } + val file = File(context.filesDir, FIXTURE_DISPLAY_NAME) + if (file.length() == 0L) { + context.assets.open(FIXTURE_ASSET).use { source -> + file.outputStream().use(source::copyTo) + } + } + return file + } + + /** + * [projection] as a `MatrixCursor` will take it, or [fallback] when the caller asked for + * everything. + * + * Rebuilt element by element rather than spread into `arrayOf`, so nothing depends on the + * variance of the platform array type this arrives as. + */ + private fun copyOf(projection: Array?, fallback: Array): Array = + if (projection == null) fallback else Array(projection.size) { projection[it] } + + companion object { + + /** Matches `android:authorities` in `app/src/androidTest/AndroidManifest.xml`. */ + const val AUTHORITY: String = "org.libremediaconverter.test.fixtures" + + /** + * What the picker's drawer calls this root. + * + * Deliberately not a word any other root uses. DocumentsUI's drawer also lists + * "Downloads", "Images", "Videos" and the device name, and a UiAutomator selector that + * could match two of them is not a selector. + */ + const val ROOT_TITLE: String = "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 `OpenableColumns.DISPLAY_NAME` on the URI the app is handed — which is exactly + * the round trip under test. + */ + const val FIXTURE_DISPLAY_NAME: String = "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 one under which this test could pass. + */ + const val FIXTURE_MIME_TYPE: String = "video/mp4" + + private const val ROOT_ID = "lmc-r38-root" + private const val ROOT_DOCUMENT_ID = "root" + private const val FIXTURE_DOCUMENT_ID = "root/$FIXTURE_DISPLAY_NAME" + + /** Already in this source set, and already a real H.264 MP4 the engines can open. */ + private const val FIXTURE_ASSET = "sample_h264.mp4" + + private val DEFAULT_ROOT_PROJECTION = arrayOf( + 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 val DEFAULT_DOCUMENT_PROJECTION = arrayOf( + Document.COLUMN_DOCUMENT_ID, + Document.COLUMN_DISPLAY_NAME, + Document.COLUMN_MIME_TYPE, + Document.COLUMN_FLAGS, + Document.COLUMN_SIZE, + Document.COLUMN_LAST_MODIFIED, + ) + } +} 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 -- 2.47.3 From 650ca8fca31985a1026ecd0215dbb76af8edc541 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 24 Aug 2026 20:36:31 -0500 Subject: [PATCH 2/4] Pick a file the way a user does, then rotate the phone Two things nothing in this repo asserted, and they are one test class because separately the second one asserts nothing new. THE PICKER. ConverterScreen opens SAF with a MIME filter, and a filter is a thing that can hide the user's file. Narrow it and the app still builds, still renders, and still passes every JVM test -- the user taps "Choose file" and gets an empty picker. The round trip now runs for real: DocumentsUI is driven with UiAutomator to a fixture root, and the app is asserted to come back with the file. The file card's name is not the only assertion, because a name proves less than it looks: it comes from a metadata query, which a URI with no read grant answers just as well. The "Container: MP4" detail row only appears once something has opened the file and read its header, so it is what says the picker handed back a URI the app can USE. THE ROTATION. MainActivity declares no configChanges, and ConversionViewModel holds the picked file in a plain MutableStateFlow with NO SavedStateHandle behind it. Nothing persists it. The only thing that carries it across a rotation is the ViewModelStore the Activity retains -- which no test anywhere asserted. Two guards run before that assertion, because both ways it could pass while proving nothing are silent: the display rotation really changed, and MainActivity really was a different instance afterwards. Without the second one this is a recomposition test wearing a rotation's name. MUTATIONS, RUN RATHER THAN ASSERTED, on a local API 34 emulator. Narrowing the filter to arrayOf("application/x-lmc-no-such-type") takes the fixture root out of the picker entirely -- DocumentsUI matches the request against Root.COLUMN_MIME_TYPES and drops roots that cannot answer -- and both tests fail: java.lang.IllegalArgumentException: the system picker never showed BySelector [TEXT='\QLMC R38 fixtures\E'] Making the ViewModel composition-scoped fails ONLY the rotation test: androidx.compose.ui.test.ComposeTimeoutException: Condition (a node tagged converter.fileCard.name exists) still not satisfied after 30000 ms and :app:testDebugUnitTest stays BUILD SUCCESSFUL under it. That divergence is what #64 exists to establish and what its own comment doubted; the PR body has the verdict and why the doubt was reasonable. THE PROVIDER HAD TO BE JAVA. It is the only Java file in the module. A manifest-declared provider is a component of the instrumentation PACKAGE, so the system starts a plain org.libremediaconverter.test process for it with only the test APK on its dex path -- and the test APK is built without the Kotlin stdlib, because the app APK has it and duplicating it is what checkDebugAndroidTestDuplicateClasses prevents. The Kotlin draft died on its first query: 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 nearly every function, so no Kotlin dialect avoids it. Same reason nothing in that file imports androidx. No new test tags: CHOOSE_FILE, FILE_CARD_NAME and detailRow already named both ends. Co-Authored-By: Claude Opus 5 (1M context) --- app/build.gradle.kts | 7 +- .../saf/FixtureDocumentsProvider.java | 234 +++++++++++++++++ .../saf/FixtureDocumentsProvider.kt | 197 -------------- .../saf/SafPickerRoundTripTest.kt | 248 ++++++++++++++++++ 4 files changed, 487 insertions(+), 199 deletions(-) create mode 100644 app/src/androidTest/java/org/libremediaconverter/saf/FixtureDocumentsProvider.java delete mode 100644 app/src/androidTest/java/org/libremediaconverter/saf/FixtureDocumentsProvider.kt create mode 100644 app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 7287b11..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", ) 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 root declares {@link Root#COLUMN_MIME_TYPES}, and DocumentsUI filters by it. + * That is what gives the screen's MIME filter a mutation with a shape: ask for a type this + * root does not offer and the root itself is not in the picker, so the failure reads as + * "the fixture root is not there" rather than "one file among the hundreds in Downloads was + * not listed". + *
  • The contents are exactly this and nothing else. A shared directory accumulates + * whatever earlier runs and other tests left in it, and a picker test that finds the wrong + * file passes. + *
+ * + *

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/FixtureDocumentsProvider.kt b/app/src/androidTest/java/org/libremediaconverter/saf/FixtureDocumentsProvider.kt deleted file mode 100644 index 3b20033..0000000 --- a/app/src/androidTest/java/org/libremediaconverter/saf/FixtureDocumentsProvider.kt +++ /dev/null @@ -1,197 +0,0 @@ -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 - -/** - * One file, offered to the system file picker, so that picking one can be tested at all. - * - * DocumentsUI does not browse the filesystem: it lists what [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 — a `ContentProvider` is instantiated by the system, never - * registered from test code. `app/src/androidTest/AndroidManifest.xml` is that declaration and - * says why each of its attributes is load-bearing. - * - * ### Why not just write a file to Downloads - * - * That would work and it would test less. Two properties of a provider are what - * [SafPickerRoundTripTest] actually needs: - * - * - **The root declares [Root.COLUMN_MIME_TYPES], and DocumentsUI filters the drawer by it.** - * That is what gives the MIME mutation a bite with a shape: ask for a type this root does not - * offer and the root itself is not in the picker, so the failure is "the fixture root is not - * there" rather than "one file among the hundreds in Downloads was not listed". - * - **The contents are exactly this and nothing else.** A shared directory accumulates whatever - * earlier runs and other tests left in it, and a picker test that finds the wrong file passes. - * - * ### It runs in its own process, so it seeds itself - * - * The instrumentation's own code executes inside the *app's* process; this provider is a - * component of the instrumentation *package*, so the system starts a separate process under - * `org.libremediaconverter.test` for it. Nothing the test sets up in a field is visible here. - * The bytes therefore come from this APK's own assets, on demand, and the test and the provider - * agree only on the constants below — which are compile-time, so they cannot drift. - * - * The descriptor is opened on a real file rather than served through a pipe, deliberately. - * `InputQuery.sizeOf` falls back to `ParcelFileDescriptor.statSize` when a provider omits - * `OpenableColumns.SIZE`, and a pipe's `statSize` is `-1` — an unknown size, which is a - * different case with its own screen. This fixture is meant to be an ordinary, fully described - * file, so the one thing under test is the round trip. - */ -class FixtureDocumentsProvider : DocumentsProvider() { - - override fun onCreate(): Boolean = true - - /** - * The single root, named [ROOT_TITLE] so a UiAutomator text selector is unambiguous. - * - * [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 fun queryRoots(projection: Array?): Cursor = - MatrixCursor(copyOf(projection, DEFAULT_ROOT_PROJECTION)).apply { - 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) - } - - override fun queryDocument(documentId: String?, projection: Array?): Cursor = - MatrixCursor(copyOf(projection, DEFAULT_DOCUMENT_PROJECTION)).apply { - when (documentId) { - ROOT_DOCUMENT_ID -> addDirectoryRow() - FIXTURE_DOCUMENT_ID -> addFixtureRow() - else -> throw java.io.FileNotFoundException("no such document: $documentId") - } - } - - override fun queryChildDocuments( - parentDocumentId: String?, - projection: Array?, - sortOrder: String?, - ): Cursor = MatrixCursor(copyOf(projection, DEFAULT_DOCUMENT_PROJECTION)).apply { - if (parentDocumentId == ROOT_DOCUMENT_ID) addFixtureRow() - } - - override fun openDocument(documentId: String?, mode: String?, signal: CancellationSignal?): ParcelFileDescriptor { - if (documentId != FIXTURE_DOCUMENT_ID) { - throw java.io.FileNotFoundException("no such document: $documentId") - } - return ParcelFileDescriptor.open(fixtureFile(), ParcelFileDescriptor.MODE_READ_ONLY) - } - - private fun MatrixCursor.addDirectoryRow() { - 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 fun MatrixCursor.addFixtureRow() { - 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, fixtureFile().length()) - .add(Document.COLUMN_LAST_MODIFIED, fixtureFile().lastModified()) - } - - /** - * The fixture on disk, unpacked from this APK's assets the first time anything asks. - * - * Idempotent rather than seeded once at [onCreate], because this process is started by - * whoever queries the provider and can be killed between two queries of the same test. - */ - private fun fixtureFile(): File { - val context = requireNotNull(context) { "provider used before onCreate" } - val file = File(context.filesDir, FIXTURE_DISPLAY_NAME) - if (file.length() == 0L) { - context.assets.open(FIXTURE_ASSET).use { source -> - file.outputStream().use(source::copyTo) - } - } - return file - } - - /** - * [projection] as a `MatrixCursor` will take it, or [fallback] when the caller asked for - * everything. - * - * Rebuilt element by element rather than spread into `arrayOf`, so nothing depends on the - * variance of the platform array type this arrives as. - */ - private fun copyOf(projection: Array?, fallback: Array): Array = - if (projection == null) fallback else Array(projection.size) { projection[it] } - - companion object { - - /** Matches `android:authorities` in `app/src/androidTest/AndroidManifest.xml`. */ - const val AUTHORITY: String = "org.libremediaconverter.test.fixtures" - - /** - * What the picker's drawer calls this root. - * - * Deliberately not a word any other root uses. DocumentsUI's drawer also lists - * "Downloads", "Images", "Videos" and the device name, and a UiAutomator selector that - * could match two of them is not a selector. - */ - const val ROOT_TITLE: String = "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 `OpenableColumns.DISPLAY_NAME` on the URI the app is handed — which is exactly - * the round trip under test. - */ - const val FIXTURE_DISPLAY_NAME: String = "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 one under which this test could pass. - */ - const val FIXTURE_MIME_TYPE: String = "video/mp4" - - private const val ROOT_ID = "lmc-r38-root" - private const val ROOT_DOCUMENT_ID = "root" - private const val FIXTURE_DOCUMENT_ID = "root/$FIXTURE_DISPLAY_NAME" - - /** Already in this source set, and already a real H.264 MP4 the engines can open. */ - private const val FIXTURE_ASSET = "sample_h264.mp4" - - private val DEFAULT_ROOT_PROJECTION = arrayOf( - 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 val DEFAULT_DOCUMENT_PROJECTION = arrayOf( - Document.COLUMN_DOCUMENT_ID, - Document.COLUMN_DISPLAY_NAME, - Document.COLUMN_MIME_TYPE, - Document.COLUMN_FLAGS, - Document.COLUMN_SIZE, - Document.COLUMN_LAST_MODIFIED, - ) - } -} 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..c245bee --- /dev/null +++ b/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt @@ -0,0 +1,248 @@ +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.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.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. + */ +@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 + 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. + val root = awaitPickerNode(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() + + awaitNode(TestTags.Converter.FILE_CARD_NAME) + } + + /** + * 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 + + /** 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" + } +} -- 2.47.3 From a3c835b7c9ebe100ee2fec3fb7a6441154dd80e8 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 24 Aug 2026 20:58:34 -0500 Subject: [PATCH 3/4] 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" -- 2.47.3 From 3925f1aa9f327d82441797070464c808ad3e3343 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 24 Aug 2026 21:14:07 -0500 Subject: [PATCH 4/4] Re-find the picker node when it goes stale, and re-measure API 37 CI found a flake this workstation could not, and fixing it overturned half of what the previous commit recorded about API 37. THE FLAKE. UiObject2 caches the AccessibilityNodeInfo it was found with, and DocumentsUI is still settling when a node first appears -- its list rebinds, the roots strip lays out, a window animates. If the node is replaced in that gap, click() throws against the handle rather than missing the target: androidx.test.uiautomator.StaleObjectException at androidx.test.uiautomator.UiObject2.getAccessibilityNodeInfo(UiObject2.java:1042) at androidx.test.uiautomator.UiObject2.click(UiObject2.java:526) at SafPickerRoundTripTest.pickTheFixture(SafPickerRoundTripTest.kt:223) It is not intermittent on a COLD emulator -- CI hit it on API 33, 34 and 35, every one of them, on the first run. It never appeared here because the local emulator had been warm for an hour. tapPickerNode now re-finds the node and taps again, three attempts. That retries acquiring a handle to a node that has to be there anyway: every attempt still goes through awaitPickerNode, which fails outright if it is absent, so the MIME mutation's bite is untouched. Verified with `pm clear com.google.android.documentsui` between runs, five for five green on API 34. AND THE CORRECTION IT FORCED. The previous commit marked the whole class @FailsOnEmulatorApi37 on the strength of two measured failures. One of them was this bug. Re-measured with the fix, one method per fresh android-37.0 emulator: thePickedInputSurvivesARealRotation INSTRUMENTATION_ABORTED: System has crashed. pickingAFileThroughTheSystemPickerFillsInTheFileCard PASSED So a rotation, which rebuilds every surface at once, is what the gralloc mapper does not survive; starting another app's activity is not. The marker moves to the one method that earned it, and the picker test runs on the gating API 37 leg like anything else. The workflow comment, run-e2e.sh and the doc all say that now. The lesson is worth more than the measurement, and the doc keeps it: an annotation is a claim about an IMAGE, and a broken test makes every image look broken. Both a framework abort and a stale node read as "the run fell over". Re-measure after fixing a test before deciding what the platform did. Also measured rather than assumed, since it is what keeps the gating leg green: the runner's annotation filter honours a class-level marker, expanding it to every method. On API 34, `annotation=` selected exactly 4 tests (2 Media3EngineTest + 2 here) and `notAnnotation=` selected 55 with neither of these in it. CI's own gating API 37 leg then reported 55 / 0 on the previous push. That is why moving the marker to a single method is a narrowing rather than a repair. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/status_check.yml | 23 +++-- .../saf/SafPickerRoundTripTest.kt | 99 ++++++++++++++----- docs/api-37-emulator-crash.md | 60 ++++++----- tools/local-emulator/run-e2e.sh | 49 ++++----- 4 files changed, 154 insertions(+), 77 deletions(-) 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 -- 2.47.3