Skip to content

Commit 8a05976

Browse files
ggazzoggazzo
committed
docs: e2e performance migration rollout plan
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.
1 parent 053b311 commit 8a05976

1 file changed

Lines changed: 142 additions & 0 deletions

File tree

‎docs/proposals/e2e-performance-migration.md‎

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
# E2E performance migration
2+
3+
Rollout plan for applying the two performance patterns landed in PR #39691 across the rest of the Playwright E2E suite. The patterns themselves (benefits, anti-patterns, template, helper catalog, per-file recipe) live in [`apps/meteor/tests/e2e/README.md`](../../apps/meteor/tests/e2e/README.md#performance-patterns). This document is only about *how we roll them out*.
4+
5+
Intended audience: contributors and AI agents who pick up migration work. Each phase is written to be actionable without additional context.
6+
7+
## Current state (baseline)
8+
9+
Measured on `develop` at the time of writing:
10+
11+
- 152 spec files under `apps/meteor/tests/e2e/`.
12+
- 43 files already use `test.describe.serial` — primary candidates for Pattern 2 (shared browser context).
13+
- At least 23 files invoke `poHomeChannel.content.sendMessage` inside setup blocks — primary candidates for Pattern 1 (API-driven seeding).
14+
- Reference data point: `quote-messages.spec.ts` went from ~80s to ~10s in CI after both patterns were applied (PR #39691).
15+
16+
## Success criteria
17+
18+
A migrated suite is considered done when:
19+
20+
1. All setup that does not verify behavior goes through REST helpers.
21+
2. If the suite is `.serial`, the browser context is created once in `beforeAll` and torn down in `afterAll`.
22+
3. Median per-test time in CI drops by at least 30% relative to the pre-migration baseline, or the PR body explains why it did not.
23+
4. No coverage regression: every behavior asserted before the migration is still asserted after (explicitly listed when tests are consolidated).
24+
25+
The project-level target is **p50 < 3s per test** per file. Files above that after migration need a justification in their PR.
26+
27+
## Phase 0 — consolidate helpers
28+
29+
Must land before Phase 2 starts. Blocks nothing else.
30+
31+
Current helpers live in `apps/meteor/tests/e2e/utils/create-target-channel.ts` (mixed responsibilities) and `apps/meteor/tests/e2e/utils/sendMessage.ts` (one function, not re-exported). The new helpers from PR #39691 (`sendMessage`, `createDiscussion`, `createDirectMessageRoom`) sit in `create-target-channel.ts` for historical reasons — they should be moved out.
32+
33+
Deliverables:
34+
35+
1. Split `create-target-channel.ts` into one file per concern:
36+
- `channels.ts` (public channels)
37+
- `groups.ts` (private channels and groups)
38+
- `teams.ts`
39+
- `direct-messages.ts` (`createDirectMessage`, `createDirectMessageRoom`)
40+
- `discussions.ts` (`createTargetDiscussion`, `createDiscussion`)
41+
- `messages.ts` (`sendMessage`, `sendTargetChannelMessage`, `sendMessageFromUser`)
42+
- `rooms.ts` (`deleteRoom`, `deleteChannel`, `deleteTeam`)
43+
2. Unify `sendMessage` and `sendMessageFromUser` into a single function with an options bag: `sendMessage(api, roomId, msg, { threadId?, asUser? })`. Remove the duplicated code path.
44+
3. Re-export everything from `utils/index.ts` so specs never need deeper imports.
45+
4. Add helpers the migration is known to need but that are missing today:
46+
- `createThreadReply(api, roomId, parentMsgId, msg)`
47+
- `inviteUsersToRoom(api, roomId, usernames)`
48+
- `setRoomTopic(api, roomId, topic)` (used in ~6 specs via UI today)
49+
5. Update the helper table in `apps/meteor/tests/e2e/README.md` to reflect the new import surface.
50+
51+
Definition of done: no existing spec broken, no new spec needs to reach past `from './utils'` to seed state.
52+
53+
## Phase 1 — triage
54+
55+
One-shot audit that produces the ordered worklist for Phase 2.
56+
57+
Deliverable: a spreadsheet (or markdown table, committed under `docs/proposals/e2e-migration-triage.md`) with one row per spec file, columns:
58+
59+
- `path`
60+
- `is_serial` (boolean)
61+
- `ui_setup_hits` — count of `content.sendMessage`, `openLastMessageMenu`, `btnCreateDiscussionModal`, `btnCreateChannel`, `btnCreateDirectMessage` occurrences inside `beforeAll` / `beforeEach` and within setup-only `test.step`s
62+
- `ci_median_ms` — last known median from the Playwright report
63+
- `priority_score` — `ci_median_ms * (is_serial + ui_setup_hits)`
64+
- `opt_out_reason` — non-empty if the spec is one of the "do not migrate" cases (see below)
65+
66+
Do-not-migrate list (mark `opt_out_reason`):
67+
68+
- Suites whose subject *is* the setup UI: `create-channel.spec.ts`, `create-direct.spec.ts`, `create-discussion.spec.ts`, `channel-management.spec.ts` (for create flows).
69+
- Auth / session suites: `account-login.spec.ts`, `account-forgetSessionOnWindowClose.spec.ts`, `account-manage-devices.spec.ts`, `enforce-2FA.spec.ts`.
70+
- Federation suite (separate concerns, already covered in `e2e/federation/README.md`).
71+
72+
The audit can be produced with a script (`scripts/e2e-triage.ts` is a reasonable home) or manually for the first pass. The script is optional — the triage file is not.
73+
74+
## Phase 2 — migrate in batches
75+
76+
Rules:
77+
78+
- **Maximum 5 spec files per PR.** Keeps review tractable and preserves bisect granularity.
79+
- Pick files off the triage list in `priority_score` order.
80+
- PRs are independent — no cross-PR dependencies beyond Phase 0.
81+
82+
Per-file recipe is in the README ([Migrating an existing suite](../../apps/meteor/tests/e2e/README.md#migrating-an-existing-suite)). Do not duplicate it here.
83+
84+
Required PR body template for Phase 2:
85+
86+
```markdown
87+
## E2E migration — batch N
88+
89+
### Files
90+
- apps/meteor/tests/e2e/<file-1>.spec.ts
91+
- ...
92+
93+
### Per-file impact
94+
| File | Tests | p50 before (ms) | p50 after (ms) | Δ |
95+
|------|------:|----------------:|---------------:|--:|
96+
| ... | ... | ... | ... | ... |
97+
98+
### Patterns applied
99+
- [ ] Pattern 1 — API seeding
100+
- [ ] Pattern 2 — shared browser context
101+
102+
### Consolidated tests
103+
(list merged tests and confirm each original assertion is still covered — omit if none)
104+
105+
### Not applied
106+
(reason for any pattern not applied on any file)
107+
```
108+
109+
If any file in the batch regresses or stays flat, split that file out into its own PR with a written justification.
110+
111+
## Phase 3 — guardrails
112+
113+
Prevents regression after Phase 2 completes. Can land in parallel with Phase 2.
114+
115+
1. **Doc guardrail** — already in place via README "Anti-patterns to flag in review". Link to it from `.github/pull_request_template.md` under the E2E section.
116+
2. **Lint guardrail (optional)** — a custom ESLint rule or a grep-based CI check that fails when a spec file:
117+
- Uses `poHomeChannel.content.sendMessage` inside `test.beforeEach` or `test.beforeAll`.
118+
- Declares `test.describe.serial` together with `beforeEach(async ({ page }) => { await page.goto(...) })`.
119+
Both are strong signals of missed Pattern 1 / Pattern 2 opportunities.
120+
3. **Timing guardrail** — add a weekly GitHub Action (or extend an existing one) that parses the Playwright report from main and posts a list of spec files with p50 > 3s/test. Recurring offenders become Phase 2 candidates.
121+
122+
## Picking up the work
123+
124+
For contributors:
125+
126+
1. Skim `apps/meteor/tests/e2e/README.md#performance-patterns` and the template.
127+
2. Pick the top unmigrated row from the triage file.
128+
3. Follow the per-file recipe. Open a PR with the template above.
129+
4. One PR = at most 5 files. No exceptions.
130+
131+
For AI agents:
132+
133+
- The per-file recipe is deterministic enough to run end-to-end. The two decisions that require judgement are: whether to apply Pattern 2 (check the preconditions listed in the README), and whether to consolidate tests (requires reading assertions carefully).
134+
- Always run the suite before and after, paste both timings in the PR.
135+
- When adding a helper, update the README table in the same PR.
136+
- Do not migrate files in the do-not-migrate list from Phase 1. If you think one should be removed from that list, raise it in the PR body instead of silently migrating.
137+
138+
## Open questions
139+
140+
- Should timing guardrails block CI (fail the build) or only report? Lean report-only initially.
141+
- How do we measure p50 reliably across runners of different capacity? Current suggestion is median-of-three on a dedicated runner; needs confirmation from infra.
142+
- Do we want per-feature area ownership for Phase 2 batches, or first-come-first-serve?

0 commit comments

Comments
 (0)