feat(e2e): Migrate Desktop E2E to Playwright and add merged cross-OS reporting

[yasserfaraazkhan]

This PR completes the Mattermost Desktop E2E migration away from the legacy Electron test stack and standardizes on Playwright.

It does three main things:

• Migrates and stabilizes the Electron E2E suite around Playwright fixtures, shared readiness helpers, isolated userDataDirs, and embedded WebContentsView discovery for Mattermost server tabs.
• Cleans up legacy E2E infrastructure by removing the active electron-mocha/robotjs path and modernizing CI/reporting around Playwright artifacts.
• Improves test correctness and infra reliability by switching menu tests back to native Electron menu/edit actions and replacing shell-based orphan cleanup with PID-based teardown.

Highlights

• Playwright is now the active E2E runner.
• CI now publishes one merged cross-OS Playwright HTML report while keeping 3 OS-specific PR checks.
• Menu-bar coverage is stronger:
  • view_menu uses real View menu items
  • edit_menu uses native Electron edit commands instead of a page-local shim
• Test teardown is more robust:
  • test-mode Electron main processes register their PIDs
  • global teardown kills only registered test PIDs
• Downloads, startup, popup, server-management, and several menu-bar specs were stabilized and verified.

Why this is better

• More deterministic than the old OS-input-driven approach
• Better artifacts for debugging failures: Playwright HTML report, trace, video, screenshot behavior
• Better CI ergonomics with merged reporting across macOS, Linux, and Windows
• Less fragile cleanup and fewer orphaned Electron processes
• Better support for testing embedded Mattermost views that Playwright cannot see as normal windows

NONE

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds and expands end-to-end and unit tests for badge, notification, and permissions behavior across platforms; exposes test-only badge hooks; replaces fullscreen sleeps with deterministic waits; increases several test timeouts; and removes the flaky-test analyzer and its known list.

Changes

Cohort / File(s)	Summary
E2E Timeouts & Fullscreen e2e/specs/deep_linking/deeplink.test.js, e2e/specs/menu_bar/full_screen.test.js	Increased Mocha timeouts to 120s; fullscreen test now waits for window.isFullScreen() via waitForFunction instead of fixed sleeps.
Notification Badge E2E e2e/specs/notification_trigger/notification_badge_windows_linux.test.js	Adds comprehensive Windows overlay-icon and Linux setBadgeCount E2E tests, platform gating, helpers (triggerBadge, getBadgeState, resetBadgeState), and many badge scenario cases.
Permissions IPC E2E e2e/specs/permissions/permissions_ipc.test.js	Adds IPC tests for media access status and Windows-only tests that assert ms-settings: camera/microphone URLs are opened via mocked shell.openExternal.
Removed Flaky-test Utilities e2e/utils/analyze-flaky-test.js, e2e/utils/known_flaky_tests.json	Deletes the flaky-test analyzer and the known flaky tests JSON file (removes analyzeFlakyTests() and known lists).
Badge Test Hooks src/app/system/badge.ts	Adds TestGlobal type; in NODE_ENV === 'test' computes resolvedType, stores {sessionExpired, mentionCount, showUnreadBadge, resolvedType} on global.__testBadgeState, and exposes global.__testTriggerBadge and global.__testTriggerSetUnreadBadgeSetting.
Notification Unit Tests src/main/notifications/Download.test.ts, src/main/notifications/Mention.test.ts, src/main/notifications/Upgrade.test.ts	Adds unit tests covering platform-specific notification behavior (icons, localization, sound selection, UID uniqueness) with mocks for Electron, OS, UUID, i18n, and util version checks.
Notification Integration Tests Expanded src/main/notifications/index.test.ts	Large additions covering DND/priority logic, click/close/timeout handling, active-notification lifecycle, displayDownload/Upgrade/Restart scenarios, mock controls (e.g., mockBlockShow, mockNotificationConstruct), and download-related fixtures (dlMainWindow).
Permissions Manager Tests src/main/security/permissionsManager.test.js	Adds tests for Windows navigation to privacy settings via shell.openExternal, media permission checks, Calls widget flows, and platform-specific behavior; tests reference openWindowsCameraPreferences() and openWindowsMicrophonePreferences() usage.
Misc. Test Flow Adjustments e2e/..., src/...	Various timeout increases and deterministic wait replacements across E2E tests; many new test files increasing test coverage and surface area.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

• wiggin77
• edgarbellot
• esarafianou

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	⚠️ Warning	The PR title mentions Playwright migration and cross-OS reporting, but the actual changeset focuses on adding E2E and unit tests for permissions, notifications, and badge behavior.	Update the title to reflect the actual changes: 'test: add E2E and unit test coverage for permissions, notifications, and badge behavior' or similar.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch add-notification-tests

📝 Coding Plan

• Generate coding plan for human review comments

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/main/security/permissionsManager.test.js (1)

74-112: ⚠️ Potential issue | 🟠 Major

setForServer tests are order-dependent without mock resets.

systemPreferences mock call history persists across tests in this describe, so PM-U06/PM-U07 can fail due to previous tests rather than current behavior.

🧪 Suggested isolation fix

 describe('main/PermissionsManager', () => {
     describe('setForServer', () => {
+        beforeEach(() => {
+            systemPreferences.getMediaAccessStatus.mockReset();
+            systemPreferences.askForMediaAccess.mockReset();
+        });
+
         if (process.platform !== 'linux') {
             it('should ask for media permission when is not granted but the user explicitly granted it', () => {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/security/permissionsManager.test.js` around lines 74 - 112, The
tests under describe('setForServer') share persisted mock call history on
systemPreferences which makes PM-U06/PM-U07 order-dependent; add a beforeEach
inside that describe to clear/reset the mocks (e.g., jest.clearAllMocks() or
specifically systemPreferences.getMediaAccessStatus.mockClear() and
systemPreferences.askForMediaAccess.mockClear()) so each test invoking
PermissionsManager.setForServer runs with a fresh mock state and does not rely
on previous test calls.

🧹 Nitpick comments (2)

e2e/utils/analyze-flaky-test.js (1)

12-21: LGTM – simplification by removing stale flaky-test allowlist.

The change correctly removes the per-OS filtering against a known flaky tests list, which aligns with the PR objective of removing the stale allowlist after fixing the flaky tests.

Minor pre-existing issue in adjacent code: The error path at line 20 returns {}, but the consuming workflow (.github/workflows/e2e-functional-template.yml) destructures newFailedTests and accesses .length. If an error occurs (e.g., report file missing), this will throw a TypeError. Consider returning a consistent shape:

♻️ Suggested fix for error handling consistency

     } catch (error) {
         console.error('Error analyzing failures:', error);
-        return {};
+        return {newFailedTests: [], os: process.platform};
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@e2e/utils/analyze-flaky-test.js` around lines 12 - 21, analyzeFlakyTests
currently returns {} on errors which breaks callers that destructure
newFailedTests and call .length; update the error path in analyzeFlakyTests to
return the same shape as the success path (e.g., { newFailedTests: [], os }) so
callers always receive newFailedTests as an array; locate and modify the catch
block in the analyzeFlakyTests function (references: readJsonFromFile,
generateShortSummary, failedFullTitles, MOCHAWESOME_REPORT_DIR) to log the error
and return the consistent object.

docs/superpowers/specs/2026-03-16-notification-unit-tests-design.md (1)

17-24: Add a language specifier to the fenced code block.

The static analysis tool flagged this code block for missing a language. Adding a language specifier (e.g., text or plaintext) improves rendering consistency and accessibility.

-```
+```text
 src/main/notifications/
   index.test.ts          ← existing file — add NM-01…NM-11, DD-01…DD-05, UV-07…UV-13
   Mention.test.ts        ← NEW (construction logic only)
   Download.test.ts       ← NEW (construction logic only)
   Upgrade.test.ts        ← NEW (NewVersionNotification + UpgradeNotification construction only)
   dnd-windows.test.js    ← existing, untouched

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @docs/superpowers/specs/2026-03-16-notification-unit-tests-design.md around
lines 17 - 24, The fenced code block listing notification test files is missing
a language specifier; update the opening fence from to a language such astext or ```plaintext so the block containing "index.test.ts",
"Mention.test.ts", "Download.test.ts", "Upgrade.test.ts", and
"dnd-windows.test.js" is rendered with the specified language for consistent
formatting and accessibility.

</details>

</blockquote></details>

</blockquote></details>

<details>
<summary>🤖 Prompt for all review comments with AI agents</summary>

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @docs/superpowers/plans/2026-03-16-permissions-manager-tests.md:

• Around line 55-57: Replace the machine-specific cd command and absolute path
  with a repo-relative instruction: remove "cd
  /Users/yasserkhan/Documents/mattermost/desktop" and instruct readers to run the
  test from the repository root using the existing npm script (e.g., "npm run
  test:unit -- --testPathPattern=permissionsManager"); alternatively suggest
  adding a package.json script like "test:permissions" that runs "npm run
  test:unit -- --testPathPattern=permissionsManager" so contributors can run a
  single, portable command.

In @src/app/system/badge.ts:

• Around line 130-132: The test-only global __testBadgeState currently stores
  the raw inputs (sessionExpired, mentionCount, showUnreadBadge) which can hide
  bugs in the actual badge resolution; modify the test branch so it records the
  resolved badge outcome (e.g., resolvedType or resolvedBadge) from the real badge
  resolution logic instead of just inputs—locate where the badge resolution is
  computed (the variable/symbol that holds the final badge decision such as
  resolvedType/resolvedBadge in badge.ts) and include that resolved value in
  __testBadgeState along with any minimal metadata needed for E2E checks, keeping
  this change limited to the process.env.NODE_ENV === 'test' branch.

---

Outside diff comments:
In @src/main/security/permissionsManager.test.js:

• Around line 74-112: The tests under describe('setForServer') share persisted
  mock call history on systemPreferences which makes PM-U06/PM-U07
  order-dependent; add a beforeEach inside that describe to clear/reset the mocks
  (e.g., jest.clearAllMocks() or specifically
  systemPreferences.getMediaAccessStatus.mockClear() and
  systemPreferences.askForMediaAccess.mockClear()) so each test invoking
  PermissionsManager.setForServer runs with a fresh mock state and does not rely
  on previous test calls.

---

Nitpick comments:
In @docs/superpowers/specs/2026-03-16-notification-unit-tests-design.md:

• Around line 17-24: The fenced code block listing notification test files is
  missing a language specifier; update the opening fence from to a language such astext or ```plaintext so the block containing "index.test.ts",
  "Mention.test.ts", "Download.test.ts", "Upgrade.test.ts", and
  "dnd-windows.test.js" is rendered with the specified language for consistent
  formatting and accessibility.

In @e2e/utils/analyze-flaky-test.js:

• Around line 12-21: analyzeFlakyTests currently returns {} on errors which
  breaks callers that destructure newFailedTests and call .length; update the
  error path in analyzeFlakyTests to return the same shape as the success path
  (e.g., { newFailedTests: [], os }) so callers always receive newFailedTests as
  an array; locate and modify the catch block in the analyzeFlakyTests function
  (references: readJsonFromFile, generateShortSummary, failedFullTitles,
  MOCHAWESOME_REPORT_DIR) to log the error and return the consistent object.

</details>

---

<details>
<summary>ℹ️ Review info</summary>

<details>
<summary>⚙️ Run configuration</summary>

**Configuration used**: Repository UI

**Review profile**: CHILL

**Plan**: Pro

**Run ID**: `ef381e77-dfda-4922-902f-c3b08d65ee85`

</details>

<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between 3357df5215b25d9448549a9fb4dd4289051a60f4 and 4b6c60fd9d5c26f5871df234a93cf05585984f5b.

</details>

<details>
<summary>📒 Files selected for processing (15)</summary>

* `docs/superpowers/plans/2026-03-16-permissions-manager-tests.md`
* `docs/superpowers/specs/2026-03-16-notification-unit-tests-design.md`
* `docs/superpowers/specs/2026-03-16-permissions-manager-tests-design.md`
* `e2e/specs/deep_linking/deeplink.test.js`
* `e2e/specs/menu_bar/full_screen.test.js`
* `e2e/specs/notification_trigger/notification_badge_windows_linux.test.js`
* `e2e/specs/permissions/permissions_ipc.test.js`
* `e2e/utils/analyze-flaky-test.js`
* `e2e/utils/known_flaky_tests.json`
* `src/app/system/badge.ts`
* `src/main/notifications/Download.test.ts`
* `src/main/notifications/Mention.test.ts`
* `src/main/notifications/Upgrade.test.ts`
* `src/main/notifications/index.test.ts`
* `src/main/security/permissionsManager.test.js`

</details>

<details>
<summary>💤 Files with no reviewable changes (1)</summary>

* e2e/utils/known_flaky_tests.json

</details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

[coderabbitai]

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/main/notifications/index.test.ts`:
- Line 673: The tests currently assert on the mock-only static
Notification.didConstruct which doesn't exist on Electron's Notification type
and causes TypeScript errors; replace these with a standalone Jest mock function
(e.g., notificationConstructMock) that you call from your Notification test
double/constructor replacement and assert on that mock instead of
Notification.didConstruct; update all occurrences referenced (around the
Notification constructor checks at the previous didConstruct assertions) to
call/expect notificationConstructMock (ensure the mock is created at top of the
test file and invoked inside the mocked Notification implementation used by
tests).
- Around line 489-490: Tests are returning null from mocks but the Map.get()
usages are typed to return undefined; update the mock return values to use
undefined instead of null for
WebContentsManager.getViewByWebContentsId.mockReturnValue and
WebContentsManager.getServerByWebContentsId.mockReturnValue so they match the
expected types (MattermostWebContentsView | undefined and MattermostServer |
undefined) referenced when calling NotificationManager.displayMention; locate
those mocks and replace null with undefined to satisfy TypeScript.
- Around line 573-581: The test uses the unavailable jest.runAllTimersAsync API;
replace it with the synchronous timers API (e.g., jest.runAllTimers() or
jest.advanceTimersByTime(10000)) so the pending 10s timeout in the
NotificationManager.displayMention test is executed in Jest 29.4.1; update the
test that invokes NotificationManager.displayMention (the "NM-07" it block) to
call the synchronous jest timer helper after creating the promise and before
awaiting it.

In `@src/main/notifications/Upgrade.test.ts`:
- Around line 6-7: The test currently imports the i18nManager default as
notMockedLocalizeMessage which is incorrect for TypeScript: change the import to
use the named export localizeMessage instead of the default export (the default
is the i18nManager instance without a localizeMessage property); update the
import line to import { localizeMessage } (or rename as needed) and update any
references to notMockedLocalizeMessage to use the named localizeMessage to
restore proper typing and runtime behavior for the localizeMessage function.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 69b48067-7ab0-4f40-b0c5-3ed23a064fe9

📥 Commits

Reviewing files that changed from the base of the PR and between 4b6c60f and 5e4cefc.

📒 Files selected for processing (7)

• e2e/specs/permissions/permissions_ipc.test.js
• e2e/utils/analyze-flaky-test.js
• src/main/notifications/Download.test.ts
• src/main/notifications/Mention.test.ts
• src/main/notifications/Upgrade.test.ts
• src/main/notifications/index.test.ts
• src/main/security/permissionsManager.test.js

💤 Files with no reviewable changes (1)

• e2e/utils/analyze-flaky-test.js

🚧 Files skipped from review as they are similar to previous changes (2)

• src/main/notifications/Mention.test.ts
• e2e/specs/permissions/permissions_ipc.test.js

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/main/security/permissionsManager.test.js (1)

119-126: Prefer testing Windows settings navigation through a public flow, not private methods.

At Lines 121 and 124, the test directly invokes internal helpers, which makes it brittle to internal refactors. Consider asserting the same behavior via the public permission-handling path that triggers these helpers.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/security/permissionsManager.test.js` around lines 119 - 126, The
test calls internal helpers openWindowsCameraPreferences and
openWindowsMicrophonePreferences directly; instead exercise the public
permission-handling flow on PermissionsManager that triggers those helpers
(e.g., call the public method that requests or handles a 'camera' and
'microphone' permission), keep shell.openExternal mocked/spied, and assert that
shell.openExternal was called with 'ms-settings:privacy-webcam' and
'ms-settings:privacy-microphone'; replace direct calls to
openWindowsCameraPreferences/openWindowsMicrophonePreferences with calls to the
public permission request/handler method so the spec remains stable against
internal refactors.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/app/system/badge.ts`:
- Around line 130-136: The nested ternary expressions used to set resolvedType
inside the process.env.NODE_ENV === 'test' block (within the process.platform
=== 'linux' and else branches) violate the lint rule; replace those ternaries
with equivalent if/else logic to preserve behavior: for the linux branch set
resolvedType to 'mention' if mentionCount > 0, else to 'expired' if
sessionExpired, else 'none'; for the non-linux branch set resolvedType to
'mention' if mentionCount > 0, else to 'unread' if both showUnreadBadge and
showUnreadBadgeSetting are true, else to 'expired' if sessionExpired, else
'none'. Ensure you update the same resolvedType variable (declared as 'let
resolvedType: ...') and keep the surrounding process.env.NODE_ENV and
process.platform checks intact.

---

Nitpick comments:
In `@src/main/security/permissionsManager.test.js`:
- Around line 119-126: The test calls internal helpers
openWindowsCameraPreferences and openWindowsMicrophonePreferences directly;
instead exercise the public permission-handling flow on PermissionsManager that
triggers those helpers (e.g., call the public method that requests or handles a
'camera' and 'microphone' permission), keep shell.openExternal mocked/spied, and
assert that shell.openExternal was called with 'ms-settings:privacy-webcam' and
'ms-settings:privacy-microphone'; replace direct calls to
openWindowsCameraPreferences/openWindowsMicrophonePreferences with calls to the
public permission request/handler method so the spec remains stable against
internal refactors.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: c11c1186-4559-4440-a044-b0b430e59d4a

📥 Commits

Reviewing files that changed from the base of the PR and between 5e4cefc and c5b39eb.

📒 Files selected for processing (3)

• e2e/specs/notification_trigger/notification_badge_windows_linux.test.js
• src/app/system/badge.ts
• src/main/security/permissionsManager.test.js