CONSOLE-5240: Migrate Helm Cypress E2E tests to Playwright#16542
Open
vikram-raj wants to merge 1 commit into
Open
CONSOLE-5240: Migrate Helm Cypress E2E tests to Playwright#16542vikram-raj wants to merge 1 commit into
vikram-raj wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@vikram-raj: This pull request references CONSOLE-5240 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 story 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
WalkthroughAdds Playwright E2E page objects and specs for Helm workflows and topology interactions, and removes legacy Cypress/Cucumber Helm integration tests, configs, test data, and CI script references. ChangesHelm Cypress to Playwright migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vikram-raj The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
component/topology
![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: 4
🧹 Nitpick comments (2)
frontend/e2e/pages/topology-page.ts (1)
76-86: 💤 Low valueRemove the dead comment and harden the right-click target selector.
Line 77 is a leftover debug artifact. More importantly, the selector on Line 83 chains several PatternFly-internal class names down to a
<path>element (.pf-v6-svg > .pf-v6-icon-default > path). These are implementation details of the PF icon and thenth(1)index assumption is brittle — both are likely to break on PF upgrades or when node ordering changes. Prefer targeting the action-icon trigger by a stable test id / role rather than its SVG internals.This was already noted as a pre-push follow-up; flagging here for visibility.
♻️ Remove dead code
async rightClickWorkload(workloadName: string): Promise<void> { - // const workloadNode = this.page await this.waitForLoadingComplete();🤖 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 76 - 86, Remove the leftover debug comment in rightClickWorkload and replace the brittle chained SVG selector and hard-coded nth(1) with a resilient locator: first locate the topology node that matches workloadName (e.g., find the node element by its visible label/text after searchTopology) and then locate the action trigger inside that node via a stable attribute or semantic selector (data-testid, aria-label, or getByRole('button')) instead of targeting .pf-v6-svg/.pf-v6-icon-default/path; keep the rest of the flow (waitForLoadingComplete, ensureGraphView, resetView/fitToScreen, scrollIntoViewIfNeeded, robustClick) but target the action-icon via the stable test id/role on the node rather than SVG internals or a fixed index.frontend/e2e/pages/topology-sidebar-page.ts (1)
62-74: 💤 Low value
namespaceparameter is unused.
verifyResourceDetailsPageacceptsnamespace(Line 64) but never references it in the body — the URL is matched viaoptions.urlPattern, which the caller already builds with the namespace. Drop the parameter (and its call-site argument intopology-helm-sidebar.spec.ts) to avoid the misleading signature.🤖 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-sidebar-page.ts` around lines 62 - 74, The verifyResourceDetailsPage function includes an unused namespace parameter; remove the namespace parameter from the verifyResourceDetailsPage signature and from all call sites (notably the call in topology-helm-sidebar.spec.ts) so the function only accepts (resource, helmRelease, options); update any TypeScript types or references to the old signature accordingly (search for verifyResourceDetailsPage and adjust invocations to pass only the required arguments), and run tests to ensure no remaining references to the removed parameter remain.
🤖 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/helm-details-page.ts`:
- Around line 47-56: The test verifyActionsInActionMenu currently only asserts
that each rendered item is in the allowed list; change it to explicitly assert
the expected actions are present and no extras: collect the text values from the
locator this.page.locator('[data-test-id="action-items"] li') (used in
verifyActionsInActionMenu), trim them, and assert that the resulting array
contains all of ['Upgrade','Rollback','Delete Helm Release'] and that its length
equals 3 (or otherwise assert exact set equality) so missing or extra items will
fail the test.
In `@frontend/e2e/pages/helm-upgrade-page.ts`:
- Around line 29-41: selectDifferentChartVersion currently always clicks the
first option (chartVersionOptions.nth(0)) which may be the current version;
change it to choose a non-current option by inspecting chartVersionOptions and
selecting an index > 0 (e.g., get count via chartVersionOptions.count(), compute
a randomIndex in range [1, count-1], or iterate options to find one without the
"current" marker/text) and then call robustClick on that option; keep the
existing waits (expect(this.chartVersionDropdown).toBeEnabled) and use the same
robustClick helper.
In `@frontend/e2e/pages/topology-sidebar-page.ts`:
- Around line 52-55: The selector mismatch warning is incorrect—playwright is
configured with testIdAttribute:'data-test' so the raw locator in selectTab
(function selectTab) is valid, but for consistency switch to using
this.page.getByTestId(`horizontal-link-${tabName}`) inside selectTab to match
the rest of the suite; also inspect the namespace variable/block in
topology-sidebar-page.ts (the declared namespace variable around lines ~62-74)
and either use it where intended or remove/clean up the unused declaration to
avoid dead code.
In `@frontend/e2e/tests/helm/helm-release.spec.ts`:
- Around line 119-123: The Helm flows currently call
HelmReleasesPage.navigateToHelmTab() and clickHelmReleasesTab() which open the
cluster-wide Helm view; change these to operate in the test's namespace instead
by either invoking a namespace-aware navigation API (e.g. add a namespace arg to
navigateToHelmTab(namespace) or call a helper like
helmPage.selectNamespace(testNamespace) before clickHelmReleasesTab()), and
update the other occurrences in this file that use the same pattern (the other
Helm-page flows) so each test only searches/releases within its created
namespace.
---
Nitpick comments:
In `@frontend/e2e/pages/topology-page.ts`:
- Around line 76-86: Remove the leftover debug comment in rightClickWorkload and
replace the brittle chained SVG selector and hard-coded nth(1) with a resilient
locator: first locate the topology node that matches workloadName (e.g., find
the node element by its visible label/text after searchTopology) and then locate
the action trigger inside that node via a stable attribute or semantic selector
(data-testid, aria-label, or getByRole('button')) instead of targeting
.pf-v6-svg/.pf-v6-icon-default/path; keep the rest of the flow
(waitForLoadingComplete, ensureGraphView, resetView/fitToScreen,
scrollIntoViewIfNeeded, robustClick) but target the action-icon via the stable
test id/role on the node rather than SVG internals or a fixed index.
In `@frontend/e2e/pages/topology-sidebar-page.ts`:
- Around line 62-74: The verifyResourceDetailsPage function includes an unused
namespace parameter; remove the namespace parameter from the
verifyResourceDetailsPage signature and from all call sites (notably the call in
topology-helm-sidebar.spec.ts) so the function only accepts (resource,
helmRelease, options); update any TypeScript types or references to the old
signature accordingly (search for verifyResourceDetailsPage and adjust
invocations to pass only the required arguments), and run tests to ensure no
remaining references to the removed parameter remain.
🪄 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: 6183e443-0be1-48ed-a176-6d6d8b3f54f1
📒 Files selected for processing (55)
frontend/e2e/pages/delete-helm-release-modal.tsfrontend/e2e/pages/helm-details-page.tsfrontend/e2e/pages/helm-install-page.tsfrontend/e2e/pages/helm-releases-page.tsfrontend/e2e/pages/helm-rollback-page.tsfrontend/e2e/pages/helm-upgrade-page.tsfrontend/e2e/pages/software-catalog-page.tsfrontend/e2e/pages/topology-page.tsfrontend/e2e/pages/topology-sidebar-page.tsfrontend/e2e/tests/helm/helm-release.spec.tsfrontend/e2e/tests/helm/topology-helm-sidebar.spec.tsfrontend/integration-tests/test-cypress.shfrontend/package.jsonfrontend/packages/helm-plugin/integration-tests/.eslintrcfrontend/packages/helm-plugin/integration-tests/README.mdfrontend/packages/helm-plugin/integration-tests/cypress.config.jsfrontend/packages/helm-plugin/integration-tests/features/BestPractices.mdfrontend/packages/helm-plugin/integration-tests/features/helm-release.featurefrontend/packages/helm-plugin/integration-tests/features/helm/actions-on-helm-release-after-upgrade.featurefrontend/packages/helm-plugin/integration-tests/features/helm/actions-on-helm-release.featurefrontend/packages/helm-plugin/integration-tests/features/helm/helm-compatibility.featurefrontend/packages/helm-plugin/integration-tests/features/helm/helm-feature-flag.featurefrontend/packages/helm-plugin/integration-tests/features/helm/helm-installation-view.featurefrontend/packages/helm-plugin/integration-tests/features/helm/helm-navigation.featurefrontend/packages/helm-plugin/integration-tests/features/helm/helm-page-tabs.featurefrontend/packages/helm-plugin/integration-tests/features/helm/install-helm-chart.featurefrontend/packages/helm-plugin/integration-tests/features/helm/install-url-chart.featurefrontend/packages/helm-plugin/integration-tests/features/helm/topology-helm-release.featurefrontend/packages/helm-plugin/integration-tests/package.jsonfrontend/packages/helm-plugin/integration-tests/reporter-config.jsonfrontend/packages/helm-plugin/integration-tests/support/commands/hooks.tsfrontend/packages/helm-plugin/integration-tests/support/commands/index.tsfrontend/packages/helm-plugin/integration-tests/support/constants/index.tsfrontend/packages/helm-plugin/integration-tests/support/constants/navigation.tsfrontend/packages/helm-plugin/integration-tests/support/constants/static-text/helm-text.tsfrontend/packages/helm-plugin/integration-tests/support/pages/helm/helm-details-page.tsfrontend/packages/helm-plugin/integration-tests/support/pages/helm/helm-page.tsfrontend/packages/helm-plugin/integration-tests/support/pages/helm/index.tsfrontend/packages/helm-plugin/integration-tests/support/pages/helm/rollBack-helm-release-page.tsfrontend/packages/helm-plugin/integration-tests/support/pages/helm/upgrade-helm-release-page.tsfrontend/packages/helm-plugin/integration-tests/support/pages/helm/url-chart-install-page.tsfrontend/packages/helm-plugin/integration-tests/support/pages/index.tsfrontend/packages/helm-plugin/integration-tests/support/step-definitions/common/common.tsfrontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/actions-on-helm-release-after-upgrade.tsfrontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-compatibility.tsfrontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-installation-view.tsfrontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-navigation.tsfrontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-release.tsfrontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm.tsfrontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/install-url-chart.tsfrontend/packages/helm-plugin/integration-tests/test-data/namespaced-helm-chart-repository.yamlfrontend/packages/helm-plugin/integration-tests/test-data/namespaced-helm-crd.yamlfrontend/packages/helm-plugin/integration-tests/test-data/red-hat-helm-charts.yamlfrontend/packages/helm-plugin/integration-tests/tsconfig.jsonfrontend/packages/topology/src/components/side-bar/TopologyGroupResourceItem.tsx
💤 Files with no reviewable changes (43)
- frontend/packages/helm-plugin/integration-tests/support/constants/navigation.ts
- frontend/packages/helm-plugin/integration-tests/reporter-config.json
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/actions-on-helm-release-after-upgrade.ts
- frontend/packages/helm-plugin/integration-tests/.eslintrc
- frontend/packages/helm-plugin/integration-tests/features/helm/helm-feature-flag.feature
- frontend/packages/helm-plugin/integration-tests/features/helm/topology-helm-release.feature
- frontend/packages/helm-plugin/integration-tests/support/pages/helm/helm-details-page.ts
- frontend/packages/helm-plugin/integration-tests/support/pages/helm/upgrade-helm-release-page.ts
- frontend/packages/helm-plugin/integration-tests/features/BestPractices.md
- frontend/packages/helm-plugin/integration-tests/features/helm/helm-installation-view.feature
- frontend/packages/helm-plugin/integration-tests/support/constants/static-text/helm-text.ts
- frontend/packages/helm-plugin/integration-tests/support/pages/helm/rollBack-helm-release-page.ts
- frontend/packages/helm-plugin/integration-tests/support/commands/hooks.ts
- frontend/packages/helm-plugin/integration-tests/support/pages/helm/url-chart-install-page.ts
- frontend/packages/helm-plugin/integration-tests/features/helm-release.feature
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/install-url-chart.ts
- frontend/packages/helm-plugin/integration-tests/features/helm/actions-on-helm-release.feature
- frontend/packages/helm-plugin/integration-tests/test-data/namespaced-helm-crd.yaml
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/common/common.ts
- frontend/packages/helm-plugin/integration-tests/support/commands/index.ts
- frontend/packages/helm-plugin/integration-tests/support/pages/index.ts
- frontend/packages/helm-plugin/integration-tests/features/helm/helm-page-tabs.feature
- frontend/packages/helm-plugin/integration-tests/features/helm/install-helm-chart.feature
- frontend/packages/helm-plugin/integration-tests/tsconfig.json
- frontend/packages/helm-plugin/integration-tests/test-data/red-hat-helm-charts.yaml
- frontend/packages/helm-plugin/integration-tests/support/pages/helm/index.ts
- frontend/packages/helm-plugin/integration-tests/support/constants/index.ts
- frontend/packages/helm-plugin/integration-tests/cypress.config.js
- frontend/packages/helm-plugin/integration-tests/README.md
- frontend/packages/helm-plugin/integration-tests/features/helm/actions-on-helm-release-after-upgrade.feature
- frontend/packages/helm-plugin/integration-tests/features/helm/helm-compatibility.feature
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm.ts
- frontend/packages/helm-plugin/integration-tests/package.json
- frontend/packages/helm-plugin/integration-tests/features/helm/install-url-chart.feature
- frontend/integration-tests/test-cypress.sh
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-navigation.ts
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-release.ts
- frontend/packages/helm-plugin/integration-tests/features/helm/helm-navigation.feature
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-compatibility.ts
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-installation-view.ts
- frontend/packages/helm-plugin/integration-tests/support/pages/helm/helm-page.ts
- frontend/packages/helm-plugin/integration-tests/test-data/namespaced-helm-chart-repository.yaml
- frontend/package.json
Comment on lines
+47
to
+56
| async verifyActionsInActionMenu(): Promise<void> { | ||
| const actions = ['Upgrade', 'Rollback', 'Delete Helm Release']; | ||
| const actionItems = this.page.locator('[data-test-id="action-items"] li'); | ||
| const count = await actionItems.count(); | ||
|
|
||
| for (let i = 0; i < count; i++) { | ||
| const text = await actionItems.nth(i).textContent(); | ||
| expect(actions).toContain(text?.trim()); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Assert the expected actions explicitly.
This only checks that every rendered item is allowed; it does not prove that all three expected actions are present. If Rollback disappears, this still passes as long as the remaining items are valid.
Suggested tightening
async verifyActionsInActionMenu(): Promise<void> {
- const actions = ['Upgrade', 'Rollback', 'Delete Helm Release'];
- const actionItems = this.page.locator('[data-test-id="action-items"] li');
- const count = await actionItems.count();
-
- for (let i = 0; i < count; i++) {
- const text = await actionItems.nth(i).textContent();
- expect(actions).toContain(text?.trim());
- }
+ const expectedActions = ['Upgrade', 'Rollback', 'Delete Helm Release'];
+ const actionItems = this.page.locator('[data-test-id="action-items"] li');
+
+ await expect(actionItems).toHaveCount(expectedActions.length);
+ for (const action of expectedActions) {
+ await expect(actionItems.filter({ hasText: action })).toHaveCount(1);
+ }
}🤖 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/helm-details-page.ts` around lines 47 - 56, The test
verifyActionsInActionMenu currently only asserts that each rendered item is in
the allowed list; change it to explicitly assert the expected actions are
present and no extras: collect the text values from the locator
this.page.locator('[data-test-id="action-items"] li') (used in
verifyActionsInActionMenu), trim them, and assert that the resulting array
contains all of ['Upgrade','Rollback','Delete Helm Release'] and that its length
equals 3 (or otherwise assert exact set equality) so missing or extra items will
fail the test.
Comment on lines
+29
to
+41
| async selectDifferentChartVersion(): Promise<void> { | ||
| // Wait for dropdown to be enabled | ||
| await expect(this.chartVersionDropdown).toBeEnabled({ timeout: 10_000 }); | ||
|
|
||
| // Click to open dropdown | ||
| await this.robustClick(this.chartVersionDropdown); | ||
|
|
||
| // Get count of available versions | ||
| // const count = await this.chartVersionOptions.count(); | ||
|
|
||
| // Select a random version (but not the first one which might be current) | ||
| // const randomIndex = Math.floor(Math.random() * (count - 1)) + 1; | ||
| await this.robustClick(this.chartVersionOptions.nth(0)); |
Contributor
There was a problem hiding this comment.
Pick a non-current chart version here.
selectDifferentChartVersion() always clicks nth(0), even though the comment says the first option may be the current version. That can make the upgrade a no-op and leave the confirmation modal/revision-based checks flaky.
One straightforward fix
async selectDifferentChartVersion(): Promise<void> {
// Wait for dropdown to be enabled
await expect(this.chartVersionDropdown).toBeEnabled({ timeout: 10_000 });
// Click to open dropdown
await this.robustClick(this.chartVersionDropdown);
- // Get count of available versions
- // const count = await this.chartVersionOptions.count();
-
- // Select a random version (but not the first one which might be current)
- // const randomIndex = Math.floor(Math.random() * (count - 1)) + 1;
- await this.robustClick(this.chartVersionOptions.nth(0));
+ const count = await this.chartVersionOptions.count();
+ expect(count).toBeGreaterThan(1);
+
+ // Skip the first option if it represents the currently selected version.
+ await this.robustClick(this.chartVersionOptions.nth(1));
}📝 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.
Suggested change
| async selectDifferentChartVersion(): Promise<void> { | |
| // Wait for dropdown to be enabled | |
| await expect(this.chartVersionDropdown).toBeEnabled({ timeout: 10_000 }); | |
| // Click to open dropdown | |
| await this.robustClick(this.chartVersionDropdown); | |
| // Get count of available versions | |
| // const count = await this.chartVersionOptions.count(); | |
| // Select a random version (but not the first one which might be current) | |
| // const randomIndex = Math.floor(Math.random() * (count - 1)) + 1; | |
| await this.robustClick(this.chartVersionOptions.nth(0)); | |
| async selectDifferentChartVersion(): Promise<void> { | |
| // Wait for dropdown to be enabled | |
| await expect(this.chartVersionDropdown).toBeEnabled({ timeout: 10_000 }); | |
| // Click to open dropdown | |
| await this.robustClick(this.chartVersionDropdown); | |
| const count = await this.chartVersionOptions.count(); | |
| expect(count).toBeGreaterThan(1); | |
| // Skip the first option if it represents the currently selected version. | |
| await this.robustClick(this.chartVersionOptions.nth(1)); | |
| } |
🤖 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/helm-upgrade-page.ts` around lines 29 - 41,
selectDifferentChartVersion currently always clicks the first option
(chartVersionOptions.nth(0)) which may be the current version; change it to
choose a non-current option by inspecting chartVersionOptions and selecting an
index > 0 (e.g., get count via chartVersionOptions.count(), compute a
randomIndex in range [1, count-1], or iterate options to find one without the
"current" marker/text) and then call robustClick on that option; keep the
existing waits (expect(this.chartVersionDropdown).toBeEnabled) and use the same
robustClick helper.
Comment on lines
+52
to
+55
| async selectTab(tabName: string): Promise<void> { | ||
| const tab = this.page.locator(`[data-test="horizontal-link-${tabName}"]`); | ||
| await this.robustClick(tab); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) What testIdAttribute does getByTestId resolve to?
fd -e ts playwright.config | xargs rg -n 'testIdAttribute' 2>/dev/null
# 2) Which attribute do the horizontal nav links actually render?
rg -nP "data-test(-id)?=.*horizontal-link"
ast-grep --pattern $'data-test-id={`horizontal-link-$_`}'
ast-grep --pattern $'data-test={`horizontal-link-$_`}'Repository: openshift/console
Length of output: 6494
Fix selector mismatch concern in selectTab (and check namespace usage).
playwright.config is configured with testIdAttribute: 'data-test', so getByTestId('horizontal-link-…') and selectTab’s raw locator [data-test="horizontal-link-${tabName}"] are aligned. The existing horizontal-link-* components also include data-test attributes (with additional data-test-id on some components), so selectTab('Resources') should still locate the tab; the attribute-mismatch concern is not valid.
Also re-check namespace in frontend/e2e/pages/topology-sidebar-page.ts (lines ~62-74) to ensure it’s actually used.
🤖 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-sidebar-page.ts` around lines 52 - 55, The
selector mismatch warning is incorrect—playwright is configured with
testIdAttribute:'data-test' so the raw locator in selectTab (function selectTab)
is valid, but for consistency switch to using
this.page.getByTestId(`horizontal-link-${tabName}`) inside selectTab to match
the rest of the suite; also inspect the namespace variable/block in
topology-sidebar-page.ts (the declared namespace variable around lines ~62-74)
and either use it where intended or remove/clean up the unused declaration to
avoid dead code.
Comment on lines
+119
to
+123
| await test.step('Navigate to Helm Release tab', async () => { | ||
| const helmPage = new HelmReleasesPage(page); | ||
| await helmPage.navigateToHelmTab(); | ||
| await helmPage.clickHelmReleasesTab(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Keep these Helm-page flows scoped to the test namespace.
Each test creates its own namespace, but these call sites jump back to the cluster-wide Helm view. Since the suite reuses nodejs-release, the later search/click/kebab steps can operate on a different release when another namespace already has the same name.
Suggested change
- await helmPage.navigateToHelmTab();
+ await helmPage.navigateToHelmTab(namespace);Apply the same change to the other namespace-aware Helm-page flows in this file.
Also applies to: 207-210, 231-234, 265-269, 366-370, 411-415
🤖 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/helm/helm-release.spec.ts` around lines 119 - 123, The
Helm flows currently call HelmReleasesPage.navigateToHelmTab() and
clickHelmReleasesTab() which open the cluster-wide Helm view; change these to
operate in the test's namespace instead by either invoking a namespace-aware
navigation API (e.g. add a namespace arg to navigateToHelmTab(namespace) or call
a helper like helmPage.selectNamespace(testNamespace) before
clickHelmReleasesTab()), and update the other occurrences in this file that use
the same pattern (the other Helm-page flows) so each test only searches/releases
within its created namespace.
Member
Author
|
/retest |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/helm/topology-helm-sidebar.spec.ts`:
- Around line 8-33: The test currently uses the hard-coded
HELM_SIDEBAR_RESOURCE_LINKS array to assert five specific sidebar links, but the
Nodejs chart output can vary; update the test in topology-helm-sidebar.spec.ts
to only assert against links actually rendered for the current release instead
of expecting the full HELM_SIDEBAR_RESOURCE_LINKS set (or swap to a
deterministic fixture/chart that always produces all resources). Concretely,
change the test logic around the code that iterates/compares
HELM_SIDEBAR_RESOURCE_LINKS so it either (a) queries the rendered sidebar DOM,
builds a list of present resource/urlSegment values and asserts those are a
superset/subset of expected values, or (b) load a deterministic test chart
fixture and then assert full equality with HELM_SIDEBAR_RESOURCE_LINKS; update
assertions used near lines referencing HELM_SIDEBAR_RESOURCE_LINKS and any
subsequent assertions at the block around lines 61-70 to match the chosen
approach.
- Line 66: The current urlPattern (new
RegExp(`/k8s/ns/${namespace}/${urlSegment}/`)) is unanchored and may match
substrings in the full URL; update the pattern used by the urlPattern variable
so it anchors the path portion of the full URL (e.g. build a RegExp that matches
protocol+host then the exact path start like
^https?:\/\/[^\/]+\/k8s\/ns\/${namespace}\/${urlSegment}\/) so Playwright's
toHaveURL(RegExp) asserts the expected path precisely.
🪄 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: dcec01d3-8cf2-4afe-a18b-e440f0c2b786
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (55)
frontend/e2e/pages/delete-helm-release-modal.tsfrontend/e2e/pages/helm-details-page.tsfrontend/e2e/pages/helm-install-page.tsfrontend/e2e/pages/helm-releases-page.tsfrontend/e2e/pages/helm-rollback-page.tsfrontend/e2e/pages/helm-upgrade-page.tsfrontend/e2e/pages/software-catalog-page.tsfrontend/e2e/pages/topology-page.tsfrontend/e2e/pages/topology-sidebar-page.tsfrontend/e2e/tests/helm/helm-release.spec.tsfrontend/e2e/tests/helm/topology-helm-sidebar.spec.tsfrontend/integration-tests/test-cypress.shfrontend/package.jsonfrontend/packages/helm-plugin/integration-tests/.eslintrcfrontend/packages/helm-plugin/integration-tests/README.mdfrontend/packages/helm-plugin/integration-tests/cypress.config.jsfrontend/packages/helm-plugin/integration-tests/features/BestPractices.mdfrontend/packages/helm-plugin/integration-tests/features/helm-release.featurefrontend/packages/helm-plugin/integration-tests/features/helm/actions-on-helm-release-after-upgrade.featurefrontend/packages/helm-plugin/integration-tests/features/helm/actions-on-helm-release.featurefrontend/packages/helm-plugin/integration-tests/features/helm/helm-compatibility.featurefrontend/packages/helm-plugin/integration-tests/features/helm/helm-feature-flag.featurefrontend/packages/helm-plugin/integration-tests/features/helm/helm-installation-view.featurefrontend/packages/helm-plugin/integration-tests/features/helm/helm-navigation.featurefrontend/packages/helm-plugin/integration-tests/features/helm/helm-page-tabs.featurefrontend/packages/helm-plugin/integration-tests/features/helm/install-helm-chart.featurefrontend/packages/helm-plugin/integration-tests/features/helm/install-url-chart.featurefrontend/packages/helm-plugin/integration-tests/features/helm/topology-helm-release.featurefrontend/packages/helm-plugin/integration-tests/package.jsonfrontend/packages/helm-plugin/integration-tests/reporter-config.jsonfrontend/packages/helm-plugin/integration-tests/support/commands/hooks.tsfrontend/packages/helm-plugin/integration-tests/support/commands/index.tsfrontend/packages/helm-plugin/integration-tests/support/constants/index.tsfrontend/packages/helm-plugin/integration-tests/support/constants/navigation.tsfrontend/packages/helm-plugin/integration-tests/support/constants/static-text/helm-text.tsfrontend/packages/helm-plugin/integration-tests/support/pages/helm/helm-details-page.tsfrontend/packages/helm-plugin/integration-tests/support/pages/helm/helm-page.tsfrontend/packages/helm-plugin/integration-tests/support/pages/helm/index.tsfrontend/packages/helm-plugin/integration-tests/support/pages/helm/rollBack-helm-release-page.tsfrontend/packages/helm-plugin/integration-tests/support/pages/helm/upgrade-helm-release-page.tsfrontend/packages/helm-plugin/integration-tests/support/pages/helm/url-chart-install-page.tsfrontend/packages/helm-plugin/integration-tests/support/pages/index.tsfrontend/packages/helm-plugin/integration-tests/support/step-definitions/common/common.tsfrontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/actions-on-helm-release-after-upgrade.tsfrontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-compatibility.tsfrontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-installation-view.tsfrontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-navigation.tsfrontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-release.tsfrontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm.tsfrontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/install-url-chart.tsfrontend/packages/helm-plugin/integration-tests/test-data/namespaced-helm-chart-repository.yamlfrontend/packages/helm-plugin/integration-tests/test-data/namespaced-helm-crd.yamlfrontend/packages/helm-plugin/integration-tests/test-data/red-hat-helm-charts.yamlfrontend/packages/helm-plugin/integration-tests/tsconfig.jsonfrontend/packages/topology/src/components/side-bar/TopologyGroupResourceItem.tsx
💤 Files with no reviewable changes (43)
- frontend/packages/helm-plugin/integration-tests/features/helm/helm-compatibility.feature
- frontend/packages/helm-plugin/integration-tests/support/constants/static-text/helm-text.ts
- frontend/packages/helm-plugin/integration-tests/reporter-config.json
- frontend/packages/helm-plugin/integration-tests/features/helm/helm-installation-view.feature
- frontend/packages/helm-plugin/integration-tests/support/constants/index.ts
- frontend/packages/helm-plugin/integration-tests/support/constants/navigation.ts
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-compatibility.ts
- frontend/packages/helm-plugin/integration-tests/features/helm/topology-helm-release.feature
- frontend/packages/helm-plugin/integration-tests/features/helm-release.feature
- frontend/packages/helm-plugin/integration-tests/features/helm/helm-feature-flag.feature
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm.ts
- frontend/packages/helm-plugin/integration-tests/package.json
- frontend/packages/helm-plugin/integration-tests/support/pages/helm/url-chart-install-page.ts
- frontend/packages/helm-plugin/integration-tests/features/helm/install-helm-chart.feature
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/install-url-chart.ts
- frontend/packages/helm-plugin/integration-tests/test-data/red-hat-helm-charts.yaml
- frontend/package.json
- frontend/packages/helm-plugin/integration-tests/support/pages/helm/index.ts
- frontend/packages/helm-plugin/integration-tests/.eslintrc
- frontend/packages/helm-plugin/integration-tests/features/helm/install-url-chart.feature
- frontend/packages/helm-plugin/integration-tests/support/pages/index.ts
- frontend/packages/helm-plugin/integration-tests/README.md
- frontend/packages/helm-plugin/integration-tests/features/helm/actions-on-helm-release-after-upgrade.feature
- frontend/packages/helm-plugin/integration-tests/support/pages/helm/helm-details-page.ts
- frontend/packages/helm-plugin/integration-tests/test-data/namespaced-helm-chart-repository.yaml
- frontend/packages/helm-plugin/integration-tests/support/pages/helm/rollBack-helm-release-page.ts
- frontend/packages/helm-plugin/integration-tests/features/BestPractices.md
- frontend/packages/helm-plugin/integration-tests/features/helm/actions-on-helm-release.feature
- frontend/packages/helm-plugin/integration-tests/cypress.config.js
- frontend/packages/helm-plugin/integration-tests/tsconfig.json
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-navigation.ts
- frontend/packages/helm-plugin/integration-tests/features/helm/helm-navigation.feature
- frontend/packages/helm-plugin/integration-tests/support/pages/helm/upgrade-helm-release-page.ts
- frontend/packages/helm-plugin/integration-tests/test-data/namespaced-helm-crd.yaml
- frontend/packages/helm-plugin/integration-tests/support/commands/hooks.ts
- frontend/packages/helm-plugin/integration-tests/support/pages/helm/helm-page.ts
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/actions-on-helm-release-after-upgrade.ts
- frontend/packages/helm-plugin/integration-tests/features/helm/helm-page-tabs.feature
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-installation-view.ts
- frontend/integration-tests/test-cypress.sh
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/common/common.ts
- frontend/packages/helm-plugin/integration-tests/support/commands/index.ts
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-release.ts
✅ Files skipped from review due to trivial changes (1)
- frontend/packages/topology/src/components/side-bar/TopologyGroupResourceItem.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- frontend/e2e/pages/helm-upgrade-page.ts
- frontend/e2e/pages/helm-rollback-page.ts
- frontend/e2e/pages/software-catalog-page.ts
- frontend/e2e/pages/helm-install-page.ts
- frontend/e2e/pages/helm-details-page.ts
- frontend/e2e/pages/topology-sidebar-page.ts
- frontend/e2e/tests/helm/helm-release.spec.ts
- frontend/e2e/pages/topology-page.ts
- frontend/e2e/pages/delete-helm-release-modal.ts
- frontend/e2e/pages/helm-releases-page.ts
Comment on lines
+8
to
+33
| const HELM_SIDEBAR_RESOURCE_LINKS = [ | ||
| { | ||
| resource: 'Deployment', | ||
| urlSegment: 'deployments', | ||
| sectionHeading: 'Deployment details', | ||
| }, | ||
| { | ||
| resource: 'BuildConfig', | ||
| urlSegment: 'buildconfigs', | ||
| sectionHeading: 'BuildConfig details', | ||
| }, | ||
| { | ||
| resource: 'Service', | ||
| urlSegment: 'services', | ||
| sectionHeading: 'Service details', | ||
| }, | ||
| { | ||
| resource: 'ImageStream', | ||
| urlSegment: 'imagestreams', | ||
| sectionHeading: 'ImageStream details', | ||
| }, | ||
| { | ||
| resource: 'Route', | ||
| urlSegment: 'routes', | ||
| sectionHeading: 'Route details', | ||
| }, |
Contributor
There was a problem hiding this comment.
Stabilize resource-link assertions against actual chart output.
This test hard-codes five expected resource links, but the PR context already indicates the Nodejs chart does not always produce all of them, which makes this scenario fail for reasons unrelated to sidebar behavior. Please either use a fixture/chart that deterministically creates the full set or scope assertions to links actually rendered for that release.
Also applies to: 61-70
🤖 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/helm/topology-helm-sidebar.spec.ts` around lines 8 - 33,
The test currently uses the hard-coded HELM_SIDEBAR_RESOURCE_LINKS array to
assert five specific sidebar links, but the Nodejs chart output can vary; update
the test in topology-helm-sidebar.spec.ts to only assert against links actually
rendered for the current release instead of expecting the full
HELM_SIDEBAR_RESOURCE_LINKS set (or swap to a deterministic fixture/chart that
always produces all resources). Concretely, change the test logic around the
code that iterates/compares HELM_SIDEBAR_RESOURCE_LINKS so it either (a) queries
the rendered sidebar DOM, builds a list of present resource/urlSegment values
and asserts those are a superset/subset of expected values, or (b) load a
deterministic test chart fixture and then assert full equality with
HELM_SIDEBAR_RESOURCE_LINKS; update assertions used near lines referencing
HELM_SIDEBAR_RESOURCE_LINKS and any subsequent assertions at the block around
lines 61-70 to match the chosen approach.
| await topologyPage.navigateToTopology(namespace); | ||
| await topologyPage.clickWorkload(RELEASE_NAME); | ||
| await sidebar.verifyResourceDetailsPage(resource, namespace, RELEASE_NAME, { | ||
| urlPattern: new RegExp(`/k8s/ns/${namespace}/${urlSegment}/`), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="frontend/e2e/tests/helm/topology-helm-sidebar.spec.ts"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
# Show surrounding context for the reported line
nl -ba "$FILE" | sed -n '40,95p'Repository: openshift/console
Length of output: 105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="frontend/e2e/tests/helm/topology-helm-sidebar.spec.ts"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
# Show surrounding context for the reported line (avoid nl)
awk 'NR>=40 && NR<=95 {printf "%d:%s\n", NR, $0}' "$FILE"Repository: openshift/console
Length of output: 1571
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the implementation of TopologySidebarPage and verifyResourceDetailsPage
rg -n "class TopologySidebarPage|verifyResourceDetailsPage\(" -S frontend/e2e/tests frontend/e2e || true
# Also search for urlPattern usage
rg -n "urlPattern" -S frontend/e2e || trueRepository: openshift/console
Length of output: 875
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="frontend/e2e/pages/topology-sidebar-page.ts"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
# Print relevant portion with line numbers via awk
awk 'NR>=1 && NR<=140 {printf "%d:%s\n", NR, $0}' "$FILE" | sed -n '1,140p'Repository: openshift/console
Length of output: 3112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="frontend/e2e/tests/helm/topology-helm-sidebar.spec.ts"
awk 'NR>=1 && NR<=80 {printf "%d:%s\n", NR, $0}' "$FILE" | sed -n '1,80p'Repository: openshift/console
Length of output: 2791
🌐 Web query:
Playwright expect(page).toHaveURL RegExp matches full URL or substring
💡 Result:
When using the Playwright expect(page).toHaveURL assertion with a regular expression, the RegExp matches against the full URL of the page [1][2]. If you want to match a substring, you must construct your regular expression to account for the preceding and following parts of the URL. A common pattern is to use /./substring/ or similar expressions that include wildcards to match the rest of the URL string [1][2]. For example, to check that the URL contains "/dashboard": await expect(page).toHaveURL(/./dashboard/); Key details regarding toHaveURL: - String matching: When you pass a string, Playwright verifies that the page URL matches that string exactly (after any potential baseURL merging) [1][2]. - RegExp matching: The regular expression is tested against the full URL [1][2]. - Other options: You can also use URLPattern for more structured matching or a predicate function for complex logic, such as validating specific search parameters [1][2].
Citations:
- 1: https://github.com/microsoft/playwright/blob/main/docs/src/api/class-pageassertions.md
- 2: https://playwright.dev/docs/next/api/class-pageassertions
Anchor the URL regex used in toHaveURL for Helm sidebar details assertions.
expect(page).toHaveURL(RegExp) tests the RegExp against the full URL string; with the current unanchored pattern, it can still match substrings and become brittle if the URL changes. Also, namespace (aut-helm-${Date.now()}) and urlSegment (hardcoded constants) are regex-safe, but anchoring is still the key improvement.
Suggested fix
+const escapeRegex = (value: string): string => value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+
const HELM_SIDEBAR_RESOURCE_LINKS = [
{
@@
await sidebar.verifyResourceDetailsPage(resource, namespace, RELEASE_NAME, {
- urlPattern: new RegExp(`/k8s/ns/${namespace}/${urlSegment}/`),
+ urlPattern: new RegExp(
+ `^.*/k8s/ns/${escapeRegex(namespace)}/${escapeRegex(urlSegment)}(?:/|$)`,
+ ),
sectionHeading,
});🤖 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/helm/topology-helm-sidebar.spec.ts` at line 66, The
current urlPattern (new RegExp(`/k8s/ns/${namespace}/${urlSegment}/`)) is
unanchored and may match substrings in the full URL; update the pattern used by
the urlPattern variable so it anchors the path portion of the full URL (e.g.
build a RegExp that matches protocol+host then the exact path start like
^https?:\/\/[^\/]+\/k8s\/ns\/${namespace}\/${urlSegment}\/) so Playwright's
toHaveURL(RegExp) asserts the expected path precisely.
Contributor
|
@vikram-raj: 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. |
Contributor
|
PR needs rebase. 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. |
openshift-ci
Bot
added
the
needs-rebase
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.
What does this PR do?
This PR migrates all Helm plugin Cypress E2E tests to Playwright, completing the Cypress→Playwright migration effort for the Helm plugin.
Migration Summary:
dataTestprop toResourceLinkfor E2E testabilityWhich Jira issue does this PR address?
CONSOLE-5240
Test Coverage
Page Objects Created
All page objects follow the Console Page Object Model pattern with
BasePageinheritance:Selector Strategy
.helmTable.getByRole())Test Isolation
All tests use self-contained isolation:
Date.now()timestampHelmInstallPage.installHelmChartFromCatalog()cleanup.trackNamespace()Production Code Changes
File:
frontend/packages/topology/src/components/side-bar/TopologyGroupResourceItem.tsxChange: Added
dataTestprop toResourceLinkcomponent:Rationale: Enables stable Playwright selectors for topology sidebar resource links without relying on DOM structure.
Known Issues
topology-helm-sidebar.spec.ts:
Pre-Push Review Findings
Ran
/pre-push-review(Claude + CodeRabbit dual AI review):Issues Identified (Not Fixed in This PR)
data-testinstead ofdata-test-id(inconsistent with constructor)namespaceparameterThese issues are documented for follow-up to keep this PR focused on the migration itself.
Screenshots / Screen Recording
QE Impact
Reduced: Playwright tests are faster and more reliable than Cypress
How to Test Locally
Checklist
Related PRs
🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores