JMR-dev/LibreMediaConverter
1
0
Fork 0
Code Issues 16 Pull Requests Actions Packages Projects Releases Wiki Activity

SafPickerRoundTripTest races the probe and the window focus: make it deterministic without weakening what it asserts #268

New Issue
Closed
opened 2026-09-07 18:26:37 +00:00 by JMR-dev · 1 comment
JMR-dev commented 2026-09-07 18:26:37 +00:00 (Migrated from github.com)
Copy Link
Copy Source

SafPickerRoundTripTest fails intermittently on the gating legs — roughly half the runs during
#261, which needed three re-runs to get past it. It is not caused by #261: the same test
failed the same way on test/publish-delete-arm-real-provider at expected: 71, before that branch
existed (runs 34056545386 API 35, 34057706195 API 34).

The symptom is always the same: ComposeTimeoutException waiting on action.saveFile, after a
Convert tap that reached nothing. No WM-SystemJobScheduler: Scheduling work ID, no FFmpeg
command, screen still on Ready five minutes later.

Two mechanisms, both measured from logcat

Diagnosed across four failures in three CI runs.

A — the tap is lost in the post-probe relayout. ConversionViewModel.onInputPicked writes state
twice:

_state.value = ConversionState.Ready(file)                     // name + size, no probe
// ... probe ...
_state.value = ConversionState.Ready(file.copy(probe = probe)) // the file card grows

The second write relays out the file card, which moves the Convert button down. The test's
performScrollTo / assertIsEnabled / performClick straddles that relayout, so the injected tap
lands where the button used to be. Logcat carries no
START u0 {act=android.content.pm.action.REQUEST_PERMISSIONS ...} at all — the tap reached nothing.

Measured gaps between the pick's FFprobe closing (Closed fd N for SAF id M) and the tap:

gap outcome
319 ms, 421 ms pass
46 ms, 98 ms, 124 ms fail

B — the app is not the focused window. One failure had a 416 ms gap and the tap did land
(GrantPermissionsActivity started), back was pressed, and still nothing was enqueued. awaitNode
reported No compose hierarchies found in the app — the hazard dismissThePermissionDialog's own
KDoc already documents.

Do not fix only A. A 416 ms gap failed through B, so any fix keyed purely on the probe having
landed will still flake.

What the fix must satisfy

  1. No timing dependence. A longer wait, a Thread.sleep, a bigger waitUntil budget or a
    retry count is not a fix — it re-tunes the race rather than removing it. The test must
    synchronise on observable state: the probe having landed and the layout having settled before the
    tap is dispatched, and the app window being focused before Compose is queried.
  2. The assertion must not weaken. These two tests exist to prove a save round-trips through the
    real system picker — aSaveWritesToTheDocumentTheSystemPickerCreated that the bytes reach the
    document DocumentsUI created, and aFailedSaveDeletesTheDocumentItCouldNotWrite (#250) that a
    failed save deletes it. Neither may be reduced to "the conversion succeeded" or "some file
    exists". If making it deterministic requires restructuring how the test reaches the save, the
    round trip through the picker must survive the restructure.
  3. It must still fail when the behaviour breaks. Name the mutation and run it — deleting the
    delete-on-failure arm must redden aFailedSaveDeletesTheDocumentItCouldNotWrite, exactly as D4's delete arm has still never run against a real DocumentsProvider: #226 proved the premise, not the fix (#250)
    required.

Worth considering

The second state write may be the real defect rather than the test's problem. A single
Ready(file, probe = null) that fills in without changing the card's height, or a card that
reserves its final height, would remove mechanism A for every test and for users — the button moving
under a finger is a real UI flaw, not only a test hazard. Decide that before patching the test;
if the production fix is right, say so and do that instead.

Notes

  • Both tests carry @FailsOnEmulatorApi37. The marker count and the gating-leg figure must be
    re-derived after any change, per CLAUDE.md — currently 72 tests, 7 markers, 65 gating.
  • Needs its own 33–36 sweep; tools/local-emulator/run-e2e.sh runs them locally.
  • ConverterScreen.kt's format picker is a wrapping FlowRow over OutputFormat.entries sitting
    above the Convert button, so every added output format widens mechanism A's window. #254 added
    one. That is an argument for fixing the cause rather than the symptom.
`SafPickerRoundTripTest` fails intermittently on the gating legs — roughly half the runs during #261, which needed **three re-runs** to get past it. It is **not** caused by #261: the same test failed the same way on `test/publish-delete-arm-real-provider` at `expected: 71`, before that branch existed (runs `34056545386` API 35, `34057706195` API 34). The symptom is always the same: `ComposeTimeoutException` waiting on `action.saveFile`, after a Convert tap that reached nothing. No `WM-SystemJobScheduler: Scheduling work ID`, no FFmpeg command, screen still on `Ready` five minutes later. ## Two mechanisms, both measured from logcat Diagnosed across four failures in three CI runs. **A — the tap is lost in the post-probe relayout.** `ConversionViewModel.onInputPicked` writes state **twice**: ```kotlin _state.value = ConversionState.Ready(file) // name + size, no probe // ... probe ... _state.value = ConversionState.Ready(file.copy(probe = probe)) // the file card grows ``` The second write relays out the file card, which moves the Convert button down. The test's `performScrollTo` / `assertIsEnabled` / `performClick` straddles that relayout, so the injected tap lands where the button *used to be*. Logcat carries no `START u0 {act=android.content.pm.action.REQUEST_PERMISSIONS ...}` at all — the tap reached nothing. Measured gaps between the pick's FFprobe closing (`Closed fd N for SAF id M`) and the tap: | gap | outcome | |---|---| | 319 ms, 421 ms | pass | | 46 ms, 98 ms, 124 ms | **fail** | **B — the app is not the focused window.** One failure had a 416 ms gap and the tap *did* land (`GrantPermissionsActivity` started), back was pressed, and still nothing was enqueued. `awaitNode` reported `No compose hierarchies found in the app` — the hazard `dismissThePermissionDialog`'s own KDoc already documents. **Do not fix only A.** A 416 ms gap failed through B, so any fix keyed purely on the probe having landed will still flake. ## What the fix must satisfy 1. **No timing dependence.** A longer wait, a `Thread.sleep`, a bigger `waitUntil` budget or a retry count is *not* a fix — it re-tunes the race rather than removing it. The test must synchronise on observable state: the probe having landed and the layout having settled before the tap is dispatched, and the app window being focused before Compose is queried. 2. **The assertion must not weaken.** These two tests exist to prove a save round-trips through the *real* system picker — `aSaveWritesToTheDocumentTheSystemPickerCreated` that the bytes reach the document DocumentsUI created, and `aFailedSaveDeletesTheDocumentItCouldNotWrite` (#250) that a failed save deletes it. Neither may be reduced to "the conversion succeeded" or "some file exists". If making it deterministic requires restructuring how the test reaches the save, the round trip through the picker must survive the restructure. 3. **It must still fail when the behaviour breaks.** Name the mutation and run it — deleting the delete-on-failure arm must redden `aFailedSaveDeletesTheDocumentItCouldNotWrite`, exactly as #250 required. ## Worth considering The second state write may be the real defect rather than the test's problem. A single `Ready(file, probe = null)` that fills in without changing the card's height, or a card that reserves its final height, would remove mechanism A for every test and for users — the button moving under a finger is a real UI flaw, not only a test hazard. Decide that before patching the test; if the production fix is right, say so and do that instead. ## Notes - Both tests carry `@FailsOnEmulatorApi37`. The marker count and the gating-leg figure must be **re-derived** after any change, per `CLAUDE.md` — currently 72 tests, 7 markers, 65 gating. - Needs its own 33–36 sweep; `tools/local-emulator/run-e2e.sh` runs them locally. - `ConverterScreen.kt`'s format picker is a wrapping `FlowRow` over `OutputFormat.entries` sitting above the Convert button, so **every added output format widens mechanism A's window**. #254 added one. That is an argument for fixing the cause rather than the symptom.
JMR-dev commented 2026-09-07 19:00:04 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Correction: the "every added output format widens mechanism A" note is wrong

That line is mine and it does not survive reading the layout. Recording it rather than deleting it,
because the ticket was used to brief work and the claim may already have been acted on.

What I wrote: the format picker is a wrapping FlowRow over OutputFormat.entries sitting above
the Convert button, so each added format widens mechanism A's window — and therefore #254 made this
flake worse.

Why it is wrong. Mechanism A is about how far Convert moves between the test's
performScrollTo/assertIsEnabled and the tap being dispatched. The chip row's height is constant
across that transition — it does not depend on the probe — so it contributes nothing to the delta.
Adding a format changes where Convert starts, not how far it moves.

And in the Ready state the chips are not even in that vertical path. ConverterScreen.kt:

257 |     FileCard(s.input)
269 |     Button(
277 |         .testTag(TestTags.Converter.CONVERT),
278 |     ) { Text("Convert") }

Convert sits directly under FileCard, and FileCard (:615) branches on input.probe == null
(:636-637). So the displacement is the card growing when the probe lands, full stop.

What this changes. The "Worth considering" section's argument for a production fix is weaker than
written: it rested partly on the flaw getting worse with every added format, and it does not. The
double state write is still a real UI behaviour worth its own discussion, but it is not degrading
over time, and that removes the urgency this ticket implied.

It does not change the acceptance criteria, which never depended on it: no timing dependence, the
assertion must not weaken, and the mutation must still redden
aFailedSaveDeletesTheDocumentItCouldNotWrite. Mechanism B is likewise untouched — a 416 ms gap
failed through window focus, so a probe-keyed fix alone was never sufficient.

## Correction: the "every added output format widens mechanism A" note is wrong That line is mine and it does not survive reading the layout. Recording it rather than deleting it, because the ticket was used to brief work and the claim may already have been acted on. **What I wrote:** the format picker is a wrapping `FlowRow` over `OutputFormat.entries` sitting above the Convert button, so each added format widens mechanism A's window — and therefore #254 made this flake worse. **Why it is wrong.** Mechanism A is about how far Convert *moves* between the test's `performScrollTo`/`assertIsEnabled` and the tap being dispatched. The chip row's height is constant across that transition — it does not depend on the probe — so it contributes nothing to the delta. Adding a format changes where Convert *starts*, not how far it moves. **And in the `Ready` state the chips are not even in that vertical path.** `ConverterScreen.kt`: ``` 257 | FileCard(s.input) 269 | Button( 277 | .testTag(TestTags.Converter.CONVERT), 278 | ) { Text("Convert") } ``` Convert sits directly under `FileCard`, and `FileCard` (`:615`) branches on `input.probe == null` (`:636-637`). So the displacement is the card growing when the probe lands, full stop. **What this changes.** The "Worth considering" section's argument for a production fix is weaker than written: it rested partly on the flaw getting worse with every added format, and it does not. The double state write is still a real UI behaviour worth its own discussion, but it is not degrading over time, and that removes the urgency this ticket implied. It does not change the acceptance criteria, which never depended on it: no timing dependence, the assertion must not weaken, and the mutation must still redden `aFailedSaveDeletesTheDocumentItCouldNotWrite`. Mechanism B is likewise untouched — a 416 ms gap failed through window focus, so a probe-keyed fix alone was never sufficient.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
above-cut
Worked autonomously overnight: local, JVM-verifiable, no product decision
 accessibility
Barrier affecting people with disabilities
 backlog
Held for manual review: product/UX call, CI/workflow, hardware, or unverifiable here
 bug
Something isn't working
 confirmed
Reviewer demonstrated the defect
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 invalid
This doesn't seem right
 plausible
Reviewer could not fully demonstrate it; treat as unproven
 question
Further information is requested
 sev:high
High severity
 sev:low
Low severity
 sev:medium
Medium severity
 wontfix
This will not be worked on
No labels bug
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
JMR-dev
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: JMR-dev/LibreMediaConverter#268