CONSOLE-5414: Migrate dev-console Cypress tests to Playwright (batch 2)#16741
Open
shahsahil264 wants to merge 6 commits into
Open
CONSOLE-5414: Migrate dev-console Cypress tests to Playwright (batch 2)#16741shahsahil264 wants to merge 6 commits into
shahsahil264 wants to merge 6 commits into
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@shahsahil264: This pull request references CONSOLE-5414 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: shahsahil264 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPlaywright page objects and E2E specs were added for several dev-console flows, and matching legacy Cypress/Cucumber feature files, page objects, and step definitions were removed. ChangesDev-console Playwright migration
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/e2e/tests/dev-console/pinned-resources.spec.ts (1)
15-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid reusing
QuickStartsPagefor perspective switching in a pinned-resources test.
QuickStartsPageis semantically unrelated to pinned resources. Using it here (and naming the instancenav) creates confusion about the test's dependencies and couples this spec to an unrelated page object. Consider extractingswitchPerspectiveinto a shared navigation helper orBasePageso each spec imports the appropriate abstraction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/e2e/tests/dev-console/pinned-resources.spec.ts` around lines 15 - 16, The pinned-resources spec is using QuickStartsPage solely to call switchPerspective, which couples the test to an unrelated page object and makes the dependency unclear. Move switchPerspective into a shared navigation abstraction such as BasePage or a dedicated helper, then update pinned-resources.spec.ts to use that shared helper instead of instantiating QuickStartsPage; keep the test-specific page object aligned with pinned resources.frontend/e2e/tests/dev-console/filter-quick-starts.spec.ts (1)
24-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid duplicating locator strings between spec and page object.
Lines 24–26 and 31 re-create locators that already exist as private fields in
QuickStartsPage(filterInputandstatusFilterToggle). If the placeholder text or test ID changes, it must be updated in both places. Expose getter methods on the page object so the spec can assert visibility without duplicating locator strings.♻️ Proposed refactor
Add getter methods to
QuickStartsPage:getQuickStartCard(name: string): Locator { return this.page.getByTestId(`${name}-catalog-tile`); } + getFilterInput(): Locator { + return this.filterInput; + } + + getStatusFilterToggle(): Locator { + return this.statusFilterToggle; + } + getPageTitle(): Locator {Then use them in the spec:
await test.step('Verify keyword filter is visible', async () => { - await expect( - quickStartsPage.page.getByPlaceholder('Filter by keyword...'), - ).toBeVisible(); + await expect(quickStartsPage.getFilterInput()).toBeVisible(); }); await test.step('Verify status filter is visible', async () => { - await expect( - quickStartsPage.page.getByTestId('quick-start-status-filter'), - ).toBeVisible(); + await expect(quickStartsPage.getStatusFilterToggle()).toBeVisible(); });Also applies to: 31-31
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/e2e/tests/dev-console/filter-quick-starts.spec.ts` around lines 24 - 26, The spec is duplicating locator strings that already exist in QuickStartsPage, which makes the test brittle when the UI changes. Add public getter methods on QuickStartsPage for the existing private locators filterInput and statusFilterToggle, then update the filter-quick-starts spec to use those getters for visibility assertions instead of re-creating the locators directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/e2e/tests/dev-console/user-preferences.spec.ts`:
- Around line 33-59: The user preference tests leave server-side state behind if
a test fails before its final cleanup step, so the reset is not guaranteed. Add
an unconditional afterEach cleanup in the same spec suite (or a shared helper
used by the suite) that resets every preference modified by the affected cases,
and keep the existing test steps focused on the behavior assertions in the
userPrefs-driven tests like TC02, TC05, TC08, and TC12.
---
Nitpick comments:
In `@frontend/e2e/tests/dev-console/filter-quick-starts.spec.ts`:
- Around line 24-26: The spec is duplicating locator strings that already exist
in QuickStartsPage, which makes the test brittle when the UI changes. Add public
getter methods on QuickStartsPage for the existing private locators filterInput
and statusFilterToggle, then update the filter-quick-starts spec to use those
getters for visibility assertions instead of re-creating the locators directly.
In `@frontend/e2e/tests/dev-console/pinned-resources.spec.ts`:
- Around line 15-16: The pinned-resources spec is using QuickStartsPage solely
to call switchPerspective, which couples the test to an unrelated page object
and makes the dependency unclear. Move switchPerspective into a shared
navigation abstraction such as BasePage or a dedicated helper, then update
pinned-resources.spec.ts to use that shared helper instead of instantiating
QuickStartsPage; keep the test-specific page object aligned with pinned
resources.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cc81c02a-8356-4db1-8a6c-bf88c2aad451
📒 Files selected for processing (24)
frontend/e2e/pages/dev-console/add-page.tsfrontend/e2e/pages/dev-console/build-config-page.tsfrontend/e2e/pages/dev-console/quick-starts-page.tsfrontend/e2e/pages/dev-console/user-preferences-page.tsfrontend/e2e/tests/dev-console/build-config.spec.tsfrontend/e2e/tests/dev-console/filter-quick-starts.spec.tsfrontend/e2e/tests/dev-console/pinned-resources.spec.tsfrontend/e2e/tests/dev-console/quick-starts.spec.tsfrontend/e2e/tests/dev-console/sample-app.spec.tsfrontend/e2e/tests/dev-console/user-preferences.spec.tsfrontend/packages/dev-console/integration-tests/features/build-config/edit-build-config.featurefrontend/packages/dev-console/integration-tests/features/getting-started/sample-card-add-page.featurefrontend/packages/dev-console/integration-tests/features/guided-tour/quick-starts/filter-quick-starts-catalog.featurefrontend/packages/dev-console/integration-tests/features/guided-tour/quick-starts/quick-start-devperspective.featurefrontend/packages/dev-console/integration-tests/features/perspectives/configure-pinned-resources.featurefrontend/packages/dev-console/integration-tests/features/user-preferences/user-preferences-dev-perspective.featurefrontend/packages/dev-console/integration-tests/support/pageObjects/buildConfig-po.tsfrontend/packages/dev-console/integration-tests/support/pageObjects/gettingStarted-po.tsfrontend/packages/dev-console/integration-tests/support/step-definitions/build-config/edit-build-config.tsfrontend/packages/dev-console/integration-tests/support/step-definitions/consoleConfiguration/pinned-resources.tsfrontend/packages/dev-console/integration-tests/support/step-definitions/getting-started/sample-card-add-page.tsfrontend/packages/dev-console/integration-tests/support/step-definitions/quck-starts/filter-quick-starts-catalog.tsfrontend/packages/dev-console/integration-tests/support/step-definitions/quck-starts/quick-start-devperspective.tsfrontend/packages/dev-console/integration-tests/support/step-definitions/userPreferences/user-preferences-dev-perspective.ts
💤 Files with no reviewable changes (14)
- frontend/packages/dev-console/integration-tests/support/step-definitions/consoleConfiguration/pinned-resources.ts
- frontend/packages/dev-console/integration-tests/features/perspectives/configure-pinned-resources.feature
- frontend/packages/dev-console/integration-tests/features/build-config/edit-build-config.feature
- frontend/packages/dev-console/integration-tests/support/pageObjects/buildConfig-po.ts
- frontend/packages/dev-console/integration-tests/support/step-definitions/build-config/edit-build-config.ts
- frontend/packages/dev-console/integration-tests/features/guided-tour/quick-starts/filter-quick-starts-catalog.feature
- frontend/packages/dev-console/integration-tests/support/step-definitions/getting-started/sample-card-add-page.ts
- frontend/packages/dev-console/integration-tests/features/getting-started/sample-card-add-page.feature
- frontend/packages/dev-console/integration-tests/support/step-definitions/quck-starts/quick-start-devperspective.ts
- frontend/packages/dev-console/integration-tests/support/pageObjects/gettingStarted-po.ts
- frontend/packages/dev-console/integration-tests/features/guided-tour/quick-starts/quick-start-devperspective.feature
- frontend/packages/dev-console/integration-tests/support/step-definitions/userPreferences/user-preferences-dev-perspective.ts
- frontend/packages/dev-console/integration-tests/features/user-preferences/user-preferences-dev-perspective.feature
- frontend/packages/dev-console/integration-tests/support/step-definitions/quck-starts/filter-quick-starts-catalog.ts
shahsahil264
force-pushed
the
CONSOLE-5414
branch
2 times, most recently
from
992a64b to
b3bbbba
Compare
rhamilto
reviewed
Jul 8, 2026
rhamilto
reviewed
Jul 8, 2026
rhamilto
reviewed
Jul 8, 2026
rhamilto
reviewed
Jul 8, 2026
rhamilto
reviewed
Jul 8, 2026
Contributor
Author
|
Still working on this PR, haven't tested it locally |
Migrate 6 dev-console feature areas from Cypress to Playwright: - Quick starts filter catalog (4 tests) - Pinned resources (1 smoke test) - Sample app from Add page (3 tests) - User preferences (5 tests, skip 6 @broken-test + 1 @Manual) - Quick starts dev perspective (3 tests) - Build config edit (2 tests, skip manual + YAML-fixture-dependent) Create 4 page objects: QuickStartsPage, AddPage, UserPreferencesPage, BuildConfigPage. Remove 14 migrated Cypress files (6 features, 6 step definitions, 2 page objects). Shared files (quickStarts-po.ts, userPreference-po.ts) still imported by other features are preserved. Tests: 18 total. Guaranteed preference cleanup via afterEach. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
shahsahil264
force-pushed
the
CONSOLE-5414
branch
from
b3bbbba to
eade961
Compare
Member
|
/test frontend |
- warmupSPA: use perspective-switcher-toggle instead of page-heading (Topology view doesn't have page-heading) - BuildConfig: fix section heading "Image Configuration" → "Images", "Git repository URL" → "Git Repo URL" - User preferences: use data-test field pattern for dropdown selectors, fix preference IDs (topology.preferredView, devconsole.preferredResource) - Topology graph: use data-test-id selector (not data-test) - YAML preference test: use BuildConfig create form (has synced editor) instead of ConfigMap create (no form/YAML toggle in OCP 5.0) All 21 batch 2 tests now pass on OCP 5.0 CI cluster. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/e2e/tests/dev-console/quick-starts.spec.ts (1)
45-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
getByTestId('quickstart drawer')locator across steps.The
drawerlocator is re-created identically in two separatetest.stepblocks (Line 46 and Line 51). Hoisting it once at the top of the test avoids the duplication.♻️ Proposed refactor
async ({ page }) => { await warmupSPA(page); const quickStarts = new QuickStartsPage(page); + const drawer = page.getByTestId('quickstart drawer'); await test.step('Navigate to quick starts catalog with quickstart query parameter', async () => { await page.goto('/quickstart?quickstart=sample-application'); await page.waitForLoadState('domcontentloaded'); }); await test.step('Verify the quick start sidebar/drawer opens', async () => { - const drawer = page.getByTestId('quickstart drawer'); await expect(drawer).toBeVisible({ timeout: 30_000 }); }); await test.step('Verify the correct quick start is shown', async () => { - const drawer = page.getByTestId('quickstart drawer'); await expect(drawer).toContainText('sample application', { ignoreCase: true }); }); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/e2e/tests/dev-console/quick-starts.spec.ts` around lines 45 - 53, The quick start drawer locator is duplicated across the two test steps. Hoist the shared page.getByTestId('quickstart drawer') locator in quick-starts.spec.ts to a single variable before the test.step blocks, then reuse that same drawer reference in both the visibility and content assertions to keep the test DRY.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/e2e/tests/dev-console/quick-starts.spec.ts`:
- Around line 45-53: The quick start drawer locator is duplicated across the two
test steps. Hoist the shared page.getByTestId('quickstart drawer') locator in
quick-starts.spec.ts to a single variable before the test.step blocks, then
reuse that same drawer reference in both the visibility and content assertions
to keep the test DRY.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2562a92b-2cf6-4443-8c17-d3fa443e08a8
📒 Files selected for processing (8)
frontend/e2e/pages/base-page.tsfrontend/e2e/pages/dev-console/build-config-page.tsfrontend/e2e/pages/dev-console/quick-starts-page.tsfrontend/e2e/pages/dev-console/user-preferences-page.tsfrontend/e2e/tests/dev-console/build-config.spec.tsfrontend/e2e/tests/dev-console/filter-quick-starts.spec.tsfrontend/e2e/tests/dev-console/quick-starts.spec.tsfrontend/e2e/tests/dev-console/user-preferences.spec.ts
✅ Files skipped from review due to trivial changes (1)
- frontend/e2e/pages/base-page.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/e2e/pages/dev-console/user-preferences-page.ts
- frontend/e2e/pages/dev-console/build-config-page.ts
- frontend/e2e/pages/dev-console/quick-starts-page.ts
- frontend/e2e/tests/dev-console/user-preferences.spec.ts
- frontend/e2e/tests/dev-console/build-config.spec.ts
Member
|
@shahsahil264, it looks like Claude introduced some regressions regarding selectors where it switched to |
- warmupSPA/switchPerspective: use getByTestId() instead of data-test-id CSS selector - Move inline locators from spec files to page objects per migration rules - Document PF topology data-test-id exception and Formik ID TODO Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…page Migration rules require importing test/expect from e2e/fixtures, not directly from @playwright/test. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- BuildConfig name field: use getByRole('textbox', { name: 'Name' })
instead of Formik-generated #form-input-formData-name-field
- Quick start cards: document #id as PF package exception
(@patternfly/quickstarts has no data-test on cards)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
getByRole('textbox', { name: 'Name' }) matches both the BC name field
and the env var Name input. Scope to getByTestId('section name') first.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
/test backend |
Contributor
Author
|
/retest |
2 similar comments
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
Contributor
|
@shahsahil264: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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
3 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.
Summary
QuickStartsPage,AddPage,UserPreferencesPage,BuildConfigPageFeatures migrated
filter-quick-starts.spec.tspinned-resources.spec.tssample-app.spec.tsuser-preferences.spec.tsquick-starts.spec.tsbuild-config.spec.tsSkipped scenarios (with reasons)
@broken-testuser preference scenarios@manualuser preference scenario@broken-testquick starts filter scenarioTest plan
npx tsc --noEmit -p e2e/tsconfig.jsonpassesyarn eslintpasses on all new filesnpx playwright test --project=dev-consolepasses on OCP 5.0 clusterScreenshots / screen recording
N/A — test infrastructure only, no UI changes
🤖 Generated with Claude Code
Summary by CodeRabbit