diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 7a18e29..6e86689 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -132,22 +132,12 @@ android { } /* - * Which build type the instrumentation tests run against — only when asked for. - * - * Setting this unconditionally also makes `benchmark` the variant that *unit* tests run against, - * and `:app:testDebugUnitTest` — the task the gate and CI are built on — stops existing. That is - * measured rather than guessed: with it set, the only unit-test task left was - * `testBenchmarkUnitTest`. - * - * ./gradlew -PbenchmarkBuild :benchmark:connectedDebugAndroidTest - * - * `hasProperty` rather than `providers.gradleProperty`: the latter is a Provider, and inside this - * block it read as absent even when the flag was passed — which showed up as the benchmark - * installing the *debug* APK and failing on it. + * No `testBuildType` here on purpose. It reads like "which build the benchmarks run against", but + * in :app it only moves the variant that this module's own *unit* tests use — with it set, the only + * unit-test task left was `testBenchmarkUnitTest` and `:app:testDebugUnitTest`, the task the gate + * and CI are built on, stopped existing. The switch that matters lives in :benchmark, where + * `targetVariant = "benchmark"` makes the run install `app-benchmark.apk` instead of `app-debug.apk`. */ - if (project.hasProperty("benchmarkBuild")) { - testBuildType = "benchmark" - } buildFeatures { compose = true diff --git a/benchmark/build.gradle.kts b/benchmark/build.gradle.kts index 65d6e9b..6d4e7dc 100644 --- a/benchmark/build.gradle.kts +++ b/benchmark/build.gradle.kts @@ -23,6 +23,28 @@ android { } targetProjectPath = ":app" + + /* + * Which variant of :app gets installed and instrumented. + * + * AGP 9 has no `testBuildType`/`targetVariant` on this plugin — the extension exposes only + * `targetProjectPath` — and the target is matched *by name* against this module's own build types. + * With just the default `debug` that meant every run installed `app-debug.apk`, whatever else the + * build said: the numbers would have been debug numbers. Declaring a `benchmark` build type is what + * makes `:benchmark:connectedBenchmarkAndroidTest` instrument `:app`'s benchmark build. + * + * Setting `testBuildType` in :app instead looks equivalent and is not: there it only moves the + * variant that :app's own *unit* tests run against, and `:app:testDebugUnitTest` stops existing. + */ + buildTypes { + create("benchmark") { + // A declared build type starts unsigned, and an unsigned test APK fails to install with + // INSTALL_PARSE_FAILED_NO_CERTIFICATES. The default type gets the debug key for free; this + // borrows the same signing config so the test APK can be installed at all. + initWith(getByName("debug")) + } + } + // The benchmark itself is the thing being instrumented: it starts the app under test, so it must // not be run as a normal instrumentation target. experimentalProperties["android.experimental.self-instrumenting"] = true diff --git a/benchmark/src/main/AndroidManifest.xml b/benchmark/src/main/AndroidManifest.xml index 41de1b1..50fee28 100644 --- a/benchmark/src/main/AndroidManifest.xml +++ b/benchmark/src/main/AndroidManifest.xml @@ -16,6 +16,13 @@ + diff --git a/benchmark/src/main/java/de/softwareapp_hb/filemanager/benchmark/BrowserBenchmark.kt b/benchmark/src/main/java/de/softwareapp_hb/filemanager/benchmark/BrowserBenchmark.kt index 59d9917..24bdfa3 100644 --- a/benchmark/src/main/java/de/softwareapp_hb/filemanager/benchmark/BrowserBenchmark.kt +++ b/benchmark/src/main/java/de/softwareapp_hb/filemanager/benchmark/BrowserBenchmark.kt @@ -49,11 +49,41 @@ class BrowserBenchmark { */ private val app = "de.softwareapp_hb.filemanager" - // No setup by shell command here on purpose. UiAutomation's executeShellCommand runs with *this - // app's* uid, not the shell user's, so it cannot create files in /sdcard and cannot grant itself - // an app-op — the first attempt failed exactly that way, with a RuntimeException from - // UiAutomationConnection and no fixture on disk. `tools/benchmark-fixture.sh` prepares the device - // instead: it runs through adb, so it has the shell's rights. + // Files still have to be created through adb — `tools/benchmark-fixture.sh` — because + // UiAutomation's executeShellCommand runs with *this app's* uid, not the shell user's, and cannot + // write to /sdcard. The storage *grant* is a different matter: it is walked through the app's own + // "Grant full access" button, because `appops set` from the test does not take effect — see + // [grantStorageAccess]. + + /** + * Gives the app all-files access the way a user would, when it came up without it. + * + * This is needed on every run because macrobenchmark uninstalls the app before measuring it, and an + * uninstall clears app-ops: the grant that `tools/benchmark-fixture.sh` applies beforehand is gone + * by the time the app starts, which is why the app legitimately shows its "choose a folder" + * fallback and the volume row the tests navigate to is not on the screen. + * + * Trying to grant it directly — `UiAutomation.executeShellCommand("appops set …")` under + * `adoptShellPermissionIdentity` — does not work: the command fails and prints its complaint to + * stderr, which `executeShellCommand` does not return, so the test continues as if it had been + * granted and fails later looking for the row. Measured; `appops get` still read `No operations`. + * + * The flow below therefore taps what a user taps: the app's button, then whatever consent the + * platform puts in front of the all-files page. MIUI adds its own — "… erlauben, auf Fotos und + * Medien … zuzugreifen?" — whose allow button is the dialog's *first* button; other builds go + * straight to the toggle page. Both end with the toggle switched on and the app in front again. + */ + private fun grantStorageAccess(device: UiDevice) { + val button = device.wait(Until.findObject(By.text(GRANT_ACTION)), SHORT_MS) ?: return + button.click() + device.wait(Until.findObject(By.clazz("android.widget.Button")), SHORT_MS)?.click() + val toggle = device.wait(Until.findObject(By.clazz("android.widget.Switch")), SHORT_MS) + if (toggle != null) { + if (!toggle.isChecked) toggle.click() + device.pressBack() + } + device.waitForIdle() + } @Test fun coldStart() = benchmark.measureRepeated( @@ -112,6 +142,13 @@ class BrowserBenchmark { openFixture(device) device.findObject(By.text(Fixture.NAME))!!.click() device.wait(Until.hasObject(By.text(Fixture.FIRST_FILE)), TIMEOUT_MS) + // The measured block clicks this, so it has to exist before the clock starts: right after a + // thousand rows render, the app bar is a frame behind the list and an immediate lookup + // comes back null. + assertNotNull( + "the app bar's search action is missing; see browser_search_hint in strings.xml", + device.wait(Until.findObject(By.desc(SEARCH_ACTION)), TIMEOUT_MS), + ) }, ) { device.findObject(By.desc(SEARCH_ACTION))!!.click() @@ -124,17 +161,18 @@ class BrowserBenchmark { // ------------------------------------------------------------------ setup /** - * The device is expected to be prepared already: `tools/benchmark-fixture.sh` creates the fixture - * and grants the app its storage access. This only navigates to it, and fails with that - * instruction if the row is not where it should be. + * The device is expected to be prepared: `tools/benchmark-fixture.sh` creates the fixture folder. + * This grants storage access if the reinstall took it away, then navigates, and fails with the + * instruction that is missing if something is not where it should be. */ private fun MacrobenchmarkScope.openFixture(device: UiDevice) { pressHome() startActivityAndWait() + grantStorageAccess(device) val volume = device.wait(Until.findObject(By.text("Internal storage")), TIMEOUT_MS) assertNotNull( - "the volume row is not on screen. Is the device in English, and did the storage grant " + - "land? The benchmark grants it with `appops set` before navigating.", + "the volume row is not on screen, so the storage grant did not land. Is the device in " + + "English? The app has to come up on its roots screen for this benchmark to navigate.", volume, ) volume!!.click() @@ -148,8 +186,14 @@ class BrowserBenchmark { companion object { private const val TIMEOUT_MS = 10_000L + /** How long to wait for something that should already be on screen. */ + private const val SHORT_MS = 2_000L + /** The app bar's own description for the search action; see strings.xml. */ private const val SEARCH_ACTION = "Search this folder" + + /** The fallback screen's button, shown only when the app has no all-files access. */ + private const val GRANT_ACTION = "Grant full access" } } diff --git a/docs/CODE_MAP.md b/docs/CODE_MAP.md index 8137d8a..73d33f1 100644 --- a/docs/CODE_MAP.md +++ b/docs/CODE_MAP.md @@ -2,7 +2,7 @@ Generated from the source by `tools/make-code-map.py` - do not edit by hand. Regenerate it after a change (`python3 tools/make-code-map.py`); `--check` exits non-zero when this file is stale. -77 source files, 12660 lines - 33 test files, 386 tests. The behaviour index at the end lists every test name, and in this project a test name is a sentence about what the app does, so it doubles as the specification. +77 source files, 12704 lines - 33 test files, 386 tests. The behaviour index at the end lists every test name, and in this project a test name is a sentence about what the app does, so it doubles as the specification. Start at `AGENTS.md`. What is done, and why: `docs/ROADMAP.md` and `CHANGELOG.md`. What was verified by hand rather than by a test: `docs/EDGE_CASE_TESTING.md`. @@ -170,7 +170,7 @@ The macrobenchmark test APK: four measurements over a fixture folder, run on a d | file | lines | what it holds | tests | |---|---|---|---| -| `BrowserBenchmark.kt` | 168 | The four numbers docs/PERFORMANCE.md promises, measured on a benchmark build of the app. BrowserBenchmark, Fixture | - | +| `BrowserBenchmark.kt` | 212 | The four numbers docs/PERFORMANCE.md promises, measured on a benchmark build of the app. BrowserBenchmark, Fixture | - | ## Behaviour index diff --git a/docs/PERFORMANCE.md b/docs/PERFORMANCE.md index b39a98b..d80959f 100644 --- a/docs/PERFORMANCE.md +++ b/docs/PERFORMANCE.md @@ -18,8 +18,10 @@ Until this file existed, the app had **no performance number of any kind**. The first is `./tools/measure-startup.sh 5` (`am start -W`, one warm-up start discarded because the first launch after an install pays for dexopt). The second is `BrowserBenchmark#coldStart` in the `:benchmark` module: `StartupTimingMetric`, five iterations, `CompilationMode.None()`, measured as time to initial -display — min 499 ms, median 532 ms, max 805 ms. Measured twice: 615 ms before the storage grant was in -place (the app was showing the permission screen) and 532 ms after, which is the figure above. +display — min 499 ms, median 532 ms, max 805 ms. Measured three times: 615 ms before the storage grant was +in place (the app was showing the permission screen), 532 ms after it, and 560 ms (min 539, max 698) on a +later run that also exercised the full benchmark suite. The three runs agree within a phone's patience; +the 532 ms figure above is the middle one. Two caveats belong to that number rather than to a footnote. The tool itself warns *"Unable to clear Runtime Image, subsequent launches/iterations may exhibit faster startup than production due to @@ -67,63 +69,74 @@ install**, and reinstalling the release APK puts it back. | `scrollAFolderOfAThousandFiles` | `FrameTimingMetric` | jank while scrolling that folder | | `searchInsideAFolderOfAThousandFiles` | `FrameTimingMetric` | jank while typing into search | -They share a fixture — `/sdcard/AABenchmark`, a thousand empty files, created once per run and deleted -in `@AfterClass` — and the storage grant is applied with `appops`, so an unattended run is possible. -`CompilationMode.None()` is deliberate: no profile is applied, which is the honest cold-install -baseline. `CompilationMode.Partial()` plus a baseline profile is the follow-up, and needs the same -module. +They share a fixture — `/sdcard/AABenchmark`, a thousand empty files, created by `tools/benchmark-fixture.sh` +and removed with `--remove`. `CompilationMode.None()` is deliberate: no profile is applied, which is the +honest cold-install baseline. `CompilationMode.Partial()` plus a baseline profile is the follow-up, and +needs the same module. -**Two of these have produced a number** (`coldStart`, above); the three interaction ones have not, and -the reason is a device policy rather than the code. Getting this far took five fixes, each written next -to what it constrains: the app under test needs the app's own package id and release key (a new id cannot -be installed here); the test APK needs package visibility for it; the fixture has to be created through -adb, because `UiAutomation.executeShellCommand` runs with the *test app's* uid and cannot write `/sdcard`; -`testBuildType` has to sit behind `-PbenchmarkBuild`, or `:app:testDebugUnitTest` stops existing; and -`hasProperty` is needed to read that flag, because the Provider form read as absent and the runner then -installed the *debug* APK and failed on it. +### What the module has actually measured -The workflow: +| benchmark | outcome on the Xiaomi phone (Android 12, MIUI) | +|---|---| +| `coldStart` | **passes** — 560.6 ms P50, min 539, max 698 over five iterations. A second measurement of the 532 ms above, and the only number this toolchain reports here | +| `scrollAFolderOfAThousandFiles` | passes, but reports **only `frameCount`** (46–100 frames per iteration): this device hands macrobenchmark no `frameDurationCpuMs`/`frameOverrunMs` at all | +| `openAFolderOfAThousandFiles` | `INSTALL_FAILED_USER_RESTRICTED: Install canceled by user` — the rule reinstalls the app mid-run, and MIUI wants a tap for that | +| `searchInsideAFolderOfAThousandFiles` | the app bar's search action is not in the accessibility tree within ten seconds of the folder opening, though it is there when the same taps are made by hand | + +So the interaction numbers are not written down here because **the device will not produce them**, not +because the code cannot ask. Two separate device behaviours are in the way, and both are outside the app: +a reinstall that needs a human, and a platform that reports frame counts but not frame durations. + +### The four switches that make an unattended run possible at all + +1. **The app under test is the app itself** — same package id, release key — because a new package id + cannot be installed on this phone (`INSTALL_FAILED_USER_RESTRICTED`, and an APK installed by tapping + ended up visible to `pm list packages` but invisible to `PackageManager`). The cost is recorded in + `:app`'s build file: **this build replaces a release install**; reinstalling the release APK puts it + back. +2. **The test module declares a `benchmark` build type.** AGP 9's `com.android.test` extension has no + `testBuildType` and no `targetVariant` — only `targetProjectPath` — and the target variant is matched + *by name* against the test module's own build types. Without that type every run installed + `app-debug.apk`, so the numbers would have been debug numbers. The type also has to + `initWith(getByName("debug"))`: a declared type starts unsigned, and an unsigned test APK fails with + `INSTALL_PARSE_FAILED_NO_CERTIFICATES`. +3. **The storage grant is walked through the UI, not set with `appops`.** The rule uninstalls and + reinstalls the app, and an uninstall clears app-ops, so a grant applied by the fixture script is gone + by the time the app starts. `appops set` from inside the test does not work either — even under + `adoptShellPermissionIdentity`, the command fails to stderr (which `executeShellCommand` does not + return) and `appops get` still reads `No operations`. What does work is tapping what a user taps: + the app's "Grant full access" button, then the platform's consent, then leaving the all-files page. + `grantStorageAccess` in `BrowserBenchmark` does exactly that, in `setupBlock`, unmeasured. +4. **The fixture is created through adb**, because `UiAutomation.executeShellCommand` runs with the *test + app's* uid and cannot write `/sdcard`. + +### The workflow ```bash -./tools/benchmark-fixture.sh # the fixture, and the storage grant the benchmark build lacks -./gradlew -PbenchmarkBuild :benchmark:connectedDebugAndroidTest +./tools/benchmark-fixture.sh # /sdcard/AABenchmark, 1000 empty files +./gradlew :benchmark:connectedBenchmarkAndroidTest ``` -**The Gradle runner is required, not preferred.** Macrobenchmark's rule uninstalls the app at the start -of a run and reinstalls it before measuring. That alone is survivable; what is not is the side effect: -**an uninstall clears the app's app-ops**, so the `MANAGE_EXTERNAL_STORAGE` grant from -`tools/benchmark-fixture.sh` is gone by the time the test navigates. The app then correctly shows its -"choose a folder" fallback, the volume row is not on the screen, and the assertion says so. Checked -directly — `appops get … MANAGE_EXTERNAL_STORAGE` reads `No operations` after a run that passed through -the reinstall. - -There is no way around that from inside the test: granting an app-op needs the shell's uid, and -`UiAutomation.executeShellCommand` runs with the *test app's* uid. So on a device where this app needs -that grant — which is every device, for the browser screen — **the three interaction benchmarks cannot run -as macrobenchmark tests here**, and the cold-start number above stands alone. - -Two paths would give the numbers, and neither is a small change: - -- **A plain instrumented test instead of a macrobenchmark one** (no rule, so no reinstall): install the - benchmark build once, grant, then navigate with UiAutomator and read frame durations from - `FrameMetrics`. It measures the release-like build, but it is our own measurement code rather than a - maintained library, and `FrameTimingMetric` exists precisely so nobody has to write that. -- **An app that does not need the grant for the flow being measured** — for instance a benchmark - build that browses its own external files directory, which needs no permission. That measures a - different screen than the one users open, so it answers a narrower question than "how fast does - opening a 1 000-file folder feel". +`coldStart` alone can be run with `-Pandroid.testInstrumentationRunnerArguments.class=…BrowserBenchmark#coldStart`. +The results land in `benchmark/build/outputs/androidTest-results/connected/benchmark/`, and — worth +knowing, because it cost an hour — the per-test JSON that holds the numbers is written to +`/sdcard/Android/media//additional_test_output/` and **deleted together with the test app when +the run ends**, often before Gradle has copied it. Copying it in a loop while the run is in progress is +what recovered the numbers above. ## Not measured yet, and why | number | how it would be taken | why it is not here | |---|---|---| -| Folder open, 1 000 / 5 000 / 20 000 files | macrobenchmark `StartupTimingMetric` over a fixture volume, or `TraceSectionMetric` around the listing | needs a `:benchmark` module and a profileable build installed under the **release package id**; on this MIUI phone the first install of a new package id waits for a confirmation dialog, so it cannot run unattended | -| Thumbnail paint while scrolling a photo grid | `FrameTimingMetric` | same | -| Keystroke → search result | `TraceSectionMetric` around the query and the index read | same | +| Folder open, 1 000 / 5 000 / 20 000 files | `FrameTimingMetric` around the folder tap (`openAFolderOfAThousandFiles` exists) | the rule reinstalls the app mid-run, and MIUI answers that install with **"Install canceled by user"** unless someone is there to tap | +| Thumbnail paint while scrolling a photo grid | `FrameTimingMetric` | same; and the scroll variant that *did* run reported only `frameCount`, because this device gives macrobenchmark no frame durations | +| Keystroke → search result | `FrameTimingMetric` around typing into search (`searchInsideAFolderOfAThousandFiles` exists) | the app bar's search action does not appear in the accessibility tree within ten seconds of the folder opening in the automated run, although the same taps by hand find it immediately | -The module does not exist yet, deliberately: half-configured build wiring that nobody can run is worse -than an honest gap. It is the next measurement step, and it belongs with the baseline-profile work, -which needs the same module. +The module exists and three of its four tests pass; what is missing is a device that lets macrobenchmark +install unattended *and* reports frame timing. A plain emulator image (or any non-MIUI phone) with the +system language set to English would answer all three — the fixture is a folder of empty files, so nothing +device-specific is being measured. Until then this file says what was measured and what was not, and the +cold-start number stands alone. ## What is measured instead, meanwhile