test(e2e): Playwright harness and main-process hooks for migration stack - #3855
Merged
Conversation
Part 1 of splitting #3847 — core Playwright config, fixtures, electronApp teardown, and GitHub Actions E2E workflow updates. No new specs yet.
Exposes __e2eTestRefs, message-box stub, tray/deep-link hooks (NODE_ENV=test only) plus directLaunch, testRefs, and shared helper updates.
|
@yasserfaraazkhan: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsI understand the commands that are listed here |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR updates E2E test infrastructure, Playwright project selection, CI caching and setup, Electron process cleanup, helper utilities, app test hooks, and related guidance and dependencies. ChangesE2E infrastructure and CI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
yasserfaraazkhan
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/fixtures/index.ts`:
- Around line 64-67: In the worker fixture teardown within the Promise.race
block, the setTimeout call creates a timeout handle that is never cleared when
cleanupRegisteredElectronProcesses() resolves first, causing the event loop to
remain active. Store the timeout ID returned by setTimeout, and after the
Promise.race completes, clear that timeout using clearTimeout to ensure the
Node.js event loop is not kept alive by the pending callback.
In `@e2e/helpers/cleanup.ts`:
- Around line 33-43: The code in the catch block unconditionally attempts to
unlink the SingletonLock file using fs.unlinkSync(lockFile), but on POSIX
systems (macOS/Linux) this removes the directory entry immediately even while a
live process holds the file descriptor, allowing premature relaunch and
violating Electron's singleton guard. Remove or skip the entire inner try-catch
block that calls fs.unlinkSync(lockFile) to prevent false cleanup success on
POSIX systems in the timeout path, ensuring the lock file persists until the
process truly releases the file descriptor.
In `@e2e/package.json`:
- Line 27: The playwright package is missing from the devDependencies section of
e2e/package.json while `@playwright/test` version 1.61.0 is present. Add
playwright as a devDependency entry in the devDependencies object with version
1.61.0 to match the `@playwright/test` version exactly, ensuring both packages are
version-locked as required by the coding guideline.
In `@e2e/playwright.config.ts`:
- Line 32: The workers constant assignment uses parseInt on the E2E_WORKERS
environment variable but does not validate the result, which can lead to NaN or
non-positive values being assigned. Add validation after parsing to ensure the
resulting value is a valid positive number, falling back to defaultWorkers if
parseInt returns NaN or if the parsed value is less than or equal to zero. This
can be done by checking if the parsed value is a valid positive integer before
assignment to the workers constant.
- Around line 68-72: The wayland project being pushed to the projects array when
E2E_WAYLAND is true is missing the policyFilter spread operator that is applied
to the main platform project. Add the policyFilter spread operator into the
wayland project configuration object (the one with name set to 'wayland' and
grep set to /@wayland/) to ensure policy test exclusion is properly applied when
policy runs are disabled.
In `@e2e/utils/analyze-flaky-test.js`:
- Around line 20-23: The bare catch block in the getXMLParserClass() function
suppresses all exceptions, which masks real errors like syntax errors or missing
peer dependencies. Modify the catch block to check the error's code property and
only suppress MODULE_NOT_FOUND (CommonJS) and ERR_MODULE_NOT_FOUND (ESM) errors.
For all other error types, rethrow the error immediately so actual failures are
properly surfaced rather than falling through to the generic error message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6f1b2683-b80e-4d90-9763-c657b001854b
⛔ Files ignored due to path filters (1)
e2e/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
.github/actions/install-os-dependencies/action.yaml.github/workflows/e2e-functional-template.yml.github/workflows/e2e-functional.ymle2e/AGENTS.mde2e/fixtures/index.tse2e/global-setup.tse2e/global-teardown.tse2e/helpers/appReadiness.tse2e/helpers/cleanup.tse2e/helpers/config.tse2e/helpers/electronApp.tse2e/package.jsone2e/playwright.config.tse2e/utils/analyze-flaky-test.js
mm-cloud-bot
added
release-note-none
and removed
do-not-merge/release-note-label-needed
labels
Fixtures in PR 1 import overlayWindows and other shared helpers from PR 2; merge them so the full E2E suite can run on #3855 before the stack continues.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
e2e/helpers/login.ts (2)
18-24: ⚖️ Poor tradeoffError swallowing in
hasAppShellmay hide real failures.The
.catch(() => false)onrunInRenderersilently converts all errors (including execution context destruction, renderer crashes, or selector bugs) intofalse. While this provides robustness during navigation, it may mask legitimate test setup issues or app crashes.Consider logging unexpected errors before returning
false, or at minimum, check for known transient error messages (like "Execution context was destroyed") before suppressing them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/helpers/login.ts` around lines 18 - 24, In the hasAppShell function, the catch block on the runInRenderer call currently silences all errors without logging. Modify this catch handler to log the error before returning false, so that unexpected errors like execution context destruction or renderer crashes are captured for debugging. You may optionally check if the error is a known transient error message (such as "Execution context was destroyed") and only log non-transient errors to avoid noise in test output.
32-34: ⚖️ Poor tradeoffError swallowing in
hasLoginFormmay hide real failures.The
.catch(() => false)silently suppresses all errors fromrunInRenderer. This is the same pattern as inhasAppShell(lines 18-24). While it makes the polling resilient to navigation, it could hide legitimate bugs or crashes.Consider adding selective error handling to distinguish transient navigation errors from genuine failures.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/helpers/login.ts` around lines 32 - 34, The catch handler in the hasLoginForm function currently suppresses all errors returned from runInRenderer by catching any error and returning false. Instead of catching all errors indiscriminately, implement selective error handling that only catches transient navigation-related errors that are expected to occur during polling (such as navigation errors or timeout errors), and allows genuine failures or unexpected errors to propagate or be logged. This way, the function remains resilient to expected navigation transitions while still surfacing real bugs or crashes.src/main/app/initialize.ts (1)
315-333: 💤 Low valueClicking disabled or invisible menu items may produce unexpected behavior.
The tray menu traversal (lines 315-333) finds items by label and invokes
item.click()without checkingitem.enabledoritem.visibleproperties. While this may be acceptable for E2E testing scenarios that need to verify behavior regardless of UI state, it could mask issues where tests should fail when interacting with disabled menu items.Consider adding a validation step or documenting this behavior if intentional.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/app/initialize.ts` around lines 315 - 333, In the __e2eClickTrayMenuItem test field function, the code finds and clicks menu items without verifying the item.enabled and item.visible properties. Before calling item.click(), add a condition to check that the item is both enabled and visible (ensure both properties are true or undefined since that typically means enabled/visible by default). If the item is disabled or invisible, either skip it and continue searching through the stack, or document why clicking disabled items is intentional for this E2E testing scenario.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/helpers/testRefs.ts`:
- Around line 74-93: The transient error handling logic that checks for
"Execution context was destroyed" and "Target page, context or browser has been
closed" is partially re-implemented in the getMainWindowId function's catch
block and again elsewhere in the module around lines 106-113. This inconsistent
duplication can cause tests to fail on transient context churn that the module
intends to tolerate. Extract this error checking logic into a reusable helper
function and apply it consistently across all locations in the module where
app.evaluate() is called, ensuring all transient errors are uniformly handled
through the expect.poll retry mechanism instead of being partially bypassed in
different places.
---
Nitpick comments:
In `@e2e/helpers/login.ts`:
- Around line 18-24: In the hasAppShell function, the catch block on the
runInRenderer call currently silences all errors without logging. Modify this
catch handler to log the error before returning false, so that unexpected errors
like execution context destruction or renderer crashes are captured for
debugging. You may optionally check if the error is a known transient error
message (such as "Execution context was destroyed") and only log non-transient
errors to avoid noise in test output.
- Around line 32-34: The catch handler in the hasLoginForm function currently
suppresses all errors returned from runInRenderer by catching any error and
returning false. Instead of catching all errors indiscriminately, implement
selective error handling that only catches transient navigation-related errors
that are expected to occur during polling (such as navigation errors or timeout
errors), and allows genuine failures or unexpected errors to propagate or be
logged. This way, the function remains resilient to expected navigation
transitions while still surfacing real bugs or crashes.
In `@src/main/app/initialize.ts`:
- Around line 315-333: In the __e2eClickTrayMenuItem test field function, the
code finds and clicks menu items without verifying the item.enabled and
item.visible properties. Before calling item.click(), add a condition to check
that the item is both enabled and visible (ensure both properties are true or
undefined since that typically means enabled/visible by default). If the item is
disabled or invisible, either skip it and continue searching through the stack,
or document why clicking disabled items is intentional for this E2E testing
scenario.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a32daa94-1821-4304-8e45-69e396283de5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
e2e/helpers/dialog.tse2e/helpers/directLaunch.tse2e/helpers/login.tse2e/helpers/notificationEffects.tse2e/helpers/overlayWindows.tse2e/helpers/prepareServerView.tse2e/helpers/serverMap.tse2e/helpers/serverView.tse2e/helpers/testRefs.tse2e/helpers/tray.tspackage.jsonsrc/main/app/initialize.test.jssrc/main/app/initialize.tssrc/main/notifications/index.tssrc/main/testMessageBoxStub.ts
✅ Files skipped from review due to trivial changes (2)
- e2e/helpers/overlayWindows.ts
- src/main/notifications/index.ts
github-actions
Bot
added
E2E/Run
lieut-data
approved these changes
Jun 30, 2026
lieut-data
left a comment
lieut-data
left a comment
Member
There was a problem hiding this comment.
Thanks, @yasserfaraazkhan! A few comments below, but not strictly blocking -- I don't need to re-review after you've decided next steps.
| ServerManager.on(SERVER_URL_CHANGED, updateServerInfo); | ||
| ServerManager.on(SERVER_PRE_AUTH_SECRET_CHANGED, updateServerInfo); | ||
|
|
||
| if (process.env.NODE_ENV === 'test') { |
Member
There was a problem hiding this comment.
Looks like setTestField already checks process.env.NODE_ENV === 'test' -- can we simplify this?
| setTestField('__e2eClickTrayMenuItem', (label: string) => { | ||
| const menu = createTrayMenu(); | ||
| const stack = [...menu.items]; | ||
| while (stack.length > 0) { |
Member
There was a problem hiding this comment.
It took me a while to realize what was going on here -- flattening the menu in a kind of manual depth-first-search.
Would we be open to just writing this recursively, ala something like:
setTestField('__e2eClickTrayMenuItem', (label: string) => {
const truncated = label.length > 50 ? `${label.slice(0, 50)}...` : label;
function clickItem(items: Electron.MenuItemConstructorOptions[]): boolean {
for (const item of items) {
const itemLabel = typeof item.label === 'string' ? item.label : '';
if (
(itemLabel === label || itemLabel === truncated) &&
item.enabled !== false &&
item.visible !== false &&
typeof item.click === 'function'
) {
item.click();
return true;
}
if (item.submenu?.items && clickItem(item.submenu.items)) {
return true;
}
}
return false;
}
if (!clickItem(createTrayMenu().items)) {
throw new Error(`Tray menu item not found: ${label}`);
}
});
github-actions
Bot
added
E2E/Run
yasserfaraazkhan
added a commit
that referenced
this pull request
Jun 30, 2026
Resolve conflicts by keeping merged harness/hook fixes from #3855 and retaining PR 3 Mattermost shell helpers and specs. Co-authored-by: Cursor <cursoragent@cursor.com>
yasserfaraazkhan
added a commit
that referenced
this pull request
Jun 30, 2026
Resolve harness/hook conflicts by keeping merged #3855 changes. Co-authored-by: Cursor <cursoragent@cursor.com>
yasserfaraazkhan
added a commit
that referenced
this pull request
Jun 30, 2026
Align notification trigger tests with #3855 helper rename after master merge. Co-authored-by: Cursor <cursoragent@cursor.com>
yasserfaraazkhan
added a commit
that referenced
this pull request
Jun 30, 2026
Resolve harness/hook conflicts by keeping merged #3855 changes. Co-authored-by: Cursor <cursoragent@cursor.com>
yasserfaraazkhan
added a commit
that referenced
this pull request
Jun 30, 2026
Resolve harness/hook conflicts by keeping merged #3855 changes. Co-authored-by: Cursor <cursoragent@cursor.com>
yasserfaraazkhan
added a commit
that referenced
this pull request
Jun 30, 2026
Resolve harness/hook conflicts by keeping merged #3855 changes. Co-authored-by: Cursor <cursoragent@cursor.com>
yasserfaraazkhan
added a commit
that referenced
this pull request
Jun 30, 2026
Resolve harness/hook conflicts by keeping merged #3855 changes. Co-authored-by: Cursor <cursoragent@cursor.com>
yasserfaraazkhan
added a commit
that referenced
this pull request
Jun 30, 2026
Resolve harness/hook conflicts by keeping merged #3855 changes. Co-authored-by: Cursor <cursoragent@cursor.com>
yasserfaraazkhan
added a commit
that referenced
this pull request
Jun 30, 2026
Resolve harness/hook conflicts by keeping merged #3855 changes. Co-authored-by: Cursor <cursoragent@cursor.com>
This was referenced Jul 1, 2026
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
4 participants
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.
This is a small set of changes to get CI running Playwright. This will put in the main hooks required by e2e tests specs to run tests from these pr. (#3857–#3864).
Summary
Playwright’s Electron driver gives us a
BrowserWindowpages for the app shell. Each Mattermost server renders in aWebContentsView, which is not a page we get from normal playwright execution. Finding and driving a specific server means going through main-process singletons like ViewManager, WebContentsManager, etc and not the page.click() alone.To automate these flow we are using hooks. The
app.evaluate()(main-process context) plusglobal.__e2e*refs to reachViewManager,WebContentsManager, tray actions, etc.Its the same case with tray menus and native certificate dialogs. They are OS/main-process and not in the DOM. We need the
global.__e2erefs.In master we already have __e2eTestRefs (MainWindow, ServerManager, TabManager, ViewManager, WebContentsManager), __e2eAppReady (intercom.ts), dock/taskbar badge hooks (badge.ts).
In this PR we are add more helper to test the scenarios mentioned in the beginning.
We do not want to push hooks to production app. In webapp we attache test fields to DOM. Here we're publishing handles on the Node main-process global. NODE_ENV === 'test' keeps normal user installs from getting those globals.
Change Impact: 🟡 Medium
Regression Risk: Refactors and expands the cross-platform Playwright/Electron E2E harness (worker/global Electron main-process lifecycle, PID-registry reaping, teardown/“fast teardown” termination behavior, and evaluation/timeout/retry logic), plus CI workflow/caching and report-tag generation. While production runtime behavior is gated to
NODE_ENV=testviasetTestField/test-only stubs (message box, tray/deep-link/menu click wiring, flash effects exposure), the teardown and main-process evaluation/timeout paths are sensitive and could cause flaky E2E behavior, orphaned processes, or teardown timing regressions—especially on macOS.QA Recommendation: Rely primarily on automated CI E2E coverage; minimal manual QA. If any failures occur, manually validate on the failing OS job(s) that the existing specs run under Playwright, the failing scenario reproduces reliably, and Electron shuts down cleanly with no orphan processes (and that behavior differs as expected between normal vs “fast teardown”), paying extra attention to macOS defaults snapshot/restore.
Generated by CodeRabbitAI