fix(e2e): fix themes_management test selectors for theme name inputs#2728
fix(e2e): fix themes_management test selectors for theme name inputs#2728wwwillchen merged 1 commit intomainfrom
Conversation
Use specific element IDs (#manual-name, #ai-name) instead of ambiguous
getByLabel("Theme Name") selectors. Also scope edit dialog selectors
to the dialog context to avoid matching create form inputs.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@BugBot run |
Summary of ChangesHello @wwwillchen-bot, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness of the themes management end-to-end tests by refining the Playwright selectors used for theme name input fields. The changes ensure that tests accurately target the intended elements, particularly in scenarios involving multiple dialogs or forms with similar labels, thereby preventing flakiness and improving test stability. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with π and π on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of ambiguous selectors in the themes management E2E tests by replacing getByLabel("Theme Name") with more specific ID-based locators or by scoping the selector to a dialog. The changes improve the robustness of the tests. The suggestion to further improve conciseness and consistency of one of the updated selectors has been retained.
| await po.page.getByRole("dialog").getByLabel("Theme Name").clear(); | ||
| await po.page | ||
| .getByRole("dialog") | ||
| .getByLabel("Theme Name") | ||
| .fill("Updated Theme"); |
There was a problem hiding this comment.
The .fill() method in Playwright already clears the input field before filling it, so the .clear() call is redundant.
Also, instead of scoping by dialog and then selecting by label, you can use the unique ID #edit-theme-name for this input. This is more direct and consistent with the other selector changes in this PR (e.g., #manual-name, #ai-name).
| await po.page.getByRole("dialog").getByLabel("Theme Name").clear(); | |
| await po.page | |
| .getByRole("dialog") | |
| .getByLabel("Theme Name") | |
| .fill("Updated Theme"); | |
| await po.page.locator("#edit-theme-name").fill("Updated Theme"); |
Greptile OverviewGreptile SummaryFixed ambiguous Confidence Score: 4/5
Important Files Changed
Last reviewed commit: 92ac37c |
| .getByLabel("Description (optional)") | ||
| .fill("A test theme description"); |
There was a problem hiding this comment.
π‘ Incomplete fix: getByLabel("Description (optional)") has the same ambiguity as the fixed getByLabel("Theme Name")
The PR fixes the ambiguous getByLabel("Theme Name") selector by switching to #manual-name / #ai-name IDs because both tab panels (AI and Manual) render inputs with the same label in the Create dialog. However, the getByLabel("Description (optional)") selector was not similarly fixed, even though it has the exact same ambiguity β both AIGeneratorTab.tsx:338 (<Label htmlFor="ai-description">Description (optional)</Label>) and CustomThemeDialog.tsx:163 (<Label htmlFor="manual-description">Description (optional)</Label>) are present in the DOM simultaneously.
Root Cause
Base UI's TabsPrimitive.Panel keeps inactive tab content mounted in the DOM (with a hidden attribute) by default. This is exactly why the PR needed to fix getByLabel("Theme Name") β both the AI and Manual tab panels are in the DOM at the same time.
The same situation applies to "Description (optional)":
AIGeneratorTab.tsx:338-344:<Label htmlFor="ai-description">Description (optional)</Label>+<Input id="ai-description" ...>CustomThemeDialog.tsx:163-169:<Label htmlFor="manual-description">Description (optional)</Label>+<Input id="manual-description" ...>
Playwright's getByLabel() matches both inputs, causing a strict-mode violation when .fill() is called. This affects four locations in the test file:
- Line 32 (CRUD create, Manual tab)
- Line 130 (chat input create, Manual tab)
- Line 244 (AI generator flow, AI tab)
- Line 314 (AI generator URL flow, AI tab)
Each should use #manual-description or #ai-description respectively, matching the fix pattern used for the name inputs.
Prompt for agents
Fix the ambiguous getByLabel("Description (optional)") selectors in all four locations in e2e-tests/themes_management.spec.ts, using the same ID-based approach as the Theme Name fix:
1. Line 32: Change `po.page.getByLabel("Description (optional)")` to `po.page.locator("#manual-description")` (CRUD test, Manual tab)
2. Line 130: Change `po.page.getByLabel("Description (optional)")` to `po.page.locator("#manual-description")` (chat input test, Manual tab)
3. Line 244: Change `po.page.getByLabel("Description (optional)")` to `po.page.locator("#ai-description")` (AI generator flow, AI tab)
4. Line 314: Change `po.page.getByLabel("Description (optional)")` to `po.page.locator("#ai-description")` (AI generator URL flow, AI tab)
The Edit dialog selectors (line 63) are fine because the Create dialog is closed when the Edit dialog is open.
Was this helpful? React with π or π to provide feedback.
π Dyadbot Code Review SummaryVerdict: β YES - Ready to merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues SummaryNo HIGH or MEDIUM issues found. π’ Low Priority Notes (1 item)
π« Dropped False Positives (1 item)
Generated by Dyadbot multi-agent code review |
π Playwright Test Resultsβ Some tests failed
Summary: 231 passed, 2 failed, 9 flaky, 6 skipped Failed Testsπ macOS
π Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/github-import.spec.ts \
e2e-tests/template-create-nextjs.spec.ts
|
Summary
getByLabel("Theme Name")selectors in themes management E2E tests#manual-name,#ai-name) for create dialogs where multiple matching inputs existTest plan
npx playwright test e2e-tests/themes_management.spec.tsπ€ Generated with Claude Code