[CONSOLE-5243] Migrate topology-ci.feature from Cypress to Playwright#16589
[CONSOLE-5243] Migrate topology-ci.feature from Cypress to Playwright#16589trgeiger wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR introduces comprehensive E2E test infrastructure for the topology UI, migrating from Cypress to Playwright. It adds page-objects, test attributes to components for reliable element targeting, and a multi-scenario test suite covering empty state, application building, workload editing, display options, and deletion workflows with namespace lifecycle management. ChangesTopology E2E Test Suite
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: trgeiger 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 |
- Convert Gherkin scenarios to idiomatic Playwright test structure - Add data-test attribute to git URL input field in GitSection.tsx - Implement test scenarios: empty state, app creation, editing groupings, display options, workload deletion - Create TopologyPage and TopologySidebarPage objects for Topology Playwright tests - Mock GitHub API requests for .NET sample repository - Follow Console's layered test architecture with proper cleanup - Delete old Cypress topology CI tests Tests migrated: 5 scenarios → 5 test cases
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/pages/topology-page.ts`:
- Around line 121-123: The method typeInQuickSearch doesn't await the Playwright
fill action, causing callers to proceed before typing finishes; update the
function to await the fill call by changing the invocation to await
this.page.getByLabel('Quick search bar').fill(text) so the Promise returned by
typeInQuickSearch resolves only after the fill completes (retain the existing
async typeInQuickSearch(text: string): Promise<void> signature).
- Around line 91-95: The clickOnNode function currently searches then grabs
this.page.getByTestId('base-node-handler').first(), which ignores nodeName and
can click the wrong node; update clickOnNode to scope the locator to the
specific nodeName (e.g., use a locator that filters handlers by the nodeName
text or an attribute after calling this.search(nodeName)) and then call
this.robustClick on that scoped locator (instead of .first()) so the clicked
handler corresponds to the requested nodeName; keep references to the existing
methods clickOnNode, search, and robustClick while replacing the unscoped
this.page.getByTestId('base-node-handler').first() with a locator that matches
nodeName.
In `@frontend/e2e/tests/topology/topology-ci.spec.ts`:
- Around line 50-170: Current tests share a workload created in the first test
("Build the application from topology page") and then rely on it in later tests,
causing cascading failures; move that provisioning into an explicit suite-level
setup/teardown or have each test create and clean its own workload. Concretely,
extract the steps that create the workload (calls to
TopologyPage.clickStartBuilding, fill Git Repo URL, form fills,
TopologyPage.verifyWorkloadVisible) into a beforeAll that creates the "dotnet"
workload and an afterAll that deletes it (use TopologyPage.clickOnNode +
TopologySidebarPage.selectAction('Delete Deployment') +
TopologyPage.clickConfirmAction), or alternatively wrap those creation and
deletion steps inside each dependent test so they are self-contained; ensure
tests that currently call navigateToTopologyGraph, verifyWorkloadVisible,
rightClickOnNode, selectContextMenuAction, clickDisplayOptions, getExpandToggle,
and getDisplayOptionCheckbox work against the explicitly provisioned workload.
🪄 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: a4665709-60e9-4839-bb6f-aeacfd55fddd
📒 Files selected for processing (7)
frontend/e2e/pages/topology-page.tsfrontend/e2e/pages/topology-sidebar-page.tsfrontend/e2e/tests/topology/topology-ci.spec.tsfrontend/packages/dev-console/src/components/import/ImportSampleForm.tsxfrontend/packages/dev-console/src/components/import/app/AppSection.tsxfrontend/packages/dev-console/src/components/import/git/GitSection.tsxfrontend/packages/topology/src/components/dropdowns/ApplicationSelector.tsx
| async clickOnNode(nodeName: string): Promise<void> { | ||
| await this.search(nodeName); | ||
| const node = this.page.getByTestId('base-node-handler'); | ||
| await this.robustClick(node.first()); | ||
| } |
There was a problem hiding this comment.
clickOnNode ignores the requested node and clicks the first handler.
Line 93–94 uses base-node-handler.first() instead of a locator scoped by nodeName, so this can click the wrong workload when multiple nodes are present.
Suggested fix
async clickOnNode(nodeName: string): Promise<void> {
await this.search(nodeName);
- const node = this.page.getByTestId('base-node-handler');
- await this.robustClick(node.first());
+ const node = this.page
+ .getByTestId('base-node-handler')
+ .filter({ has: this.getNode(nodeName) })
+ .first();
+ await expect(node).toBeVisible({ timeout: 30_000 });
+ await this.robustClick(node);
}🤖 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/pages/topology-page.ts` around lines 91 - 95, The clickOnNode
function currently searches then grabs
this.page.getByTestId('base-node-handler').first(), which ignores nodeName and
can click the wrong node; update clickOnNode to scope the locator to the
specific nodeName (e.g., use a locator that filters handlers by the nodeName
text or an attribute after calling this.search(nodeName)) and then call
this.robustClick on that scoped locator (instead of .first()) so the clicked
handler corresponds to the requested nodeName; keep references to the existing
methods clickOnNode, search, and robustClick while replacing the unscoped
this.page.getByTestId('base-node-handler').first() with a locator that matches
nodeName.
| async typeInQuickSearch(text: string): Promise<void> { | ||
| this.page.getByLabel('Quick search bar').fill(text); | ||
| } |
There was a problem hiding this comment.
Await the quick-search fill action.
typeInQuickSearch does not await fill, so callers can continue before typing completes.
Suggested fix
async typeInQuickSearch(text: string): Promise<void> {
- this.page.getByLabel('Quick search bar').fill(text);
+ await this.page.getByLabel('Quick search bar').fill(text);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async typeInQuickSearch(text: string): Promise<void> { | |
| this.page.getByLabel('Quick search bar').fill(text); | |
| } | |
| async typeInQuickSearch(text: string): Promise<void> { | |
| await this.page.getByLabel('Quick search bar').fill(text); | |
| } |
🤖 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/pages/topology-page.ts` around lines 121 - 123, The method
typeInQuickSearch doesn't await the Playwright fill action, causing callers to
proceed before typing finishes; update the function to await the fill call by
changing the invocation to await this.page.getByLabel('Quick search
bar').fill(text) so the Promise returned by typeInQuickSearch resolves only
after the fill completes (retain the existing async typeInQuickSearch(text:
string): Promise<void> signature).
| test('Build the application from topology page', async ({ page }) => { | ||
| const topology = new TopologyPage(page); | ||
| await page.goto('/'); | ||
| await topology.switchPerspective('Administrator'); | ||
| await topology.navigateToTopologyGraph(NS); | ||
|
|
||
| await test.step('Open quick search and select .NET builder image', async () => { | ||
| await topology.clickStartBuilding(); | ||
| await topology.typeInQuickSearch('.NET'); | ||
| await page.getByTestId('item-name-.NET SDK-Builder Images').first().click(); | ||
| }); | ||
|
|
||
| await test.step('Create application from quick search', async () => { | ||
| await expect(page.getByRole('progressbar')).not.toBeAttached({ timeout: 60_000 }); | ||
| await page | ||
| .getByRole('listitem') | ||
| .filter({ hasText: /^\.NET SDK.*Builder Images$/ }) | ||
| .first() | ||
| .click(); | ||
| await page.getByRole('button', { name: 'Create' }).click(); | ||
| }); | ||
|
|
||
| await test.step('Mock GitHub API and fill git repo URL', async () => { | ||
| const apiBase = 'https://api.github.com/repos/redhat-developer/s2i-dotnetcore-ex'; | ||
| await page.route(new RegExp(`${apiBase.replace(/\//g, '\\/')}(\\?.*)?$`), async (route) => { | ||
| await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(repoMock) }); | ||
| }); | ||
| await page.route(new RegExp(`${apiBase.replace(/\//g, '\\/')}\/contents\\/?(\\?.*)?$`), async (route) => { | ||
| await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(contentsMock) }); | ||
| }); | ||
| await page.route(new RegExp(`${apiBase.replace(/\//g, '\\/')}\/.*package\\.json`), async (route) => { | ||
| await route.fulfill({ status: 404 }); | ||
| }); | ||
| await page.route(/\/api\/devfile\//, async (route) => { | ||
| await route.fulfill({ status: 404 }); | ||
| }); | ||
| await page.route(new RegExp(`${apiBase.replace(/\//g, '\\/')}\/.*func\\.yaml`), async (route) => { | ||
| await route.fulfill({ status: 404 }); | ||
| }); | ||
|
|
||
| await page.getByLabel('Git Repo URL').clear(); | ||
| await page.getByLabel('Git Repo URL').fill('https://github.com/redhat-developer/s2i-dotnetcore-ex'); | ||
| await expect(page.locator('#form-input-git-url-field-helper')).toContainText('Validated', { | ||
| timeout: 120_000, | ||
| }); | ||
| }); | ||
|
|
||
| await test.step('Enter application and workload names', async () => { | ||
| await page.locator('#form-input-application-name-field').fill('dotnet-app'); | ||
| await page.locator('#form-input-name-field').fill('dotnet'); | ||
| }); | ||
|
|
||
| await test.step('Select Deployment resource type and submit', async () => { | ||
| await page.locator('#form-select-input-resources-field').scrollIntoViewIfNeeded(); | ||
| await page.locator('#form-select-input-resources-field').click(); | ||
| await page.locator('#select-option-resources-kubernetes').click(); | ||
| await page.getByTestId('save-changes').click(); | ||
| }); | ||
|
|
||
| await test.step('Verify workload appears in topology', async () => { | ||
| await topology.verifyWorkloadVisible('dotnet', 60_000); | ||
| }); | ||
| }); | ||
|
|
||
| test('Edit workload application groupings: T-09-TC01', async ({ page }) => { | ||
| const topology = new TopologyPage(page); | ||
| await page.goto('/'); | ||
| await topology.switchPerspective('Administrator'); | ||
| await topology.navigateToTopologyGraph(NS); | ||
|
|
||
| await test.step('Right-click dotnet and select Edit', async () => { | ||
| await topology.rightClickOnNode('dotnet'); | ||
| await topology.selectContextMenuAction('Edit dotnet'); | ||
| }); | ||
|
|
||
| await test.step('Change application groupings to "app"', async () => { | ||
| const appDropdown = page.locator('[id$="application-name-field"]'); | ||
| await expect(appDropdown).toBeVisible({ timeout: 30_000 }); | ||
| await appDropdown.click(); | ||
| await page.getByTestId('console-select-item').first().click(); | ||
| await page.getByTestId('application-form-app-input').fill('app'); | ||
| }); | ||
|
|
||
| await test.step('Save and verify workload persists', async () => { | ||
| await page.getByRole('button', { name: 'Save' }).click(); | ||
| await topology.navigateToTopologyGraph(NS); | ||
| await topology.verifyWorkloadVisible('dotnet', 60_000); | ||
| await expect(topology.getGraphSurface()).toBeAttached(); | ||
| }); | ||
| }); | ||
|
|
||
| test('Default state of Display dropdown: T-16-TC01', async ({ page }) => { | ||
| const topology = new TopologyPage(page); | ||
| await page.goto('/'); | ||
| await topology.switchPerspective('Administrator'); | ||
| await topology.navigateToTopologyGraph(NS); | ||
|
|
||
| await topology.clickDisplayOptions(); | ||
| await expect(topology.getExpandToggle()).toBeChecked(); | ||
| await expect(topology.getDisplayOptionCheckbox('Pod count')).not.toBeChecked(); | ||
| await expect(topology.getDisplayOptionCheckbox('Labels')).toBeChecked(); | ||
| }); | ||
|
|
||
| test('Delete workload via Action menu: T-15-TC01', async ({ page }) => { | ||
| const topology = new TopologyPage(page); | ||
| const sidebar = new TopologySidebarPage(page); | ||
| await page.goto('/'); | ||
| await topology.switchPerspective('Administrator'); | ||
| await topology.navigateToTopologyGraph(NS); | ||
|
|
||
| await test.step('Open sidebar and delete via Actions menu', async () => { | ||
| await topology.clickOnNode('dotnet'); | ||
| await sidebar.verify(); | ||
| await sidebar.selectAction('Delete Deployment'); | ||
| }); | ||
|
|
||
| await test.step('Confirm deletion and verify empty state', async () => { | ||
| await topology.clickConfirmAction(); | ||
| await expect(topology.getNoResourcesFound()).toBeVisible({ timeout: 60_000 }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Later tests depend on state created by an earlier test case.
The workload is created in one test() and then edited/deleted in subsequent test() blocks, so a single failure cascades and obscures root causes. Keep the shared-resource exception, but move shared workload provisioning to explicit suite setup (or make each test provision/cleanup its own workload).
As per coding guidelines, frontend/e2e/tests/**/*.spec.ts should keep tests self-contained; based on learnings, shared resources are acceptable when explicitly managed in suite setup/teardown.
🧰 Tools
🪛 ast-grep (0.43.0)
[warning] 73-73: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(${apiBase.replace(/\//g, '\\/')}(\\?.*)?$)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 76-76: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(${apiBase.replace(/\//g, '\\/')}\/contents\\/?(\\?.*)?$)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 79-79: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(${apiBase.replace(/\//g, '\\/')}\/.*package\\.json)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 85-85: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(${apiBase.replace(/\//g, '\\/')}\/.*func\\.yaml)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🤖 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/topology/topology-ci.spec.ts` around lines 50 - 170,
Current tests share a workload created in the first test ("Build the application
from topology page") and then rely on it in later tests, causing cascading
failures; move that provisioning into an explicit suite-level setup/teardown or
have each test create and clean its own workload. Concretely, extract the steps
that create the workload (calls to TopologyPage.clickStartBuilding, fill Git
Repo URL, form fills, TopologyPage.verifyWorkloadVisible) into a beforeAll that
creates the "dotnet" workload and an afterAll that deletes it (use
TopologyPage.clickOnNode + TopologySidebarPage.selectAction('Delete Deployment')
+ TopologyPage.clickConfirmAction), or alternatively wrap those creation and
deletion steps inside each dependent test so they are self-contained; ensure
tests that currently call navigateToTopologyGraph, verifyWorkloadVisible,
rightClickOnNode, selectContextMenuAction, clickDisplayOptions, getExpandToggle,
and getDisplayOptionCheckbox work against the explicitly provisioned workload.
Sources: Coding guidelines, Learnings
rhamilto
left a comment
rhamilto
left a comment
There was a problem hiding this comment.
Plus a few suggestions from Claude:
Bugs
1. Missing await in typeInQuickSearch — [topology-page.ts:121](vscode-webview://1vtcgfscfosoccqkvs1psrnb2o14e4t7o2f82teppc96updftt2k/frontend/e2e/pages/topology-page.ts#L121)
async typeInQuickSearch(text: string): Promise<void> {
this.page.getByLabel('Quick search bar').fill(text); // ← missing await
}
fill() returns a Promise<void> — without await, the fill races with whatever comes next. Should be:
await this.page.getByLabel('Quick search bar').fill(text);
Migration Guideline Issues
2. PR description mentions a change not in the diff. The description says "Add data-test attribute to git URL input field in GitSection.tsx" but no GitSection.tsx changes are included. The spec uses page.getByLabel('Git Repo URL') instead, which works. Either update the description to remove the stale line, or add the data-test attribute if it was intended.
3. Unused import in topology-sidebar-page.ts — [line 1](vscode-webview://1vtcgfscfosoccqkvs1psrnb2o14e4t7o2f82teppc96updftt2k/frontend/e2e/pages/topology-sidebar-page.ts#L1)
import type { Locator } from '@playwright/test'; // unused — no explicit Locator annotation
Remove the unused import; dialog and actionsDropdown types are inferred.
Suggestions (non-blocking)
4. Missing semicolons — inconsistent style — Several lines are missing trailing semicolons, which is inconsistent with the existing codebase style:
[topology-page.ts:8](vscode-webview://1vtcgfscfosoccqkvs1psrnb2o14e4t7o2f82teppc96updftt2k/frontend/e2e/pages/topology-page.ts#L8): private readonly switcher = ... (no semicolon)
[topology-page.ts:105](vscode-webview://1vtcgfscfosoccqkvs1psrnb2o14e4t7o2f82teppc96updftt2k/frontend/e2e/pages/topology-page.ts#L105): const actionButton = ... (no semicolon)
[topology-page.ts:115](vscode-webview://1vtcgfscfosoccqkvs1psrnb2o14e4t7o2f82teppc96updftt2k/frontend/e2e/pages/topology-page.ts#L115): return this.page.getByRole('menuitem', {name: label}).getByRole('checkbox'); (missing spaces in {name: label})
Also: note the console code base has a /pre-push-review skill that is helpful. Note you'll need to have the CodeRabbit CLI installed for maximum benefit.
|
|
||
| const MOCK_DIR = path.join( | ||
| path.dirname(fileURLToPath(import.meta.url)), | ||
| '../../../packages/dev-console/integration-tests/testData/git-import/s2i-dotnetcore-ex', |
There was a problem hiding this comment.
We should probably duplicate the mock data since packages/dev-console/integration-tests should eventually be deleted.
| "scripts": { | ||
| "test-cypress": "../../../node_modules/.bin/cypress open --env openshift=true", | ||
| "test-cypress-headless": "node --max-old-space-size=4096 ../../../node_modules/.bin/cypress run ${CYPRESS_RECORD_KEY:+--record} --env openshift=true --browser ${BRIDGE_E2E_BROWSER_NAME:-electron} --headless --spec \"features/*/topology-ci.feature\"", | ||
| "test-cypress-headless": "node --max-old-space-size=4096 ../../../node_modules/.bin/cypress run ${CYPRESS_RECORD_KEY:+--record} --env openshift=true --browser ${BRIDGE_E2E_BROWSER_NAME:-electron} --headless", |
There was a problem hiding this comment.
This whole file should be going away at some point. Up to you if that's in this PR or a separate one.
| await topology.navigateToTopologyGraph(NS); | ||
| await topology.verifyWorkloadVisible('dotnet', 60_000); | ||
| await expect(topology.getGraphSurface()).toBeAttached(); | ||
| }); |
There was a problem hiding this comment.
Claude noticed we're not confirming the application grouping exists:
In the "Edit workload application groupings" test (Scenario 3), the Cypress test verifies the grouping was renamed by searching for the new name "app". The Playwright test only checks that the workload "dotnet" is still visible — it never confirms the application grouping actually changed. To fix, the test should assert that the app group label "app" is visible, e.g., by searching for "app" and verifying the group node appears.
|
@trgeiger: The following tests 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. |
2 participants
Tests migrated: 5 scenarios → 5 test cases
Analysis / Root cause:
Solution description:
Screenshots / screen recording:
Test setup:
Test cases:
Browser conformance:
Additional info:
Reviewers and assignees:
Summary by CodeRabbit