docs: e2e performance migration rollout plan by ggazzo · Pull Request #40257 · RocketChat/Rocket.Chat

ggazzo · 2026-04-22T17:08:08Z

Summary

New proposal at docs/proposals/e2e-performance-migration.md laying out how we roll out the two performance patterns landed in test: optimize quote messages suite #39691 across the remaining ~150 Playwright specs.
The patterns themselves (benefits, anti-patterns, template, helper catalog, per-file recipe) live in apps/meteor/tests/e2e/README.md — this doc intentionally only covers the rollout (phasing, triage, batch rules, guardrails, handoff to contributors and AI agents).

What's inside

Phase 0 — consolidate and split the utils/ helpers before any migration starts.
Phase 1 — one-shot triage that produces the prioritized worklist (includes explicit do-not-migrate list).
Phase 2 — migration in batches of max 5 specs per PR, with a mandatory PR body template for before/after timings.
Phase 3 — guardrails (review checklist, optional lint, timing watchdog) so we don't regress.
Pickup guide — separate instructions for human contributors and AI agents.

Test plan

Docs-only change — no code paths touched.
Internal links verified (#performance-patterns, #migrating-an-existing-suite, path to e2e README).
Team agreement on the do-not-migrate list and the 5-files-per-PR cap before the first Phase 2 PR lands.

Summary by CodeRabbit

Documentation
- Added end-to-end testing performance migration guidelines and rollout plan to development documentation.

changeset-bot · 2026-04-22T17:08:15Z

⚠️ No Changeset found

Latest commit: be99397

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

dionisio-bot · 2026-04-22T17:08:20Z

Looks like this PR is not ready to merge, because of the following issues:

This PR is missing the 'stat: QA assured' label
This PR is targeting the wrong base branch. It should target 8.5.0, but it targets 8.4.0

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

coderabbitai · 2026-04-22T17:08:34Z

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bf77e382-13c9-4f8a-8870-6265458dea76

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

Walkthrough

Introduces a new documentation file defining an end-to-end rollout plan for migrating the Playwright E2E test suite to performance patterns established in PR #39691. The document specifies baseline metrics, success criteria, a phased execution approach, and contributor guidelines.

Changes

Cohort / File(s)	Summary
E2E Performance Migration Documentation `docs/proposals/e2e-performance-migration.md`	New proposal document outlining a phased migration plan for the Playwright E2E suite, including baseline state, success criteria (helper consolidation, lifecycle behavior, CI time improvements, coverage non-regression), phased execution details (Phases 0-3), and contributor instructions.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Suggested labels

type: chore

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title 'docs: e2e performance migration rollout plan' accurately and concisely describes the main change: a new documentation file introducing an end-to-end rollout plan for migrating the Playwright E2E suite to performance patterns.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

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

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

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

Proposal for rolling out the two performance patterns landed in #39691 across the remaining E2E suite. Patterns themselves live in the e2e README; this doc focuses only on how we migrate 150+ files in batches without regressing coverage or review throughput.

coderabbitai

🧹 Nitpick comments (3)

docs/proposals/e2e-performance-migration.md (3)
94-94: Clarify the "Tests" column in the impact table.

The per-file impact table includes a "Tests" column, but it's unclear what this represents. Is it:

Total number of test cases in the file?

Number of tests that were modified?

Number of tests that were consolidated?

Adding a brief note in the template (e.g., "Tests = number of test cases") would improve clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/proposals/e2e-performance-migration.md` at line 94, Clarify the meaning
of the "Tests" column in the per-file impact table by updating the table header
or adding a brief note in the template: change the header to e.g. "Tests (number
of test cases)" or append a short footnote like "Tests = number of test cases in
the file" near the table in e2e-performance-migration.md so readers know it
represents the total number of test cases per file; update the table header text
or the surrounding explanatory text accordingly to reflect this definition.
61-61: Define "setup-only test.step" criteria.

The term "setup-only test.steps" is used to filter UI setup hits but isn't clearly defined. What distinguishes a setup-only step from a behavior-testing step? Consider adding explicit criteria, such as:

Steps that only create state without assertions

Steps labeled with "setup" or "arrange" in their description

Steps that appear before the first expect() call

This will ensure consistent triage across contributors and AI agents.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/proposals/e2e-performance-migration.md` at line 61, The term "setup-only
test.step"s used in the ui_setup_hits metric is ambiguous; update the proposal
to define deterministic criteria for identifying setup-only steps: steps inside
beforeAll/beforeEach or test.step whose body contains only state-mutating calls
(e.g. content.sendMessage, openLastMessageMenu, btnCreateDiscussionModal,
btnCreateChannel, btnCreateDirectMessage) with no assertions, or whose
description includes canonical flags like "setup" or "arrange", or that occur
entirely before the first expect() in the test; include these three rules (no
assertions, descriptive label, pre-expect position) and an explicit precedence
rule for conflicting cases so contributors and automation can consistently
classify test.step entries.
43-43: Clarify the function signature notation.

The signature sendMessage(api, roomId, msg, { threadId?, asUser? }) is ambiguous. Is the entire options object optional, or just the properties within it? Consider using TypeScript-like notation for clarity:
sendMessage(api, roomId, msg, options?: { threadId?: string, asUser?: User })
This makes it clear that the fourth parameter is optional and contains optional properties.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/proposals/e2e-performance-migration.md` at line 43, Update the
proposal's function signature to remove ambiguity by using TypeScript-style
optional notation: replace `sendMessage(api, roomId, msg, { threadId?, asUser?
})` with `sendMessage(api, roomId, msg, options?: { threadId?: string, asUser?:
User })`, and mention that this unifies `sendMessage` and `sendMessageFromUser`
into a single function where the entire options object is optional and its
properties are individually optional.

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@docs/proposals/e2e-performance-migration.md`:
- Line 94: Clarify the meaning of the "Tests" column in the per-file impact
table by updating the table header or adding a brief note in the template:
change the header to e.g. "Tests (number of test cases)" or append a short
footnote like "Tests = number of test cases in the file" near the table in
e2e-performance-migration.md so readers know it represents the total number of
test cases per file; update the table header text or the surrounding explanatory
text accordingly to reflect this definition.
- Line 61: The term "setup-only test.step"s used in the ui_setup_hits metric is
ambiguous; update the proposal to define deterministic criteria for identifying
setup-only steps: steps inside beforeAll/beforeEach or test.step whose body
contains only state-mutating calls (e.g. content.sendMessage,
openLastMessageMenu, btnCreateDiscussionModal, btnCreateChannel,
btnCreateDirectMessage) with no assertions, or whose description includes
canonical flags like "setup" or "arrange", or that occur entirely before the
first expect() in the test; include these three rules (no assertions,
descriptive label, pre-expect position) and an explicit precedence rule for
conflicting cases so contributors and automation can consistently classify
test.step entries.
- Line 43: Update the proposal's function signature to remove ambiguity by using
TypeScript-style optional notation: replace `sendMessage(api, roomId, msg, {
threadId?, asUser? })` with `sendMessage(api, roomId, msg, options?: {
threadId?: string, asUser?: User })`, and mention that this unifies
`sendMessage` and `sendMessageFromUser` into a single function where the entire
options object is optional and its properties are individually optional.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2edcc46e-f943-47c6-bce7-40d9c2ab0b91

📥 Commits

Reviewing files that changed from the base of the PR and between 5cf4c7c and 8a05976.

📒 Files selected for processing (1)

docs/proposals/e2e-performance-migration.md

📜 Review details

🧰 Additional context used

🧠 Learnings (13)

📓 Common learnings

Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.

Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files

Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests

Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests

Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Implement proper wait strategies for dynamic content in Playwright tests

Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.spec.ts : Use descriptive test names that clearly communicate expected behavior in Playwright tests

Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests

Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests

Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `page.waitFor()` with specific conditions instead of hardcoded timeouts in Playwright tests

Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests

Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests