From 9fd96d08fd2daa7ec2fd71776ba1588b90689dad Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 7 Sep 2026 15:44:20 -0500 Subject: [PATCH] Synchronise SafPickerRoundTripTest on state, not on timing (#268) Its two picker tests failed on roughly half of gating runs, by two measured mechanisms. Both are removed here rather than re-tuned; the fix is in the test. **A -- the Convert tap was lost in the post-probe relayout.** `ConversionViewModel.onInputPicked` writes `_state` twice: name and size first, then the probe. The second write grows the file card and moves the Convert button. Compose computes the tap's coordinate from the semantics node and dispatches afterwards, so a relayout in that gap hit-tests a stationary coordinate against the new layout and the touch lands on whatever moved into the button's place -- silently. Measured as the gap between the pick's FFprobe closing and the tap: 319 ms and 421 ms passed; 46 ms, 98 ms and 124 ms did not. `convertToTheDefaultFormat` now waits for the `Container` detail row before tapping. That row is composed only under `input.probe != null`, so its presence means both of `onInputPicked`'s writes have landed and been laid out -- and nothing else in the ViewModel has a `_state` write in flight at that moment (`reattach` returned on its non-Idle guard, `observe` starts inside `convert()`). The card cannot change height again before the tap. That is a different claim from waiting longer. **B -- the app was not the focused window when Compose was queried.** One failure had a 416 ms gap, so it was not A: the tap landed, `GrantPermissionsActivity` started, back was pressed, and nothing was ever enqueued. A back press goes to whichever window holds *input* focus, while `Until.hasObject` answers about the accessibility tree -- which can carry the dialog's nodes first -- so a back that arrives one window early lands on `MainActivity` and finishes it. `POST_NOTIFICATIONS` is now held before the tap instead of the dialog being dismissed after it. `RequestPermission.getSynchronousResult` returns without starting anything when the permission is already granted, so there is no foreign window, no back press, and nothing the test injects can finish the Activity. `@Before` asserts the grant rather than assuming it. The class KDoc claimed granting "was tried first and did not take". Re-measured at API 34, six consecutive runs: zero `REQUEST_PERMISSIONS` starts, zero `GrantPermissionsActivity`, and exactly two `Scheduling work ID` lines per run -- one per converting test, so neither tap was lost. Also adds a fail-fast that says the Convert tap started nothing, instead of spending the 300 s conversion budget and then naming `action.saveFile`. It is a diagnostic, explicitly not the synchronisation. Not fixed in production. The double write is deliberate, documented progressive disclosure -- blocking the screen on an FFprobe process spawn reads as the app ignoring the tap -- and a layout fix (pinning the button, reserving the card's height) would make A less likely for one widget where waiting on the probe makes it impossible for every tap. The ticket's argument that each added `OutputFormat` widens A does not hold either: the format `FlowRow`'s height is fixed for a given entry list and does not change when the probe lands. What displaces Convert is the card growing, independent of chip count. Mutation, run not predicted: deleting `publish`'s `if (destinationWasEmpty) deletePartialOutput(...)` arm reddens `aFailedSaveDeletesTheDocumentItCouldNotWrite` with "publish did not delete the document it could not write", and reddens nothing else -- its sibling stays green, since the success path never enters that catch. Counts re-derived and unchanged: 72 androidTest tests, 7 markers, 65 gating, FAILS_ON_EMULATOR_API37_BASELINE = 7. Both tests keep @FailsOnEmulatorApi37. `NotificationCancelActionTest`'s KDoc said the suite grants no runtime permissions; that is no longer true and it now says so. Co-Authored-By: Claude Opus 5 (1M context) --- .../saf/SafPickerRoundTripTest.kt | 175 ++++++++++++++---- .../work/NotificationCancelActionTest.kt | 14 +- 2 files changed, 150 insertions(+), 39 deletions(-) diff --git a/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt b/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt index 7121e33..d6db4fe 100644 --- a/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt +++ b/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt @@ -1,7 +1,9 @@ package org.libremediaconverter.saf +import android.Manifest import android.app.UiAutomation import android.content.Context +import android.content.pm.PackageManager import android.net.Uri import android.provider.DocumentsContract import android.provider.OpenableColumns @@ -33,6 +35,7 @@ import org.junit.Assert.assertFalse import org.junit.Assert.assertNotEquals import org.junit.Assert.assertNotNull import org.junit.Assert.assertTrue +import org.junit.Before import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith @@ -361,6 +364,71 @@ class SafPickerRoundTripTest { /** Counts [MainActivity] creations from the moment [watchForRecreation] is called. */ private val recreations = AtomicInteger() + /** + * Holds `POST_NOTIFICATIONS`, so tapping Convert cannot open a window this test has to fight. + * + * ## What this replaces, and why the replacement is not a smaller wait + * + * Until #268 the tap was followed by `dismissThePermissionDialog`, which waited for + * `com.google.android.permissioncontroller` to appear and pressed back on it. That is a + * *foreign, focused window* in the middle of the one step this class most needs to be + * deterministic, and it is what mechanism B of #268 was: on the API 35 leg of run 34146936252 + * the tap landed — `START u0 {act=android.content.pm.action.REQUEST_PERMISSIONS ... + * GrantPermissionsActivity}` at 17:38:30.516 — back was pressed at 17:38:32.479, and no + * `ConversionWorker` was ever enqueued in the five minutes that followed. A back press goes to + * whichever window holds *input* focus, and `Until.hasObject` answers about the accessibility + * tree, which can carry the dialog's nodes before it has the focus; a back that arrives one + * window early lands on `MainActivity` and finishes it, which is a screen no `waitUntil` can + * wait for the return of. + * + * ## Why holding the permission removes the window rather than making it less likely + * + * `ConverterScreen` wires Convert to `requestNotifications.launch(POST_NOTIFICATIONS)`, and + * `ActivityResultContracts.RequestPermission.getSynchronousResult` returns + * `SynchronousResult(true)` — *without starting anything* — when + * `checkSelfPermission` already answers `PERMISSION_GRANTED`. So with the permission held there + * is no `GrantPermissionsActivity`, no foreign window, no back press, and nothing this test + * injects can finish the Activity. That is the whole chain, and [holdTheNotificationPermission] + * asserts its one premise rather than assuming it. + * + * ## The KDoc this contradicts, and the measurement that settles it + * + * `convertToTheDefaultFormat` used to say granting "was tried first and did not take — + * `GrantPermissionsActivity` appeared anyway". Re-measured on 2026-09-07, API 34 on this host, + * six consecutive runs of this class: logcat carries **zero** + * `act=android.content.pm.action.REQUEST_PERMISSIONS` starts and zero `GrantPermissionsActivity` + * across all six, and exactly two `WM-SystemJobScheduler: Scheduling work ID` lines per run — + * one for each test that converts, so neither Convert tap was lost. Whatever the earlier + * attempt did, a `pm grant` issued before the tap does take. The assertion below is what keeps + * that from going quietly stale. + * + * ## Two consequences, both deliberate + * + * The grant is **not** undone in teardown: revoking a runtime permission restarts the app's + * process, which would take the rest of the instrumentation run with it. The suite runs without + * Orchestrator, so every class that converts *after* this one now does so with notifications + * permitted. That is benign — `ConversionNotifications` builds its channel at + * `IMPORTANCE_LOW`, so nothing heads-up over the screen — but it is a real change to the + * device state the rest of the run sees, and `NotificationCancelActionTest`'s KDoc is updated + * with it. + * + * And this class no longer takes the denial path. It never asserted anything about it — the + * permission is setup for a test whose subject is SAF — and nothing is lost by it: the + * callback `ConverterScreen` registers is `{ viewModel.convert() }`, which **ignores its + * boolean**, so "converts whichever way the answer goes" is the shape of the code rather than a + * branch a test has to choose. `StaleLauncherResultTest` is what pins that callback path. + */ + @Before + fun holdTheNotificationPermission() { + device.executeShellCommand("pm grant $appPackage ${Manifest.permission.POST_NOTIFICATIONS}") + assertEquals( + "POST_NOTIFICATIONS is not held, so tapping Convert would open a permission dialog " + + "and this class's determinism argument does not hold -- see the KDoc above", + PackageManager.PERMISSION_GRANTED, + context.checkSelfPermission(Manifest.permission.POST_NOTIFICATIONS), + ) + } + /** * Counts a rotation's recreation without asking the Activity anything. * @@ -721,13 +789,10 @@ class SafPickerRoundTripTest { * showed LMC R38 fixtures"*. The default `MP4_H265` produces `video/mp4` and the root is * offered. * - * **The notification dialog is dismissed rather than pre-granted, and that is the honest - * version.** Convert never calls `convert()` directly — it launches `RequestPermission` for - * `POST_NOTIFICATIONS` and converts from the callback **whichever way the answer goes**. So the - * dialog only has to be got out of the way; denying it is a real user's path and the conversion - * still runs. Granting it programmatically was tried first and did not take — - * `GrantPermissionsActivity` appeared anyway, the click that followed went to it rather than to - * the app, and the screen sat in `Ready` with nothing enqueued. + * **The notification permission is held rather than dismissed**, which is #268's mechanism B + * and is argued in [holdTheNotificationPermission]. The short version: `RequestPermission` + * starts no Activity at all when the permission is already granted, so the tap below is + * followed by no foreign window. * * **Both taps scroll first.** On `Ready` the screen carries a file card, five pickers and then * the button, so Convert is below the fold on a phone. `performClick` on an off-screen node @@ -735,38 +800,83 @@ class SafPickerRoundTripTest { * either way — the first version of this sat waiting for a `Converted` that could never come. */ private fun convertToTheDefaultFormat() { + awaitTheProbeHavingLanded() + composeRule.onNodeWithTag(TestTags.Converter.CONVERT) .performScrollTo() .assertIsEnabled() .performClick() - dismissThePermissionDialog() + requireTheTapToHaveStartedTheJob() awaitNode(TestTags.SAVE_FILE, CONVERSION_TIMEOUT_MS) } /** - * Gets the `POST_NOTIFICATIONS` dialog out of the way, if this device shows one. + * Blocks until the pick's probe has been rendered, so no relayout can straddle the next tap. * - * Backing out of it is a denial, and a denial is fine here: the conversion starts either way, - * and what that costs the user is a progress notification confined to the Task Manager. Waiting - * only briefly, because on a device where the permission is already held no dialog appears at - * all and the conversion is already under way. + * **This is #268's mechanism A, and the argument is that it becomes impossible rather than + * unlikely.** `ConversionViewModel.onInputPicked` writes `_state` exactly twice: once with the + * name and size as soon as the metadata query returns, and once more with the probe filled in. + * The second write is what grows the file card, which moves everything below it — including the + * Convert button. Compose's injection computes the target's centre from the semantics node and + * dispatches the touch afterwards; a relayout in that gap hit-tests the stationary coordinate + * against the *new* layout, so the down and the up land on whatever moved into the button's old + * place. Nothing throws. Measured on the two failing gating legs as the gap between the pick's + * FFprobe closing and the tap: 319 ms and 421 ms passed, 46 ms, 98 ms and 124 ms did not. + * + * A detail row can only be composed from that second write, because `FileCard` renders the rows + * exclusively under `input.probe != null`. So once one exists, both of `onInputPicked`'s writes + * have landed and been laid out, and no coroutine in the ViewModel has a `_state` write left in + * flight: `reattach` has either returned on its `_state.value !is Idle` guard or found nothing + * (teardown prunes finished work), and `observe` is not started until `convert()` runs. **The + * card cannot change height again before the tap**, which is a different claim from waiting + * longer. + * + * The `Container` row specifically, rather than a new "probing finished" tag in `main`, because + * this fixture is an MP4 video and that row is already what + * [pickingAFileThroughTheSystemPickerFillsInTheFileCard] waits on and asserts. It is a + * *presence* wait, which cannot be satisfied by a composition that is momentarily absent — an + * absence wait can, and that would tap into nothing. + * + * **Not in [pickTheFixture].** The rotation test does not tap a Compose affordance in this + * window at all, and the picker test already makes this exact wait its own assertion. Putting + * it here keeps a broken read grant reddening one test with the message that explains it. */ - private fun dismissThePermissionDialog() { - if (device.wait(Until.hasObject(By.pkg(PERMISSION_UI_PACKAGE)), PERMISSION_DIALOG_MS) != true) { - return + private fun awaitTheProbeHavingLanded() { + awaitNode(TestTags.Converter.detailRow(CONTAINER_LABEL)) + } + + /** + * Fails fast if the Convert tap started nothing, instead of waiting out the conversion budget. + * + * **A diagnostic, not the synchronisation** — [awaitTheProbeHavingLanded] is what makes the tap + * land, and this cannot rescue a tap that did not. It exists because of what a lost tap used to + * look like: `ComposeTimeoutException`, 300000 ms for `action.saveFile`, five minutes after a + * screen that had never left `Ready`, which names the save affordance and says nothing about + * the tap two steps earlier. Every #268 failure was read from logcat rather than from the + * message, and this is the message it should have had. + * + * The condition is monotonic and needs no budget of its own: `convert()` sets `Converting` + * synchronously, and `Ready` is the only state that renders a Convert button, so once the tag + * is gone it stays gone. [APP_TIMEOUT_MS] rather than a new constant, because "the app should + * have reacted by now" is exactly what that number already means here. + */ + private fun requireTheTapToHaveStartedTheJob() { + val tag = TestTags.Converter.CONVERT + try { + composeRule.waitUntil("the Convert tap left the Ready screen", APP_TIMEOUT_MS) { + // A composition that is momentarily absent throws, and must read as "not yet" + // rather than as "the button is gone" -- see awaitNode. + runCatching { composeRule.onAllNodesWithTag(tag).fetchSemanticsNodes().isEmpty() } + .getOrDefault(false) + } + } catch (timeout: ComposeTimeoutException) { + throw AssertionError( + "the Convert tap did not start a conversion: $tag is still on screen " + + "${APP_TIMEOUT_MS}ms after it was clicked, so the screen never left Ready", + timeout, + ) } - device.pressBack() - device.wait(Until.gone(By.pkg(PERMISSION_UI_PACKAGE)), PERMISSION_DIALOG_MS) - // And wait for the app to be in front again before anything asks Compose about it. - // Querying while another window still owns the screen raises "No compose hierarchies found - // in the app", which is what this test did on an API 35 leg: the back press had landed but - // the dialog had not finished going away. - // - // Asked of UiAutomator rather than through awaitAppFocus, which is the opposite of what the - // class KDoc argues for elsewhere and is right here: awaitAppFocus goes through - // composeRule.waitUntil, so it would raise the very error it is being used to avoid. - device.wait(Until.hasObject(By.pkg(context.packageName)), FOCUS_TIMEOUT_MS) } /** @@ -1205,8 +1315,9 @@ class SafPickerRoundTripTest { * `fetchSemanticsNodes` **throws** `IllegalStateException: No compose hierarchies found in the * app` when nothing is attached at that instant, and `waitUntil` propagates it on the first * poll instead of waiting out the deadline. This class spends much of its time with another - * app in front — the picker, the create-document dialog, the permission dialog — so there is - * always a window where the app is coming back and has no composition yet. Before this, that + * app in front — the picker and the create-document dialog, and until #268 the permission + * dialog too — so there is always a window where the app is coming back and has no composition + * yet. Before this, that * window was a hard failure: measured on the API 34 leg of run 34057196628, where **both** SAF * tests died that way while the same commit passed API 33, 35, 36 and 37, and the previous * commit passed API 34 and failed 35. A failing leg that moves between runs is #190's @@ -1240,12 +1351,6 @@ class SafPickerRoundTripTest { const val PICKER_TIMEOUT_MS = 30_000L const val APP_TIMEOUT_MS = 30_000L - /** The runtime-permission dialog's package, so it can be recognised and dismissed. */ - const val PERMISSION_UI_PACKAGE = "com.google.android.permissioncontroller" - - /** Short: either the dialog is up almost immediately, or the permission was already held. */ - const val PERMISSION_DIALOG_MS = 5_000L - /** * Bounds a hang, and **the first number here was measured on one API level and wrong on * another.** It read 120 s, on the strength of the whole test taking 11.8 s on the API 34 diff --git a/app/src/androidTest/java/org/libremediaconverter/work/NotificationCancelActionTest.kt b/app/src/androidTest/java/org/libremediaconverter/work/NotificationCancelActionTest.kt index 53b218a..abab120 100644 --- a/app/src/androidTest/java/org/libremediaconverter/work/NotificationCancelActionTest.kt +++ b/app/src/androidTest/java/org/libremediaconverter/work/NotificationCancelActionTest.kt @@ -37,10 +37,16 @@ import java.util.concurrent.TimeUnit * ## Why this fires the intent rather than reading the shade * * The obvious version asks `NotificationManager.getActiveNotifications()` for id 1001 and taps what - * it finds. That was rejected: the instrumented suite grants no runtime permissions, so - * `POST_NOTIFICATIONS` is denied throughout, and whether a suppressed foreground-service - * notification is returned there is a platform detail that varies — the test would be asserting - * something about notification *visibility* rather than about cancellation. + * it finds. That was rejected because it would be asserting something about notification + * *visibility* rather than about cancellation — and because whether the shade holds the + * notification at all is not this class's to know. + * + * **It used to say `POST_NOTIFICATIONS` is denied throughout, and since #268 that is no longer + * true.** `SafPickerRoundTripTest` grants it in `@Before`, so that its Convert tap cannot open a + * permission dialog, and a runtime grant cannot be undone in teardown without restarting the app's + * process. The suite runs without Orchestrator, so whether this class sees the permission held + * depends on class order — which is exactly the reading this test does not do, and the reason it + * stays the right shape rather than a reason to change it. * * The `PendingIntent` is the subject; where it is read from is incidental. Building the * notification for a real, live work id and firing its action exercises exactly the thing that can