fix(ui): account for system default persona fallback in Users persona removal e2e#29451
Conversation
harsh-vador
added
safe to test
❌ 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 |
harsh-vador
changed the title
Code Review ✅ ApprovedUpdates the Users persona removal E2E test to account for the system default persona fallback. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const removedDefaultPersonaLabel = adminPage | ||
| .locator('[data-testid="persona-label"]') | ||
| .filter({ hasText: persona2.responseData.displayName }); | ||
|
|
||
| // Verify default persona is first in the list | ||
| const personaLabels = adminPage.locator('[data-testid="persona-label"]'); | ||
| const firstPersona = personaLabels.first(); | ||
| await expect( | ||
| removedDefaultPersonaLabel.locator('[data-testid="default-persona-tag"]') | ||
| ).not.toBeVisible(); | ||
|
|
||
| await expect( | ||
| firstPersona.locator('[data-testid="default-persona-tag"]') | ||
| ).toBeVisible(); | ||
| removedDefaultPersonaLabel.locator('input[type="radio"]') | ||
| ).not.toBeChecked(); |
There was a problem hiding this comment.
The variable removedDefaultPersonaLabel implies persona2's default status was removed, but in this test's scope the beforeAll sets persona1 as the default — persona2 was never the default to begin with. More importantly, the test is titled "Should display default persona tag correctly" yet the new code only verifies the negative case (persona2 does not have the tag / is not checked). The positive assertion — that persona1 actually shows the default-persona-tag — has been dropped entirely. If the feature were to stop rendering the default tag altogether, this test would still pass, making it a silent regression hole.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
✅ Playwright Results — all 32 tests passed✅ 32 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 0 skipped
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
|
Changes have been cherry-picked to the 1.12.13 branch. |
|
Changes have been cherry-picked to the 1.13 branch. |
2 participants
Describe your changes:
What
Fix flaky-looking failure in
Users.spec.ts› "Should handle default persona change and removal correctly". The final step asserted that after a user removes their default persona, the profile dropdown shows no default persona tag and nochecked radio.
Type of change:
High-level design:
N/A — small change.
Tests:
Use cases covered
Unit tests
Backend integration tests
Ingestion integration tests
Playwright (UI) tests
Manual testing performed
UI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.Greptile Summary
This PR updates the "Should display default persona tag correctly" E2E test in
Users.spec.tsto fix a flaky assertion caused by the system's default persona fallback behaviour after persona removal. The old code checked that the first persona label carries thedefault-persona-tag; the new code instead filters forpersona2by display name and asserts the absence of the tag and an unchecked radio on that label.removedDefaultPersonaLabelis semantically misleading in this test because persona2 was never set as the default in thebeforeAll— the name fits the removal test (Should handle default persona change and removal correctly) better than this one.Confidence Score: 4/5
Safe to merge as a flakiness fix; the changed assertions will not cause false positives, but the test no longer verifies the positive display path for the default persona tag.
The only changed file is a Playwright spec. The new assertions are logically correct for persona2 (which is never the default in this describe block's setup), but the test drops the positive check that persona1 actually shows the
default-persona-tag. If the feature regresses and stops rendering the tag entirely, this test would still pass. The misleading variable name (removedDefaultPersonaLabel) adds confusion. Neither issue is a runtime defect — only test coverage is weakened.openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Users.spec.ts — the positive assertion for the default persona tag display should be restored alongside the new negative assertions.
Important Files Changed
removedDefaultPersonaLabelis misleading in this test's context.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant beforeAll participant Test1 as Should display persona dropdown with pagination participant Test2 as Should display default persona tag correctly (changed) participant Test3 as Should handle default persona change and removal beforeAll->>adminUser: PATCH /personas [persona1, persona2] beforeAll->>adminUser: "PATCH /defaultPersona = persona1" Test1->>adminPage: Open dropdown, verify pagination Test2->>adminPage: Open dropdown, expand all Note over Test2: OLD: assert persona1 (first) has default-persona-tag Note over Test2: NEW: assert persona2 has NO default-persona-tag (negative only) Note over Test2: NEW: assert persona2 radio is NOT checked (negative only) Test3->>adminPage: Step 1 - verify persona1 is default and checked Test3->>profilePage: Step 2 - change default persona to persona2 Test3->>adminPage: Step 3 - verify persona2 is now default and checked Test3->>profilePage: Step 4 - remove default persona entirely Test3->>adminPage: Step 5 - verify no tag and no checked radio anywhere%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant beforeAll participant Test1 as Should display persona dropdown with pagination participant Test2 as Should display default persona tag correctly (changed) participant Test3 as Should handle default persona change and removal beforeAll->>adminUser: PATCH /personas [persona1, persona2] beforeAll->>adminUser: "PATCH /defaultPersona = persona1" Test1->>adminPage: Open dropdown, verify pagination Test2->>adminPage: Open dropdown, expand all Note over Test2: OLD: assert persona1 (first) has default-persona-tag Note over Test2: NEW: assert persona2 has NO default-persona-tag (negative only) Note over Test2: NEW: assert persona2 radio is NOT checked (negative only) Test3->>adminPage: Step 1 - verify persona1 is default and checked Test3->>profilePage: Step 2 - change default persona to persona2 Test3->>adminPage: Step 3 - verify persona2 is now default and checked Test3->>profilePage: Step 4 - remove default persona entirely Test3->>adminPage: Step 5 - verify no tag and no checked radio anywhereReviews (1): Last reviewed commit: "fix(ui): account for system default pers..." | Re-trigger Greptile