refactor(playwright): sign in through a fixture, not the form - #33219
Open
chirag-madlani wants to merge 2 commits into
Open
refactor(playwright): sign in through a fixture, not the form#33219chirag-madlani wants to merge 2 commits into
chirag-madlani wants to merge 2 commits into
Conversation
Migrates every spec off `UserClass.login()` and promotes `openmetadata-playwright/prefer-role-page-fixture` to `error` with no suppressions. 275 call sites across 83 files become `signIn()`. Both paths funnel through the same `completeSignIn()`, so the rewrite changes how the session was established and nothing else — which the type checker confirms: a receiver that was not a `UserClass` would not compile, and the playwright tsc count is unchanged. Nine specs keep `login()` because the sign-in flow is what they test — `Pages/Login.spec.ts`, `Features/OnlineUsers.spec.ts`, `Flow/Tour.spec.ts` and the `Features/AppMode` specs. Each call carries a justified disable naming its reason, rather than a blanket entry in a baseline file. `performUserLogin` is no longer flagged: it signs in through the API and owns its page, context and teardown, so it is a sanctioned path. The rule targets *authenticating as* a bespoke user, not *creating* one — `new UserClass()` for an owner, reviewer or assignee is ordinary test data and is untouched. `eslint-suppressions.json` is byte-identical to main. The rule was written to be enforceable only once the corpus was clean, which is what this change makes true. Validated against a live server: ~320 tests across the migrated specs, the role fixtures, and the eight storage states, with no failure attributable to the migration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
chirag-madlani
requested review from
ShaileshParmar11 and
karanh37
as code owners
September 11, 2026 14:13
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
Contributor
🔄 Playwright impact map auto-refreshedThis PR touched specs or UI source that changed the source→spec routing map. I regenerated What is this file? It is the auto-generated half of Playwright's PR planner. It routes "if source X changes, run specs Y" by walking spec imports and cross-referencing What if I want to regenerate locally instead? Run this before pushing your next change to skip the bot commit: python3 .github/scripts/generate_playwright_impact_map.py
git add .github/playwright/impact-map.generated.json
git commit --amend --no-edit # or a separate commit |
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsRefactors 275 sign-in call sites across 83 files to use a fixture instead of the form, channeling all paths through 💡 Quality: Rule JSDoc still says it runs at
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source
Contributor
🔴 Playwright Results — workflow failedValidated commit ✅ 0 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky ⚙️ No test failures. This run failed in CI infrastructure or reporting (see Pipeline and setup failures), not because of your changes — no test action needed from you. Pipeline and setup failures (110)
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) 34m 46s ⏱️ Max setup 0s · max shard execution 0s · max shard-job elapsed before upload 4m 11s · reporting 1s 🌐 0.00 requests/attempt · 0.00 app boots/UI scenario · 0.00% common-shard skew Optimization targets still in progress:
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
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
2 participants
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.
Stack 4 of 4. Targets #33218. This is the large, mechanical one — review #33218 first, since it contains the mechanism this PR applies.
The change
275 call sites across 83 files become
signIn(), andopenmetadata-playwright/prefer-role-page-fixturegoes toerrorwith no suppressions.Both paths funnel through the same
completeSignIn()(see #33218), so the rewrite changes how the session was established and nothing else. The type checker confirms it: a receiver that was not aUserClasswould not compile, and the playwrighttsccount is unchanged by the rewrite.Exemptions are stated, not hidden
Nine specs keep
login()because the sign-in flow is what they test —Pages/Login.spec.ts,Features/OnlineUsers.spec.ts,Flow/Tour.spec.tsand theFeatures/AppModespecs. Each call carries a justified disable naming its reason, rather than a blanket entry in a baseline file.performUserLoginis deliberately not flagged: it signs in through the API and owns its page, context and teardown, so it is a sanctioned path.The rule targets authenticating as a bespoke user, not creating one —
new UserClass()for an owner, reviewer or assignee is ordinary test data and is untouched. That distinction is what keeps the rule at zero false positives.eslint-suppressions.jsonis byte-identical tomain. The rule was written to be enforceable only once the corpus was clean, which is what this change makes true.The rule already works
While rebasing onto a
mainthat had moved 62 commits, the rule caught a brand-newnonAsciiUser.login(page)that arrived inLogin.spec.ts— failing lint on contact, which is precisely its job.Verification (live server)
~320 tests across the migrated specs, the role fixtures and the eight storage states, with no failure attributable to the migration:
TaskComments+Users+EntityPermissionsRoleFixtureSessions+TokenStorage+TaskComments+TasksUIFlowon the rebased tipCoverage caveat, stated plainly: 113 of 272 migrated call sites (42%) across 12 of 83 files were exercised locally. The rest rests on the transform being mechanical and type-verified, and on
login()/signIn()sharingcompleteSignIn(). CI is the real gate — and thanks to #33217, any failure will now name the step that hung.🤖 Generated with Claude Code