ci(playwright): lint rules for UI-in-setup and unjustified page.reload - #33056
chirag-madlani wants to merge 1 commit into
Conversation
Two new om-playwright rules, both wired at error with the existing eslint-suppressions.json ratchet — new violations fail lint, existing ones are grandfathered and can only shrink. no-ui-in-test-setup Bans page.click / fill / press / selectOption / check / uncheck / setInputFiles / hover / dblclick / tap / dragAndDrop / focus / blur inside test.beforeAll / beforeEach / afterAll / afterEach. page.goto is intentionally NOT banned — navigating to the URL under test is legitimate setup; it is the user-input subset that turns setup into a slow UI journey. Push state via apiContext.<Entity>.create() or a REST helper, matching the canonical pattern in this codebase (sampled from ClassificationVersionPage / ServiceEntityVersionPage / MetricVersionPage and every other healthy suite). Motivation: PR #32594 measured 21% of API calls wasted, mostly from UI-driven setup. Every UI click in setup adds ~30 API calls to the SUT; over ~4200 tests this compounds into the timeouts we then call flakiness. Baseline: 15 sites across 10 files (small, easy to work down). no-page-reload-without-justification Bans bare page.reload(). Legit reloads (persistence tests, service- worker upgrades, SSO return flows) pass with a `// TEST_KEEP_RELOAD: <reason>` comment on the line above or on the same line as the call. Receiver is Page-scoped by identifier heuristic (`page`, `p`, any *Page`, `this.page`, `browser.newPage()` result) so domain objects like `store.reload()` don't trigger. Motivation: measured appBootsPerUIScenario is 2.3, convergence target is ≤1. Every reload boots the SPA entry chunk again (index.tsx's recordPlaywrightAppBoot beacon counts it via a favicon fetch). Bare reloads used as "refresh to see the update" are the dominant contributor — a stale UI after mutation is a product bug the test shouldn't work around. Baseline: 217 sites across 90 files. Both rules follow the existing plugin conventions in playwright/eslint-rules/: pure ESM, no external deps, RuleTester tests under tests/, wired into index.mjs, gated at error in eslint.config.mjs. Docs in .claude/rules/frontend-playwright.md list both rules alongside the existing highest-value constraints. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| const isUiInputCall = (node) => { | ||
| const { callee } = node; | ||
|
|
||
| if (callee?.type !== 'MemberExpression') { | ||
| return false; | ||
| } | ||
|
|
||
| const methodName = getMethodName(callee.property, callee.computed); | ||
|
|
||
| if (methodName === null || !UI_INPUT_METHODS.has(methodName)) { | ||
| return false; | ||
| } | ||
|
|
||
| // Any receiver — `page`, `this.page`, `newPage`, or a Locator variable — | ||
| // is a Playwright surface as long as the method name matches. Narrowing |
There was a problem hiding this comment.
⚠️ Edge Case: no-ui-in-test-setup matches any receiver, not just Page/Locator
isUiInputCall fires on any member call whose method name is in UI_INPUT_METHODS regardless of receiver (return unwrap(callee.object) !== undefined). Several of those names collide with common JS/DOM APIs — notably Array.prototype.fill (new Array(n).fill(0), arr.fill(null)), plus .type, .focus, .blur, .check. Since the rule is registered at error, a legitimate arr.fill(...) inside a beforeAll/beforeEach would fail CI as a bogus UI-in-setup violation. No such call exists in the repo today (so the baseline is clean), but this is a latent false-positive that will block ordinary setup code. Consider narrowing the receiver to a Page/Locator heuristic (as the reload rule does with isPageReceiver) or at least excluding the ambiguous names like fill/type/focus/blur.
Was this helpful? React with 👍 / 👎
| for (const comment of sourceCode.getAllComments()) { | ||
| if (!RELOAD_JUSTIFICATION.test(comment.value)) { | ||
| continue; | ||
| } | ||
| const commentStart = comment.loc?.start.line ?? -1; | ||
| const commentEnd = comment.loc?.end.line ?? commentStart; | ||
| if (commentStart < 0) { | ||
| continue; | ||
| } | ||
|
|
||
| // Leading comment: ends at most 1 line above the reload, and starts | ||
| // no more than MAX_LEAD_LINES above it. | ||
| if (commentEnd <= callLine && callLine - commentEnd <= MAX_LEAD_LINES) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
💡 Quality: One TEST_KEEP_RELOAD comment can justify multiple nearby reloads
hasJustificationComment scans all file comments and matches any TEST_KEEP_RELOAD whose end line is within MAX_LEAD_LINES (3) above the reload call. A single justification comment therefore silently justifies up to three subsequent bare page.reload() calls stacked within 3 lines, weakening the ratchet (those reloads never get counted as violations). Consider requiring the comment to be immediately adjacent (within 1 line) to the reload, or associating each justification with exactly one call.
Was this helpful? React with 👍 / 👎
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
❌ UI Checkstyle Failed❌ Playwright - Guardrails + ESLint + Prettier + Organise ImportsEither a Playwright test file has linting/formatting issues, or a guardrail check failed: ESLint rule unit tests, a new guardrail violation, a stale suppression entry (its violation was fixed but the baseline was not pruned), a blanket Affected filesSubtest: const a = 'server.entity-fetch-error';\n const b = 'server.entity-fetch-error';\n const c = 'server.entity-fetch-error';not ok 27 - the suppressions baseline matches its recorded state exactly fail 1error Command failed with exit code 1. Fix locally (fast - only checks files changed in this branch): make ui-checkstyle-changed |
✅ Playwright Results — workflow succeededValidated commit ✅ 4484 passed · ❌ 0 failed · 🟡 4 flaky · ⏭️ 1 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 58m 15s ⏱️ Max setup 4m 27s · max shard execution 20m 36s · max shard-job elapsed before upload 24m 32s · reporting 17s 🌐 218.24 requests/attempt · 2.31 app boots/UI scenario · 41.10% common-shard skew Optimization targets still in progress:
🟡 4 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
1 participant
Summary
Two new
om-playwright/*ESLint rules — both gated aterrorbehind the existingeslint-suppressions.jsonratchet, so new violations fail lint while existing sites are grandfathered and can only shrink.Direct follow-up to Pere's #32591 (deterministic-failures-hidden-by-flakiness), #32594 (21% wasted API calls), and #32611 (retry/timeout deadlock): both rules attack the same root cause — the tests are doing enough unnecessary work that they stress the SUT, which slows every subsequent test, which trips timeouts, which we call flakiness.
om-playwright/no-ui-in-test-setupBans
page.click/fill/press/selectOption/check/uncheck/setInputFiles/hover/dblclick/tap/dragAndDrop/focus/blurinsidetest.beforeAll/beforeEach/afterAll/afterEach.page.gotoin setup is not banned — navigating to the URL under test is legitimate; it's the user-input subset that turns setup into a slow UI journey.Push state via
apiContext.<Entity>.create()or a REST helper — the canonical pattern already used inClassificationVersionPage,ServiceEntityVersionPage,MetricVersionPageand every other healthy suite.Baseline: 15 sites across 10 files. Small — this rule is mostly a regression guard.
om-playwright/no-page-reload-without-justificationBans bare
page.reload(). Legit reloads (persistence tests, service-worker upgrades, SSO return flows) pass with// TEST_KEEP_RELOAD: <reason>on the line above or on the same line as the call.Receiver is Page-scoped by identifier heuristic (
page,p, any*Page,this.page,browser.newPage()result), so domain objects likestore.reload()don't trigger.Motivation: measured
appBootsPerUIScenario = 2.3, convergence target is≤1. Every reload boots the SPA entry chunk again (index.tsx'srecordPlaywrightAppBootbeacon counts it via a favicon fetch). Bare reloads used as "refresh to see the update" are the dominant contributor — a stale UI after mutation is a product bug the test shouldn't work around.Baseline: 217 sites across 90 files. Meaningful — every one worked down brings the boot ratio closer to 1, which cuts wall time roughly in proportion.
What the ratchet means in practice
page.reload()fails lint immediately. The author fixes or explicitly justifies.eslint-suppressions.json. The file can only shrink — every PR that touches a listed file must fix its listed violations (or the suppression is pruned byyarn lint:playwright:suppressions).no-page-reload-without-justificationineslint-suppressions.jsonand it's the current backlog.Non-goals
appBootsPerUIScenarioratio. This rule prevents further regression and creates the mechanism to work it down; the actual work-down is follow-up.performAdminLogintoday is 2 boots per shard, and there are ~276 files using it).Testing
playwright/eslint-rules/tests/. RuleTester covers hoisted receivers, TS wrappers (!,as,satisfies), computed member access, and the specific evasion shapes the existingno-positional-locatorrule already had to defend against.yarn lint:playwrightpasses with 0 errors, 214 warnings (unchanged — allrequire-aggregation-wait-helperwarnings already present on main).Test plan
yarn test:eslint-rules— 116 passed, 0 failedyarn lint:playwright— 0 errors, 214 pre-existing warningseslint-suppressions.jsonpage.reload()or a UI action inbeforeAllshould fail lint locally and in CI.🤖 Generated with Claude Code