CONSOLE-5240: Migrate Helm Cypress E2E tests to Playwright#16711
Open
vikram-raj wants to merge 2 commits into
Open
CONSOLE-5240: Migrate Helm Cypress E2E tests to Playwright#16711vikram-raj wants to merge 2 commits into
vikram-raj wants to merge 2 commits into
Conversation
Address CodeRabbit review findings: - Add rollback success assertion to verify operation completed - Fix race condition in upgrade confirmation dialog handling - Remove unused helmReleasesTab variable These changes prevent potential test flakiness by: 1. Ensuring rollback failures are detected (was silently continuing) 2. Properly waiting for confirmation dialogs that may appear delayed 3. Cleaning up unused code Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
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
WalkthroughThis PR introduces new Playwright page objects (HelmDetailsPage, HelmPage) and an E2E spec covering Helm release install, upgrade, rollback, and delete flows. It also removes the entire legacy Cypress/Cucumber Helm integration test suite, its scripts, configuration files, page objects, step definitions, feature files, and CI wiring. ChangesPlaywright Helm E2E Migration
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Test as helm-release.spec.ts
participant HelmPage
participant HelmDetailsPage
participant UI as Console UI
Test->>HelmPage: navigateToCatalog(namespace)
Test->>HelmPage: searchAndSelectChart(chartName)
Test->>HelmPage: clickCreateOnSidePane()
Test->>HelmPage: enterReleaseName(name)
Test->>HelmPage: clickInstallButton()
HelmPage->>UI: submit install form
UI-->>Test: release listed with status
Test->>HelmPage: clickKebabMenu()
Test->>HelmPage: selectAction("Upgrade")
Test->>HelmPage: upgradeChartVersion()
Test->>HelmPage: clickUpgradeButton()
UI-->>Test: updated status
Test->>HelmDetailsPage: clickRevisionHistoryTab()
Test->>HelmPage: selectRevision()
Test->>HelmPage: clickRollbackButton()
UI-->>Test: rollback complete
Test->>HelmDetailsPage: clickActionsMenu()
Test->>HelmDetailsPage: enterReleaseNameInDeletePopup(name)
Test->>HelmDetailsPage: confirmDelete()
UI-->>Test: navigate to releases list
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ 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/helm
![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: 3
🧹 Nitpick comments (3)
frontend/e2e/pages/helm-page.ts (1)
40-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate filter-item selector logic.
filterByStatusreconstructs the exact samedata-ouia-component-id="DataViewCheckboxFilter-filter-item-..."selector already exposed viagetFilterDropdownItem(Lines 186-190). Reuse the getter to avoid selector drift between the two.♻️ Proposed dedup
async filterByStatus(status: string): Promise<void> { const filterToggle = this.dataViewFilters.locator('.pf-v6-c-menu-toggle').first(); await this.robustClick(filterToggle); await this.page.locator('.pf-v6-c-menu__list-item', { hasText: 'Status' }).click(); await this.robustClick(this.filterDropdown); - const filterItem = this.page.locator( - `[data-ouia-component-id="DataViewCheckboxFilter-filter-item-${status.toLowerCase()}"]`, - ); + const filterItem = this.getFilterDropdownItem(status); await this.robustClick(filterItem); await this.robustClick(this.filterDropdown); }🤖 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-page.ts` around lines 40 - 50, The filter-by-status flow duplicates the `DataViewCheckboxFilter-filter-item-*` selector logic already centralized in `getFilterDropdownItem`; update `filterByStatus` in `HelmPage` to reuse that getter instead of reconstructing the `data-ouia-component-id` string directly. Keep the existing click sequence, but resolve the item through `getFilterDropdownItem(status)` so selector changes only need to be made in one place.frontend/e2e/pages/helm-details-page.ts (1)
71-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
verifyActionsInMenuhelperIt has no callers in the e2e suite and only scrolls menu items into view; either delete it or replace it with an assertion-based check if it’s meant to validate the menu.
🤖 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 71 - 77, The verifyActionsInMenu helper in HelmDetailsPage is unused and only performs scrolling without validating anything. Remove the method if it is not needed, or update it to assert the expected menu items are present using this.actionItems and the listed actions ('Upgrade', 'Rollback', 'Delete Helm Release') so it actually verifies the menu state.frontend/e2e/tests/helm/helm-release.spec.ts (1)
151-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated raw action-item locators instead of reusing the page object.
page.locator('[data-test-action="..."]')is constructed inline here even thoughHelmPage.selectAction()already encapsulates this exact locator pattern. Consider adding agetActionMenuItem(actionName)getter toHelmPage(mirroringHelmDetailsPage.getActionMenuItem) so the spec doesn't duplicate selector strings.♻️ Suggested approach
- const upgradeAction = page.locator('[data-test-action="Upgrade"]'); - await expect(upgradeAction).toBeVisible({ timeout: 15_000 }); - await expect(page.locator('[data-test-action="Rollback"]')).toBeVisible(); - await expect(page.locator('[data-test-action="Delete Helm Release"]')).toBeVisible(); + await expect(helmPage.getActionMenuItem('Upgrade')).toBeVisible({ timeout: 15_000 }); + await expect(helmPage.getActionMenuItem('Rollback')).toBeVisible(); + await expect(helmPage.getActionMenuItem('Delete Helm Release')).toBeVisible();🤖 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 151 - 158, The helm release spec is duplicating raw action-menu selectors instead of using the page object abstraction. Update the verification in helm-release.spec.ts to use HelmPage for these menu items, and add a getActionMenuItem(actionName) helper on HelmPage that mirrors HelmDetailsPage.getActionMenuItem so the selector logic lives in one place. Keep the existing clickKebabMenu and action-name usage, but replace inline page.locator('[data-test-action="..."]') calls with the shared helper.
🤖 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-page.ts`:
- Around line 97-103: The upgradeChartVersion method currently silently no-ops
when no alternate chart version is available, allowing the caller to continue as
if an upgrade happened. Update HelmPage.upgradeChartVersion to explicitly handle
the zero-item case by failing or otherwise surfacing that no upgrade target
exists before returning, and keep the existing click-on-first-option behavior
only when console-select-item has entries.
In `@frontend/e2e/tests/helm/helm-release.spec.ts`:
- Around line 107-121: The details-page actions check is missing Rollback
coverage because it manually asserts only Upgrade and Delete. Update the helm
release e2e step to use HelmDetailsPage.verifyActionsInMenu() instead of
individual menu-item checks, so all three actions including Rollback are
verified through the existing helper.
- Around line 160-166: The second upgrade flow in helm-release.spec.ts only
waits for the URL change, so the revision list may still be stale when rollback
is opened. In the upgrade step around clickUpgradeButton() and the following
expect(page).toHaveURL check, add the same post-upgrade wait used earlier to
confirm the release reaches Deployed before proceeding. This should be done in
the test step that performs the second upgrade, so selectRevision() sees the
refreshed revision list.
---
Nitpick comments:
In `@frontend/e2e/pages/helm-details-page.ts`:
- Around line 71-77: The verifyActionsInMenu helper in HelmDetailsPage is unused
and only performs scrolling without validating anything. Remove the method if it
is not needed, or update it to assert the expected menu items are present using
this.actionItems and the listed actions ('Upgrade', 'Rollback', 'Delete Helm
Release') so it actually verifies the menu state.
In `@frontend/e2e/pages/helm-page.ts`:
- Around line 40-50: The filter-by-status flow duplicates the
`DataViewCheckboxFilter-filter-item-*` selector logic already centralized in
`getFilterDropdownItem`; update `filterByStatus` in `HelmPage` to reuse that
getter instead of reconstructing the `data-ouia-component-id` string directly.
Keep the existing click sequence, but resolve the item through
`getFilterDropdownItem(status)` so selector changes only need to be made in one
place.
In `@frontend/e2e/tests/helm/helm-release.spec.ts`:
- Around line 151-158: The helm release spec is duplicating raw action-menu
selectors instead of using the page object abstraction. Update the verification
in helm-release.spec.ts to use HelmPage for these menu items, and add a
getActionMenuItem(actionName) helper on HelmPage that mirrors
HelmDetailsPage.getActionMenuItem so the selector logic lives in one place. Keep
the existing clickKebabMenu and action-name usage, but replace inline
page.locator('[data-test-action="..."]') calls with the shared helper.
🪄 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: 545130e5-3425-4d94-baae-9f04c9054ca9
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (46)
frontend/e2e/pages/helm-details-page.tsfrontend/e2e/pages/helm-page.tsfrontend/e2e/tests/helm/helm-release.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.json
💤 Files with no reviewable changes (43)
- frontend/packages/helm-plugin/integration-tests/features/BestPractices.md
- frontend/packages/helm-plugin/integration-tests/package.json
- frontend/packages/helm-plugin/integration-tests/features/helm/actions-on-helm-release-after-upgrade.feature
- frontend/packages/helm-plugin/integration-tests/features/helm/actions-on-helm-release.feature
- frontend/packages/helm-plugin/integration-tests/support/pages/helm/rollBack-helm-release-page.ts
- frontend/packages/helm-plugin/integration-tests/features/helm-release.feature
- frontend/packages/helm-plugin/integration-tests/features/helm/topology-helm-release.feature
- frontend/packages/helm-plugin/integration-tests/tsconfig.json
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/install-url-chart.ts
- frontend/packages/helm-plugin/integration-tests/features/helm/install-url-chart.feature
- frontend/packages/helm-plugin/integration-tests/features/helm/helm-installation-view.feature
- frontend/packages/helm-plugin/integration-tests/test-data/namespaced-helm-crd.yaml
- frontend/packages/helm-plugin/integration-tests/support/constants/index.ts
- frontend/packages/helm-plugin/integration-tests/test-data/namespaced-helm-chart-repository.yaml
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/actions-on-helm-release-after-upgrade.ts
- frontend/packages/helm-plugin/integration-tests/README.md
- frontend/packages/helm-plugin/integration-tests/test-data/red-hat-helm-charts.yaml
- frontend/packages/helm-plugin/integration-tests/support/commands/hooks.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/install-helm-chart.feature
- frontend/packages/helm-plugin/integration-tests/support/pages/helm/helm-details-page.ts
- frontend/packages/helm-plugin/integration-tests/support/pages/helm/url-chart-install-page.ts
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-release.ts
- frontend/packages/helm-plugin/integration-tests/support/commands/index.ts
- frontend/packages/helm-plugin/integration-tests/support/constants/static-text/helm-text.ts
- frontend/packages/helm-plugin/integration-tests/support/pages/helm/upgrade-helm-release-page.ts
- frontend/packages/helm-plugin/integration-tests/support/constants/navigation.ts
- frontend/packages/helm-plugin/integration-tests/features/helm/helm-navigation.feature
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-installation-view.ts
- frontend/packages/helm-plugin/integration-tests/features/helm/helm-compatibility.feature
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/common/common.ts
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-compatibility.ts
- frontend/packages/helm-plugin/integration-tests/support/pages/helm/helm-page.ts
- frontend/packages/helm-plugin/integration-tests/features/helm/helm-page-tabs.feature
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-navigation.ts
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm.ts
- frontend/packages/helm-plugin/integration-tests/cypress.config.js
- frontend/packages/helm-plugin/integration-tests/support/pages/index.ts
- frontend/packages/helm-plugin/integration-tests/reporter-config.json
- frontend/package.json
- frontend/integration-tests/test-cypress.sh
- frontend/packages/helm-plugin/integration-tests/support/pages/helm/index.ts
Comment on lines
+97
to
+103
| async upgradeChartVersion(): Promise<void> { | ||
| await this.chartVersionDropdown.click(); | ||
| const items = this.page.getByTestId('console-select-item'); | ||
| const count = await items.count(); | ||
| if (count > 0) { | ||
| await items.first().click(); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Silent no-op when no alternate chart version is available.
If items.count() is 0, upgradeChartVersion does nothing and the caller proceeds to submit anyway, so the "upgrade" may complete without any version change, potentially passing the test without actually exercising the upgrade behavior.
🐛 Proposed fix
async upgradeChartVersion(): Promise<void> {
await this.chartVersionDropdown.click();
const items = this.page.getByTestId('console-select-item');
const count = await items.count();
- if (count > 0) {
- await items.first().click();
- }
+ if (count === 0) {
+ throw new Error('No selectable chart versions available to upgrade to');
+ }
+ await items.first().click();📝 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 upgradeChartVersion(): Promise<void> { | |
| await this.chartVersionDropdown.click(); | |
| const items = this.page.getByTestId('console-select-item'); | |
| const count = await items.count(); | |
| if (count > 0) { | |
| await items.first().click(); | |
| } | |
| async upgradeChartVersion(): Promise<void> { | |
| await this.chartVersionDropdown.click(); | |
| const items = this.page.getByTestId('console-select-item'); | |
| const count = await items.count(); | |
| if (count === 0) { | |
| throw new Error('No selectable chart versions available to upgrade to'); | |
| } | |
| await items.first().click(); |
🤖 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-page.ts` around lines 97 - 103, The
upgradeChartVersion method currently silently no-ops when no alternate chart
version is available, allowing the caller to continue as if an upgrade happened.
Update HelmPage.upgradeChartVersion to explicitly handle the zero-item case by
failing or otherwise surfacing that no upgrade target exists before returning,
and keep the existing click-on-first-option behavior only when
console-select-item has entries.
Comment on lines
+107
to
+121
| await test.step('Verify details page tabs and actions (HR-05-TC13)', async () => { | ||
| await helmPage.navigateToHelmReleases(ns); | ||
| await helmPage.searchByName(releaseName); | ||
| await helmPage.clickReleaseName(releaseName); | ||
|
|
||
| await expect(helmDetailsPage.getSectionHeading()).toBeVisible({ timeout: 30_000 }); | ||
| await expect(helmDetailsPage.getResourcesTab()).toBeVisible(); | ||
| await expect(helmDetailsPage.getRevisionHistoryTab()).toBeVisible(); | ||
| await expect(helmDetailsPage.getReleaseNotesTab()).toBeVisible(); | ||
|
|
||
| await helmDetailsPage.clickActionsMenu(); | ||
| await expect(helmDetailsPage.getActionMenuItem('Upgrade')).toBeVisible(); | ||
| await expect(helmDetailsPage.getActionMenuItem('Delete Helm Release')).toBeVisible(); | ||
| await page.keyboard.press('Escape'); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Rollback menu item not verified here.
HelmDetailsPage already exposes verifyActionsInMenu(), which checks all three actions (Upgrade, Rollback, Delete Helm Release). This step manually re-checks only Upgrade and Delete, silently dropping Rollback coverage compared to the existing helper.
🐛 Proposed fix
await helmDetailsPage.clickActionsMenu();
await expect(helmDetailsPage.getActionMenuItem('Upgrade')).toBeVisible();
+ await expect(helmDetailsPage.getActionMenuItem('Rollback')).toBeVisible();
await expect(helmDetailsPage.getActionMenuItem('Delete Helm Release')).toBeVisible();
await page.keyboard.press('Escape');📝 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
| await test.step('Verify details page tabs and actions (HR-05-TC13)', async () => { | |
| await helmPage.navigateToHelmReleases(ns); | |
| await helmPage.searchByName(releaseName); | |
| await helmPage.clickReleaseName(releaseName); | |
| await expect(helmDetailsPage.getSectionHeading()).toBeVisible({ timeout: 30_000 }); | |
| await expect(helmDetailsPage.getResourcesTab()).toBeVisible(); | |
| await expect(helmDetailsPage.getRevisionHistoryTab()).toBeVisible(); | |
| await expect(helmDetailsPage.getReleaseNotesTab()).toBeVisible(); | |
| await helmDetailsPage.clickActionsMenu(); | |
| await expect(helmDetailsPage.getActionMenuItem('Upgrade')).toBeVisible(); | |
| await expect(helmDetailsPage.getActionMenuItem('Delete Helm Release')).toBeVisible(); | |
| await page.keyboard.press('Escape'); | |
| }); | |
| await test.step('Verify details page tabs and actions (HR-05-TC13)', async () => { | |
| await helmPage.navigateToHelmReleases(ns); | |
| await helmPage.searchByName(releaseName); | |
| await helmPage.clickReleaseName(releaseName); | |
| await expect(helmDetailsPage.getSectionHeading()).toBeVisible({ timeout: 30_000 }); | |
| await expect(helmDetailsPage.getResourcesTab()).toBeVisible(); | |
| await expect(helmDetailsPage.getRevisionHistoryTab()).toBeVisible(); | |
| await expect(helmDetailsPage.getReleaseNotesTab()).toBeVisible(); | |
| await helmDetailsPage.clickActionsMenu(); | |
| await expect(helmDetailsPage.getActionMenuItem('Upgrade')).toBeVisible(); | |
| await expect(helmDetailsPage.getActionMenuItem('Rollback')).toBeVisible(); | |
| await expect(helmDetailsPage.getActionMenuItem('Delete Helm Release')).toBeVisible(); | |
| await page.keyboard.press('Escape'); | |
| }); |
🤖 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 107 - 121, The
details-page actions check is missing Rollback coverage because it manually
asserts only Upgrade and Delete. Update the helm release e2e step to use
HelmDetailsPage.verifyActionsInMenu() instead of individual menu-item checks, so
all three actions including Rollback are verified through the existing helper.
Comment on lines
+160
to
+166
| await test.step('Upgrade helm release again (HR-08-TC02)', async () => { | ||
| await helmPage.clickKebabMenu(); | ||
| await helmPage.selectAction('Upgrade'); | ||
| await helmPage.upgradeChartVersion(); | ||
| await helmPage.clickUpgradeButton(); | ||
| await expect(page).toHaveURL(/\/helm\//, { timeout: 30_000 }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '120,220p' frontend/e2e/tests/helm/helm-release.spec.tsRepository: openshift/console
Length of output: 3388
🏁 Script executed:
rg -n "selectRevision|clickRollbackButton|getActionMenuItem|navigateToHelmReleases|searchByName|upgradeChartVersion" frontend/e2e/tests -g '*.ts'Repository: openshift/console
Length of output: 2301
🏁 Script executed:
fd -a "helm*.ts" frontend/e2e/tests frontend/e2e/page-objects frontend/e2eRepository: openshift/console
Length of output: 227
🏁 Script executed:
fd -a "helm*.ts" frontendRepository: openshift/console
Length of output: 316
🏁 Script executed:
rg -n "selectRevision\(" frontendRepository: openshift/console
Length of output: 321
🏁 Script executed:
sed -n '1,220p' frontend/e2e/pages/helm-page.tsRepository: openshift/console
Length of output: 7365
🏁 Script executed:
rg -n "class HelmDetailsPage|getActionMenuItem|clickActionsMenu|getSectionHeading|clickRevisionHistoryTab|getStatusDetails" frontend/e2e/pages frontend/e2e/testsRepository: openshift/console
Length of output: 1893
🏁 Script executed:
sed -n '1,180p' frontend/e2e/pages/helm-details-page.tsRepository: openshift/console
Length of output: 3211
Wait for the second upgrade to settle before rollback
frontend/e2e/tests/helm/helm-release.spec.ts:160-166 only waits for the URL change. Add the same post-upgrade Deployed check used above before opening rollback so selectRevision() doesn’t read a stale revision list.
🐛 Proposed fix
await helmPage.clickUpgradeButton();
await expect(page).toHaveURL(/\/helm\//, { timeout: 30_000 });
+ await helmPage.navigateToHelmReleases(ns);
+ await helmPage.searchByName(releaseName);
+ await expect(helmPage.getStatusText().first()).toContainText('Deployed', { timeout: 60_000 });
});📝 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
| await test.step('Upgrade helm release again (HR-08-TC02)', async () => { | |
| await helmPage.clickKebabMenu(); | |
| await helmPage.selectAction('Upgrade'); | |
| await helmPage.upgradeChartVersion(); | |
| await helmPage.clickUpgradeButton(); | |
| await expect(page).toHaveURL(/\/helm\//, { timeout: 30_000 }); | |
| }); | |
| await test.step('Upgrade helm release again (HR-08-TC02)', async () => { | |
| await helmPage.clickKebabMenu(); | |
| await helmPage.selectAction('Upgrade'); | |
| await helmPage.upgradeChartVersion(); | |
| await helmPage.clickUpgradeButton(); | |
| await expect(page).toHaveURL(/\/helm\//, { timeout: 30_000 }); | |
| await helmPage.navigateToHelmReleases(ns); | |
| await helmPage.searchByName(releaseName); | |
| await expect(helmPage.getStatusText().first()).toContainText('Deployed', { timeout: 60_000 }); | |
| }); |
🤖 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 160 - 166, The
second upgrade flow in helm-release.spec.ts only waits for the URL change, so
the revision list may still be stale when rollback is opened. In the upgrade
step around clickUpgradeButton() and the following expect(page).toHaveURL check,
add the same post-upgrade wait used earlier to confirm the release reaches
Deployed before proceeding. This should be done in the test step that performs
the second upgrade, so selectRevision() sees the refreshed revision list.
Contributor
|
@vikram-raj: all tests passed! 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. |
Member
|
/label docs-approved |
openshift-ci
Bot
added
docs-approved
rhamilto
reviewed
Jul 6, 2026
Comment on lines
+7
to
+20
| '[data-test-section-heading="Helm Release details"]', | ||
| ); | ||
| private readonly resourcesTab = this.page.locator( | ||
| '[data-test-id="horizontal-link-Resources"]', | ||
| ); | ||
| private readonly revisionHistoryTab = this.page.locator( | ||
| '[data-test-id="horizontal-link-Revision history"]', | ||
| ); | ||
| private readonly releaseNotesTab = this.page.locator( | ||
| '[data-test-id="horizontal-link-Release notes"]', | ||
| ); | ||
| private readonly actionsMenuButton = this.page.locator( | ||
| '[data-test-id="actions-menu-button"]', | ||
| ); |
Member
There was a problem hiding this comment.
Legacy data-test-id selectors used directly without adding data-test to React components
These locators all use data-test-id attribute selectors directly:
[data-test-id="horizontal-link-Resources"][data-test-id="horizontal-link-Revision history"][data-test-id="horizontal-link-Release notes"][data-test-id="actions-menu-button"][data-test-id="action-items"][data-test-id="modal-title"]
Same pattern in helm-page.ts: [data-test-id="submit-button"], [data-test-id="reset-button"], [data-test-id="kebab-button"].
The migration guidelines say: when a component only has data-test-id="x", add data-test="x" alongside it in the React source, then use getByTestId("x") in the page object. This PR has no React component changes, so all these selectors use the legacy attribute directly.
This matters because when legacy attributes are eventually removed, all these selectors will break silently. Adding data-test now is a small upfront cost that prevents that.
rhamilto
reviewed
Jul 6, 2026
Comment on lines
+73
to
+79
| for (const action of actions) { | ||
| const item = this.actionItems.locator('li', { hasText: action }); | ||
| await item.scrollIntoViewIfNeeded(); | ||
| } | ||
| } | ||
|
|
||
| getActionMenuItem(actionName: string): Locator { |
Member
There was a problem hiding this comment.
Dead code — verifyActionsInMenu() is never called and has no assertions
This method is defined but never used in the spec. It also only calls scrollIntoViewIfNeeded() without any expect() assertions, so even if it were called it wouldn't verify anything. Remove it.
rhamilto
reviewed
Jul 6, 2026
Comment on lines
+33
to
+36
| async searchByName(name: string): Promise<void> { | ||
| const filterToggle = this.dataViewFilters.locator('.pf-v6-c-menu-toggle').first(); | ||
| await this.robustClick(filterToggle, { timeout: 60_000 }); | ||
| await this.page.locator('.pf-v6-c-menu__list-item', { hasText: 'Name' }).click(); |
Member
There was a problem hiding this comment.
Fragile PatternFly CSS class selectors
.pf-v6-c-menu-toggle and .pf-v6-c-menu__list-item are PatternFly internal class names that will break on major PF version upgrades. These selectors also appear at lines 41, 100, 118, and 133 (.pf-v6-c-button__progress).
Where possible, prefer getByRole() or add data-test attributes. For example, the filter toggle could use getByRole('button') scoped to the filters container, and the menu items could use getByRole('menuitem', { name: 'Name' }).
rhamilto
reviewed
Jul 6, 2026
Comment on lines
+57
to
+63
|
|
||
| await test.step('Cancel creation', async () => { | ||
| await helmPage.getCancelButton().click(); | ||
| }); | ||
| }); | ||
|
|
||
| test('installs Helm Chart, verifies status and details, filters, and manages lifecycle', async ({ |
Member
There was a problem hiding this comment.
Inline selectors belong in page objects
These raw selectors should be encapsulated in HelmPage or HelmDetailsPage:
- Line 59:
page.locator('[data-test="form-title"]') - Line 61:
page.locator('#root_field-group') - Line 148:
page.locator('[data-test-action="Upgrade"]')(and Rollback, Delete at 149-150)
The migration guidelines state: "locators in page objects, test scenarios in specs." These could be helmPage.getFormTitle(), helmPage.getFormSections(), etc.
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.
Analysis / Root cause:
The Helm E2E tests were still using Cypress and needed migration to Playwright as part of the overall test migration effort. Some tests were experiencing flakiness due to timing issues with async operations.
Solution description:
waitForLoadingComplete()call that could cause race conditionsScreenshots / screen recording:
Test setup:
cd frontend && yarn test:e2e --grep @helmTest cases:
Browser conformance:
Additional info:
This PR includes two commits:
Reviewers and assignees:
Summary by CodeRabbit
New Features
Tests