test(settings): stop an event collector outliving the test that started it - #100
Merged
Conversation
The text has been running the full height of the screen since the last release, and on a phone with a camera in the display it runs straight under it — the reporter has been tilting their Pixel 8 Pro on every page to read the first couple of sentences. Readium applies the page's vertical margins and the display-cutout clearance in the same pass, and that pass only ever ran from the window-insets listener it registers when `shouldApplyInsetsPadding` is on. Turning that flag off to stop it re-paginating the chapter under the reader (#95) therefore took the margins with it, not just the late resize. Nothing else was applying either, so the page went edge to edge: on an emulator with a punch-hole the WebView measured [0,0][1080,2424], where the same build before #95 settles at [0,241][1080,2319] — 136px of camera plus 105px of margin at the top, 105px at the bottom. Quire already owns the reader's insets, so it now owns the page's margins too. ReaderPageArea holds the text clear of `WindowInsets.displayCutout` — Android's own per-device, per-orientation answer to what is covering the screen, so a punch-hole, a notch or nothing at all all come through the same value with no device list to keep current — and adds Readium's own 40dp (20dp landscape) on top of it. Asking for the cutout unconditionally is safe because `windowInsetsPadding` subtracts what an ancestor already consumed: with full-screen reading off the subtree is padded by the system bars, which in portrait cover the cutout, and the page comes away with nothing left to clear rather than inset twice. Two things fall out of putting the margins here rather than in Readium's view tree. The size that is left is measured by the same `onSizeChanged` the re-anchor machinery keys on, so insets arriving a frame late are just another viewport change and the reader keeps its place across them — which is what #95 could not do. And the strip the padding exposes is Quire's to paint, so ReaderTheme now carries the colour ReadiumCSS paints the page with and the margin reads as part of the page instead of a band around it; ReaderThemeTest pins those against Readium's own values so an upgrade that repaints a theme fails there rather than on screen. Verified on an emulator with a punch-hole cutout, against the book from #95. Portrait [0,241][1080,2319] and landscape [136,53][2424,1027] — the camera moves to the left edge on rotation and the page moves with it. Three background-and-resume cycles and three rotate-and-return trips leave the portrait screenshot byte-identical, so #95 stays fixed. With full-screen reading off the page sits at [0,242][1080,2256]: status bar plus margin, the cutout adding nothing. Dark theme samples black in the margin as well as the page. Closes #97
`:app:testDebugUnitTest` timed out twice on main today against a tree that had passed the same tests on its own PR fourteen seconds earlier, and the entire record of it is three lines: the task started, ten minutes passed, the net caught it. The same thing happened on 2026-07-28 and left the same three lines, which is why the budget in this file exists at all. Two incidents, no idea which test either was in — because a test task says nothing between starting and finishing, so a task that never finishes never says anything. Logging the start of every test costs one line each and turns silence into an answer: the last test to report STARTED with no result after it is the one that stopped. Failures carry their exceptions too, for the ordinary case where a test fails rather than hangs. Passes stay quiet — a task that finishes has already said what matters by finishing. This diagnoses rather than fixes. The hang is intermittent, predates the tests running when it last struck, and has never been attributed; the next occurrence will name itself, and then it can be fixed at the source instead of guessed at.
…rted it The test-event logging added in #99 caught its first hang on the run that merged it: `:app:testDebugUnitTest` timed out on main again, and this time the log named the test. `SettingsViewModelTest` reported three of its four tests STARTED, and `restoreInProgressBooks emits RestoreFinished with summary` never reported anything after that — so the wedge is inside that test, not in the reader tests that ran and finished earlier in the same JVM. Three tests in that class collect `vm.events`, a SharedFlow that never completes, from a coroutine launched into the test's own scope, and cancel it on the last line. `runTest` does not return until every child of that scope is done, so the cancel is what lets the test end — and it only runs if all the assertions above it pass. An assertion that fails therefore does not fail the test: it skips the cancel, leaves a collector that will never finish, and the test hangs instead, taking the task and the release with it. `backgroundScope` is what `runTest` provides for exactly this: work that should live as long as the test and be cancelled when it ends, however it ends. ProgressTrackerTest already uses it. The cancels go with it, since there is nothing left to cancel by hand. This removes a way the suite can hang; it does not prove it was the only one. If something in there still wedges, it now does so with a collector that cannot be the cause, and #99's logging will say so.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The test-event logging from #99 caught its first hang on the very run that merged it.
:app:testDebugUnitTesttimed out on main, and the log finally named the test:SettingsViewModelTestreported 3 of its 4 tests STARTED, andrestoreInProgressBooks emits RestoreFinished with summaryreported nothing after that.Three tests in that class collect
vm.events— aSharedFlowthat never completes — from a coroutine launched into the test's own scope, cancelled on the last line.runTestwaits for every child of that scope, so that cancel is what lets the test finish, and it only runs if every assertion above it passes. A failing assertion therefore doesn't fail the test; it skips the cancel and the test hangs instead, taking the task and the release with it. That is a latent hang in any run where those assertions go red, which is consistent with it striking intermittently on main and never on a PR.backgroundScopeis whatrunTestprovides for this, andProgressTrackerTestin:readeralready uses it.Component(s)
Checklist
SettingsViewModelTesttests verified locally)core/identitychanged: matching change on the other side + fixtures updated (N/A)Honest scope
This removes a way the suite can hang. It does not prove it was the only one — the hang is intermittent and I could not reproduce it locally (4/4 passes for this class, 3/3 for the reader tests, plus a full local
testrun). If something in there still wedges, it now does so with a collector that can't be the cause, and #99's logging will name it again.Related issues
refs #93, refs #99