Playwright guardrails (3/N): burn down positional locators in e2e/Pages - #31028
Closed
ShaileshParmar11 wants to merge 23 commits into
Closed
Playwright guardrails (3/N): burn down positional locators in e2e/Pages#31028ShaileshParmar11 wants to merge 23 commits into
ShaileshParmar11 wants to merge 23 commits into
Conversation
Adds the CommonJS eslint-rules plugin skeleton (rules: {}) that later
tasks register rules into, wires it into eslint.config.mjs under the
om-playwright namespace, and adds a node --test based unit test target.
Also adds an eslint.config.mjs block scoping the plugin's own .js files
to CommonJS globals/no-require-imports:off, and switches the
test:eslint-rules script to a glob instead of a bare directory path --
both needed because `node --test <dir>` does not recurse into an
explicit directory argument on this repo's pinned Node (22.17.0),
confirmed against the .nvmrc version, not just the locally active one.
…d fix 16 races Registers a listener promise before the triggering action instead of awaiting waitForResponse() after it, which races an already-fired response and hangs until timeout. Zero suppressions — all 16 pre-existing violations fixed by hand.
…n, not as a weaker loader wait Review found the explore.ts fix from the prior commit substituted a strictly weaker wait (toHaveCount(0) is a no-op if the loader hasn't mounted yet) with an inaccurate comment. selectDataAssetFilter has exactly one call site, so the listener moves to EntitySummaryPanel.spec.ts's single beforeEach, before the sidebarClick that actually triggers the query.
…test objects - beforeEach runs on the triggering test's own timeout slot (verified against playwright@1.57.0 worker/workerMain.js), so test.slow() there has the same blast radius as a describe-scope call; the previous heuristic treated it as legal. beforeAll/afterAll/afterEach each get their own slot and correctly remain legal. - Widen detection to aliased test objects (e.g. base.slow() inside base.describe(), where base = test.extend(...)) via a dynamic test-object-identifier signal, while excluding variables bound directly to the describe function itself (const d = test.describe;) from being misclassified as per-test callbacks. - Add RuleTester cases pinning beforeEach, beforeAll, afterEach, test.step, aliased test objects, and the describe-alias edge case. - Re-snapshot: 89 violations (79 + 9 beforeEach + 1 aliased describe), suppressions total 94 (unchanged, 4 promoted rules) + 89 = 183.
Flags e2e tests whose body only performs page/locator interactions
(clicks, fills, navigation) with no expect() reference anywhere and no
call to anything beyond the test's own Playwright fixtures - i.e. tests
that are provably assertion-free.
The rule intentionally does not flag delegation to a helper or
page-object method (entity.descriptionUpdate(page), addUser(...),
verifyAuthenticated(...)), since those may assert internally and
proving otherwise needs interprocedural analysis this rule doesn't do.
An earlier text-scan-only design flagged 474 tests, but hand-verifying
samples showed most delegate to helpers that do assert - a large,
confirmed false-positive rate. Scoping the rule to only what it can
prove (no expect text + every call rooted at a page fixture) drops
that to a single, high-confidence violation:
playwright/e2e/nightly/ServiceIngestion.spec.ts's "Default Pagination
size should be 15" test, which waits on two network patterns and
clicks once but never asserts a status, body, or visible count.
Registered as om-playwright/require-assertion-per-test, scoped to
playwright/e2e/**/*.spec.{js,jsx,ts,tsx} only (utils/support files
contain no tests). Snapshot suppresses the one existing violation via
--suppress-rule; the four already-promoted playwright/* suppressions
(94) and om-playwright/no-blanket-test-slow (89) are untouched.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e-assertion-per-test test.slow(true), test.setTimeout(...), and test.step(...) were being treated as ordinary non-page calls, so any test opening with one of them (89 suppressed sites use test.slow()) was silently exempted no matter how purely it clicked afterward - none of them can assert. A call rooted at the `test` identifier is now filtered out before the "every remaining call is page/locator-only" check, instead of counting either way. test.step's own nested calls are unaffected: they're separate CallExpression nodes checked on their own merits, since the callback is inline and fully visible, not a delegated helper. Corpus impact: still exactly 1 violation, same site (ServiceIngestion.spec.ts:189) - no re-snapshot needed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…row helper defaults
Widen no-positional-locator to flag Identifier/MemberExpression receivers
(hoisted-variable and property-stored locators), not just inline call-chain
tails - 185 previously-missed sites, closing the cheapest evasion path
(hoist into a variable). Discriminate genuine Playwright .first()/.last()/
.nth() calls from same-named non-Locator calls (e.g. lodash) by argument
arity instead.
Fix scopedLocators helpers: getRowByName defaulted to a CSS attribute
selector that never matches implicit ARIA roles (Ant Design <tr> rows),
and expectRowFor never forwarded rowSelector. Default to page.getByRole('row')
and thread rowSelector through both helpers; document the hasText
substring-collision failure mode.
Re-snapshot: no-positional-locator suppressions 1068 -> 1253 via
--suppress-rule (never --suppress-all/--prune-suppressions); the three
pre-existing suppression groups are byte-identical.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e escape hatch
Add no-unscoped-count-assertion and require-unique-entity-name (heuristic,
expect false positives) plus justified-rule-disable, the escape hatch that
lets a developer document why a heuristic misfire is being suppressed.
require-unique-entity-name is implemented, tested, and registered in the
plugin but deliberately not enabled: a raw pass found 1625 violations, and
a 25-sample hand review found 0/25 true positives (matches were almost
entirely Playwright's own getByRole(..., { name }) option and static
config keys, not entity creation). Snapshotting that baseline would ratchet
near-total noise; left for a follow-up to narrow the heuristic. The other
two rules were sanity-checked, found to carry real signal, and are
snapshotted normally (+73, +9 violations; suppressions total 1437 -> 1519).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e-entity-name
Both were reviewed against their full/near-full population, not just the
sanity-check samples from the initial implementation:
- require-unique-entity-name: confirmed ~3/1625 (0.18%) true-positive
rate. The heuristic can't distinguish assigning a name to a new entity
from using one to look an existing element up — 59% of hits are
Playwright's own getByRole/getByLabel(role, { name }) accessible-name
option, where adding uuid() would break the locator. 43/51 files under
playwright/support/entity/ already call uuid() internally, so
uniqueness is solved one layer down from where this rule looked.
Never enabled, so no suppressions to remove.
- no-unscoped-count-assertion: full classification of all 73 sites found
~77% false positives (17 true / 56 false), dominated by the rule
flagging locators already scoped through a variable per its own
recommended fix (const c = loc.filter({ hasText }); expect(c)...) —
failing code for following its own advice. Suppressions pruned via a
full-corpus --prune-suppressions run; total 1519 -> 1446.
justified-rule-disable is untouched (its count of 9 was independently
re-verified) and now registers as the fifth rule in the plugin.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ate guidance Docs previously listed a hand-written, stale error/warn split for the Playwright ESLint rules that no longer matched eslint.config.mjs (every rule now runs at error). Generate the rule table from the config instead, so the docs can't drift from what CI actually enforces, and consolidate the overlapping rules file and skills onto the generated table and the real lint:playwright:suppressions gate.
Adds a corpus meta-test that fails if eslint-suppressions.json's total grows (ceiling 1446, the current baseline), and wires a full-corpus CI step running the rule unit tests, the suppressions-aware lint with pruning, and the generated rule-table freshness check. The step is unconditional rather than changed-files-scoped because --prune-suppressions against a partial file set would delete valid suppressions for files the PR didn't touch. Until this lands, all 18 Playwright guardrail rules only ran locally. Also updates skills/playwright-validation/SKILL.md's stale three-rule/warn-tier description to reflect that every rule is now error-level with no warn tier.
…wright guardrail step --prune-suppressions writes eslint-suppressions.json in place when it drops a stale entry, and the step never restored it — a later step in the same job (lint_core_components) runs a repo-wide git status check that could then misattribute this failure to itself. Restore the file unconditionally on both the pass and fail paths. Also guard the failure-detail grep with `|| true`: under GitHub's default `bash -eo pipefail` shell, a grep with no matches exits non-zero and would abort the script before the GITHUB_OUTPUT writes, leaving the PR summary comment blank.
Playwright already runs with retries: 1 and the JSON reporter is already enabled, so flaky/failed data is produced and discarded on every CI run. Add a small aggregator that turns results.json into a flake-report.json summary per shard, uploaded as an artifact. Purely additive: no new test runtime, no changes to any existing step or job outcome.
…load Four fixes from final branch review: - ui-checkstyle: run the non-pruning yarn lint:playwright:full so a fixed violation fails CI until its stale suppression is pruned and committed, instead of silently discarding the rewrite; drop the now-dead git checkout restore; update the failure-reason text accordingly. - justified-rule-disable rule + CI grep guard: a bare eslint-disable (no rule list) or a disable of the rule itself previously bypassed all 18 guardrail rules undetected; the rule now reports blanket disables and a CI-level grep guard catches the self-referential case the rule cannot see. - playwright-postgresql-e2e: add continue-on-error/overwrite/if-no-files-found to the flake-report upload so artifact-name collisions on job re-run don't fail an otherwise-passing 40-minute shard. - docs: point developer-facing lint instructions at yarn lint:playwright:suppressions to match the new CI command split. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI's changed-files checkstyle runs prettier, which the guardrail work did not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ShaileshParmar11
requested review from
a team,
akash-jain-10,
harshach and
tutte
as code owners
August 5, 2026 12:01
Contributor
❌ 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 |
github-actions
Bot
added
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR continues the Playwright guardrails rollout by reducing positional locator usage across playwright/e2e/Pages/**, and aligning repo guidance/tooling around the new full-corpus lint gate (yarn lint:playwright:suppressions).
Changes:
- Refactors many Playwright Page specs to avoid
.first()/.last()/.nth()by narrowing locators, scoping to stable identifiers, using count assertions, or adding justified disables where position is the semantic. - Introduces/updates Playwright guardrail tooling: custom
om-playwrightESLint rules + rule tests, generated handbook rule table script, and flake report aggregation. - Updates skills/docs/CI configs to reference the suppressions ratchet lint command and generated rule table.
Reviewed changes
Copilot reviewed 88 out of 88 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| skills/writing-playwright-tests/SKILL.md | Redirects authorship guidance to the consolidated playwright skill. |
| skills/test-enforcement/SKILL.md | Updates Playwright lint verification command to lint:playwright:suppressions. |
| skills/playwright/SKILL.md | Updates lint gate guidance + merges “Writing Playwright Tests” content into this skill. |
| skills/playwright-validation/SKILL.md | Updates validation checklist and lint command for the new gate. |
| openmetadata-ui/src/main/resources/ui/scripts/generate-playwright-rule-table.js | Generates/validates the handbook’s rule table from eslint.config.mjs. |
| openmetadata-ui/src/main/resources/ui/scripts/aggregate-flake-report.js | Aggregates Playwright JSON reporter output into a flake summary JSON. |
| openmetadata-ui/src/main/resources/ui/scripts/aggregate-flake-report.test.js | Unit tests for flake report aggregation. |
| openmetadata-ui/src/main/resources/ui/playwright/utils/waitHelpers.ts | Adds clickAndWaitFor helper to avoid waitForResponse-after-action races. |
| openmetadata-ui/src/main/resources/ui/playwright/utils/scopedLocators.ts | Adds getRowByName / expectRowFor helpers to avoid position-based row selection. |
| openmetadata-ui/src/main/resources/ui/playwright/utils/user.ts | Registers response listeners before actions in user helper flows. |
| openmetadata-ui/src/main/resources/ui/playwright/utils/glossary.ts | Registers response listeners before actions in glossary helper flows. |
| openmetadata-ui/src/main/resources/ui/playwright/utils/explore.ts | Removes an early waitForResponse call before clicking a filter. |
| openmetadata-ui/src/main/resources/ui/playwright/utils/customProperty.ts | Ensures response listener is registered before triggering search. |
| openmetadata-ui/src/main/resources/ui/playwright/PLAYWRIGHT_DEVELOPER_HANDBOOK.md | Updates lint command, documents ratchet behavior, and replaces rule table with generated section. |
| openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/index.js | Exports local om-playwright ESLint rules. |
| openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/no-positional-locator.js | Implements rule banning .first()/.last()/.nth() locator calls. |
| openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/no-blanket-test-slow.js | Implements rule banning test.slow() at file/describe/beforeEach scope. |
| openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/require-assertion-per-test.js | Implements rule requiring at least one assertion in e2e tests (conservative detection). |
| openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/require-response-listener-before-action.js | Implements rule disallowing await page.waitForResponse(...) patterns. |
| openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/justified-rule-disable.js | Enforces -- <why> justification on disabling Playwright rules and blocks blanket disables. |
| openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/tests/smoke.test.js | Plugin export smoke test. |
| openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/tests/no-positional-locator.test.js | RuleTester suite for positional-locator rule. |
| openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/tests/no-blanket-test-slow.test.js | RuleTester suite for blanket test.slow rule. |
| openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/tests/require-assertion-per-test.test.js | RuleTester suite for assertion-per-test rule. |
| openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/tests/require-response-listener-before-action.test.js | RuleTester suite for response-listener ordering rule. |
| openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/tests/justified-rule-disable.test.js | RuleTester + pinned Linter behavior test for disable comment self-suppression. |
| openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/tests/corpus.test.js | Pins suppression-count ceiling to enforce monotonic “baseline only shrinks”. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Users.spec.ts | Removes positional locators and scopes persona/avatar selection; converts some existence waits. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/UserDetails.spec.ts | Replaces positional locators with scoped lookups and count-based waits; adds justified disables where needed. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/TestSuiteDetailsPage.spec.ts | Switches modal checkbox selection to a known test-case test id. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Teams.spec.ts | Removes .first() from switch locator to rely on strictness/scoping. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/TeamAssetsRightPanel.spec.ts | Removes .first() from right-panel link locator. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/TasksUIFlow.spec.ts | Removes positional locators when opening task cards and verifying task presence. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/TaskFormSettings.spec.ts | Removes .first() from AntD option selection chain. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/TaskComments.spec.ts | Removes .first() from task-card locator. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Tags.spec.ts | Converts loader waits to count assertions; adds justified positional disable where duplicates are known. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/TagPageRightPanel.spec.ts | Removes .first() from right-panel link locator. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Tag.spec.ts | Converts loader waits to count assertions; removes .first() from classification entry selection. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/ServiceListing.spec.ts | Removes .first() from visibility assertion. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/SearchSettings.spec.ts | Keeps .first() with justified disables where “first configured field” is intentional. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/SearchIndexApplication.spec.ts | Converts “wait for first card visible” to count assertions. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Roles.spec.ts | Removes positional selection and narrows delete button locator. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/ProfilerConfigurationPage.spec.ts | Converts initial row wait to count assertion; uses justified .first() while draining rows. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Policies.spec.ts | Replaces positional selectors with scoped rule-card targeting; adds justified positional disables where position is semantic. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/ODCSImportExport.spec.ts | Removes .first() from description/markdown locators. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/LogsViewer.spec.ts | Removes .first() from logs button locators. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/LiveIndexingTab.spec.ts | Converts .first()-based “exists” checks to count assertions. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Lineage/PlatformLineage.spec.ts | Moves waitForResponse before click dispatch events. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Lineage/LineageInteraction.spec.ts | Removes .first() from close button; keeps .nth(i) with justified disable for iteration. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/IntakeForm.spec.ts | Replaces positional locators with better scoping; keeps justified positional uses where UI structure requires it. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/InputOutputPorts.spec.ts | Replaces positional count checks with helper-based assertions. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/GlossaryTermRightPanel.spec.ts | Removes .first() from right-panel link locator. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/GlossaryTermRelationSettings.spec.ts | Removes .first() from Next Page button role lookup. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/GlossaryImportExport.spec.ts | Adds justified positional disables for overlays/grid cell order; removes some .first() usages. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/GlossaryFormValidation.spec.ts | Removes .first() from form-error selection; scopes duplicate-name error lookup. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Glossary.spec.ts | Removes .first() where safe; adds justified positional disables where ordering is the assertion. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/ExploreTree.spec.ts | Narrows tree switcher click to a stable node container; adds justified positional disable for tag breadcrumb selection. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/ExplorePageRightPanel.spec.ts | Converts “first visible” waits to counts; adds justified positional disables for representative picks. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/ExploreBrowse.spec.ts | Scopes breadcrumb collapse button to a specific result card. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Entity.spec.ts | Replaces positional existence checks with count assertions; adds justified disables where UI has indistinguishable prev/next buttons, etc. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/DomainUIInteractions.spec.ts | Removes .first() in many places; keeps justified positional selection where the UI reuses ids/testids. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Domains.spec.ts | Scopes combobox selection and removes .nth(2) grid-cell clicks in domain tree view test. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/DomainDataProductsRightPanel.spec.ts | Removes .first() from right-panel link locator. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/DomainAdvanced.spec.ts | Converts waits to response-first ordering; removes .first() where safe. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/DataProducts.spec.ts | Converts .first()-based card existence check to count assertion; removes .first() on listbox hidden wait. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/DataProductAndSubdomains.spec.ts | Moves response listeners before action; adds justified positional disables where duplicate rendering is known. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/DataMarketplaceAnnouncements.spec.ts | Replaces .first().waitFor() with a count assertion. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/DataMarketplace.spec.ts | Adds justified positional disables for nondeterministic “recent items” widgets. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/DataInsight.spec.ts | Registers waitForResponse before sidebar navigation. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/DataContracts.spec.ts | Scopes query-builder selectors to active card and reduces global .nth() reliance; keeps justified positional for append-last semantics. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/DataContractInheritance.spec.ts | Keeps “any owner” selection as justified .first(); reduces .nth(0) usage. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/CustomProperties.spec.ts | Keeps justified positional disable for CodeMirror line placement; removes unnecessary .nth(0). |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/CSVImportWithQuotesAndCommas.spec.ts | Removes .last() from dropdown container selection and simplifies gridcell assertions. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/ClassificationConditionalRendering.spec.ts | Converts .first() visibility checks to count assertions. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/AuditLogs.spec.ts | Scopes dropdown option selection; converts skeleton waits to count assertions; adds justified positional disables where selection is intentionally arbitrary. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/AppRunsHistoryLogs.spec.ts | Removes .first() from logs button locators. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Table.spec.ts | Moves response listeners before save click. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/SSOConfiguration.spec.ts | Moves roles search listener before typing filters. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/LargeGlossaryPerformance.spec.ts | Moves glossary term search listeners before typing/clearing. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/EntitySummaryPanel.spec.ts | Adds response listener before navigating to Explore. |
| openmetadata-ui/src/main/resources/ui/package.json | Adds scripts for suppressions ratchet lint, rule tests, generated handbook table, and flake report. |
| openmetadata-ui/src/main/resources/ui/eslint.config.mjs | Wires om-playwright plugin and promotes Playwright rules to error under suppressions ratchet. |
| openmetadata-ui/src/main/resources/ui/.prettierignore | Ignores eslint-suppressions.json to avoid formatter churn vs ESLint’s format. |
| .github/workflows/ui-checkstyle.yml | Adds full-corpus Playwright guardrails job (rule tests + lint + stale-table check) and bypass guards. |
| .github/workflows/playwright-postgresql-e2e.yml | Uploads aggregated flake report artifact per shard. |
| .claude/rules/frontend-playwright.md | Updates Playwright constraints doc to reflect the single lint gate and new enforced rules. |
Suppressed comments (1)
openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/TasksUIFlow.spec.ts:141
- Same strict-mode issue as above:
page.locator('[data-testid="task-feed-card"]')can match multiple cards, andclick()will throw. If the rejection flow is meant to open the first available task, keep.first()with a justified disable, or otherwise uniquely scope the card locator.
Comment on lines
124
to
129
| const resolveTaskWithApproval = async (page: Page) => { | ||
| // Click on the first task card to open it | ||
| const taskCard = page.locator('[data-testid="task-feed-card"]').first(); | ||
| const taskCard = page.locator('[data-testid="task-feed-card"]'); | ||
| if (await taskCard.isVisible()) { | ||
| await taskCard.click(); | ||
| await waitForPageLoaded(page); |
Comment on lines
+21
to
+36
| export const clickAndWaitFor = async ( | ||
| page: Page, | ||
| locator: Locator, | ||
| urlPattern: string | RegExp, | ||
| expectedStatus = 200 | ||
| ): Promise<Response> => { | ||
| const responsePromise = page.waitForResponse(urlPattern); | ||
| await locator.click(); | ||
| const response = await responsePromise; | ||
|
|
||
| if (response.status() !== expectedStatus) { | ||
| throw new Error( | ||
| `Expected ${String( | ||
| urlPattern | ||
| )} to return ${expectedStatus}, got ${response.status()}` | ||
| ); |
Comment on lines
825
to
828
| ### ESLint | ||
| - [ ] `yarn lint:playwright` passes with zero errors | ||
| - [ ] `yarn lint:playwright:suppressions` passes with zero errors | ||
| - [ ] No new warnings introduced (fix existing ones when touching a file) | ||
|
|
Contributor
Jest test CoverageUI tests summary
|
CodeQL flagged the separate guardrail step as a high-severity cache-poisoning risk: it executes PR-authored code (the eslint config and the local rule plugin) in a pull_request_target job with a privileged checkout. The file already documents the convention - every `run:` executing PR code in this job is a poisonable step, so a separate step adds one more for no gain in isolation. Folding the guardrail commands into the existing playwright step removes the new alert without weakening the gate. The checks now run before --fix/--write so they judge what the contributor actually wrote, and the changed-files matcher is widened to eslint.config.mjs and eslint-suppressions.json so a config-only change still triggers the gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tes)
Suppressions 1446 -> 1146; no-positional-locator 1265 -> 953.
Notable root cause resolved rather than worked around: in the query-builder
specs, `.group` matches two elements per instance (RuleGroup wraps itself in
Draggable("group rule_group")) and ExpandableCard never unmounts saved
semantics, so global counts grew as a test added more. Scoping to
`.expanded-active-card .rule_group` removes the need for page-wide .nth().
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ShaileshParmar11
force-pushed
the
claude/playwright-positional-pages
branch
from
August 5, 2026 12:18
9f1490c to
3ab6799
Compare
14 tasks
|
Contributor
Author
|
Closing until the 2.0 release is out — parking this rather than abandoning it. Will reopen once the release lands. State when closed: ready for review. The branch
On reopen, in this order:
Note these sibling PRs all touch |
Code Review ✅ Approved 1 resolved / 1 findingsBurns down 312 positional locator usages across 48 Playwright e2e page files by replacing them with scoped selectors, identity attributes, and count assertions, reducing the suppression ceiling to 1,146. No issues found. ✅ 1 resolved✅ Quality: DataContractsSemanticRules left on unscoped .group unlike DataContracts
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
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
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.
Describe your changes:
PR 3 of a series. Burns down
om-playwright/no-positional-locatoracrossplaywright/e2e/Pages/.Positional locators (
.first(),.last(),.nth()) are called "not recommended" by Playwright's own docs — when the page changes, Playwright may act on an element you did not intend.312 sites across 48 files. Suppressions 1,446 → 1,146.
Type of change:
High-level design:
Four conversion patterns, in priority order:
.filter({ hasText }),getByRole(role, { name }), a specificdata-testid, orgetRowByName()fromscopedLocators.ts.loc.first().waitFor({ state: 'detached' })is exactlyexpect(loc).toHaveCount(0); existence checks becameexpect(loc).not.toHaveCount(0).eslint-disable— last resort, only where position genuinely is the semantic. Each carries a-- <why>justification, enforced aterrorby thejustified-rule-disablerule.The most substantial finding is in the query-builder specs. 54 sites in
DataContractsSemanticRules.spec.tsplus a cluster inDataContracts.spec.tsall tracked rows with page-wide.nth(). Reading the@react-awesome-query-buildersource showed why:.groupmatches two elements per instance, becauseRuleGroupwraps itself inDraggable("group rule_group")— andExpandableCardnever unmounts saved semantics, so the global.group/.rulecount kept growing as a test added more. Scoping to.expanded-active-card .rule_group(a class the component already sets on the single card being edited) isolates the active tree and removes the need for positional tracking entirely.A second recurring fix: for rows with non-deterministic ids, capture the row's real
data-testidvia.getAttribute()while it is still uniquely resolvable, then address it by identity.data-testidis a reflected HTML attribute; a controlled input'svalueis a DOM property no selector can see — which is what the original "position is the only stable identifier" justifications missed.The corpus meta-test's suppression ceiling ratchets from 1,446 to 1,146.
Tests:
Use cases covered
.first()are now count assertions, which cannot select a wrong element at allUnit tests
yarn test:eslint-rules(7/7) still passes, including the ratcheted ceiling.Backend integration tests
Ingestion integration tests
Playwright (UI) tests
Manual testing performed
yarn lint:playwright:full→ exit 0yarn test:eslint-rules→ 7/7yarn tsc:playwright→ 163 errors, unchanged from the base branch, zeroTS2304--report-unused-disable-directivescorpus-wide → zero orphaned directives--checkon all 48 changed files → cleanUI screen recording / screenshots:
Not applicable — no application UI changes.
Checklist:
Fixes <issue-number>: <short explanation>— no issue linked yetFixes #<issue-number>above — outstandingMerge order
This PR and its siblings all modify
eslint-suppressions.json, so they can be reviewed in parallel but must merge sequentially, each rebasing and re-running the full-corpus prune after the previous lands.🤖 Generated with Claude Code