Repository navigation
test(e2e): cover closing the app menu with the scrim and reopening it - #713
Merged
Merged
Conversation
Every link in the open -> scrim click -> close -> reopen chain was already tested, but only with its neighbours mocked: app-menu.browser.test.mjs fires the wrapper click in JSDOM, preload.test.js maps close to the appMenuClose channel, and global-listeners.test.js drives the main process side. Nothing proved a real scrim click in a real renderer reaches the real main process and leaves the menu reopenable. The e2e suites only ever opened the menu once. Verified it fails on broken code rather than assuming it would. Making appMenuClose a no-op fails the close assertion; making appMenuOpen refuse the second open fails all four reopen assertions. The suite carries a long comment about what it does NOT prove, which is the part worth keeping: it cannot show the reopened menu is visible. A WebContentsView can be attached, visible, correctly bounded and backed by a healthy renderer while compositing nothing, which is what happens on Linux when a view is attached twice. In that state every assertion here still passes while the window is blank and dead to mouse and keyboard, because DOM visibility is renderer-side and the renderer is fine. Catching that needs the composited window scored out of process; see experiment/x11-repaint-harness.
…exists Two fixes to the reopen suite. SonarLint flagged 'releases the app menu renderer' as having no assertion, since expect.poll is not recognised as one. Re-asserts the settled state synchronously, matching how the other e2e suites pair an awaited operation with a plain expect. More importantly, the suite's mutation evidence did not survive contact with #712. It was validated against rebuild-on-open, where no app-menu window exists unless the menu was opened. With pre-warming, a menu renderer is always alive - built at startup and after every close - so waitForWindow matches one whether or not the click did anything, and the reopen assertions pass against a menu that was never attached. Measured, with appMenuOpen mutated to refuse the second open: on main 4 of 5 tests fail, as the PR claimed; with #712 merged only 1 fails, via a 30s Playwright click timeout - the failure shape most likely to be dismissed as flake. Waiting on attachment restores it to 5 of 7 failing, and fast. isAppMenuOpen mirrors the existing isFindInPageOpen helper. It says the view is attached, which is strictly weaker than visible: an attached view can composite nothing while its renderer stays healthy, so the suite's standing caveat about what it cannot prove is unchanged. Full suites on Fedora: 1173 unit, 138 e2e.
manusa
force-pushed
the
test/app-menu-reopen
branch
from
September 13, 2026 04:52
960e14f to
1b41093
Compare
|
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
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.
What this covers
The open → scrim click → close → reopen chain, end to end.
Every link is already tested, but only with its neighbours mocked:
close()(JSDOM)src/app-menu/__tests__/app-menu.browser.test.mjs:31close→appMenuClosechannelsrc/app-menu/__tests__/preload.test.js:49src/main/__tests__/global-listeners.test.jsNothing proved that a real scrim click in a real renderer reaches the real main process and leaves the menu reopenable — the e2e suites (
about,help) only ever open the menu once and never close it. This closes that gap, and catches a broken scrim handler, a bad guard inappMenuOpen, or a menu that fails to be rebuilt for the next open.Verified it fails on broken code
A test that has never been seen to fail isn't evidence of anything, so both halves were mutation-checked:
appMenuClosemade a no-opappMenuOpenrefuses the second openWhat it deliberately does NOT prove
The suite carries a long comment about this, and it's the part most worth keeping.
It cannot show the reopened menu is visible. A
WebContentsViewcan be attached,visible, correctly bounded and backed by a perfectly healthy renderer while compositing nothing — which is exactly what happens on Linux when a view is attached a second time (see the comment abovedestroyAppMenuinsrc/main/index.js). In that stateisDestroyed()is false,isLoading()is false, the DOM is laid out, and every assertion in this file still passes while the window is blank and dead to both mouse and keyboard.Playwright cannot see it: DOM visibility is renderer-side and the renderer is fine; what fails is that the view's composited output never reaches the window. Catching that requires scoring the composited window from outside the process — see the
experiment/x11-repaint-harnessbranch.So this suite must not be renamed into something that claims to guard the repaint bug, and a green run here is not evidence that reopening the menu works on Linux.
Testing
New suite passes (5/5). Note
keyboard-shortcuts.test.e2e.jscurrently fails its 2 F11 fullscreen assertions on my machine — reproduced without this change and in isolation, so it is pre-existing environmental flakiness, unrelated to this PR.