CONSOLE-5279: Migrate secrets e2e tests from Cypress to Playwright#16522
CONSOLE-5279: Migrate secrets e2e tests from Cypress to Playwright#16522Cragsmann wants to merge 2 commits into
Conversation
Migrate 4 Cypress storage test files to Playwright under e2e/tests/console/storage/ and delete the original Cypress files. - create-storage-class: 9 provisioner tests (all passing) - clone: 2 PVC clone tests (skip on non-AWS clusters) - snapshot: 2 VolumeSnapshot tests (skip on non-AWS clusters) - volume-attributes-class: 1 VAC lifecycle test (skip on non-AWS) Add KubernetesClient methods for cluster-scoped custom resources, PVCs, and Deployments. Add ListPage and ModalPage page objects. Fix ListPage filter selector to use data-test-id="item-filter" (PF6 TextInput does not forward data-test to the DOM input). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Migrate 12 secrets tests across 5 spec files to Playwright: - key-value.spec.ts (4 tests): binary/ascii/unicode file secrets, TLS edit - image-pull.spec.ts (3 tests): registry credentials, config file upload, password obfuscation - source.spec.ts (2 tests): basic auth and SSH auth secrets - webhook.spec.ts (1 test): create, regenerate, and delete - add-to-workload.spec.ts (2 tests): env vars and volume mount Adds SecretPage page object and extends KubernetesClient with createSecret, getSecret, and getDeployment methods. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@Cragsmann: This pull request references CONSOLE-5279 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. |
openshift-ci
Bot
added
the
needs-rebase
|
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. |
WalkthroughThis PR migrates end-to-end test coverage from Cypress to Playwright across secret and storage operations. It extends the Kubernetes client with wrapper methods, introduces Playwright page objects, creates shared test fixtures, and adds comprehensive e2e test suites for secret CRUD operations and storage management workflows. ChangesCypress to Playwright Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
openshift-ci
Bot
added
the
kind/cypress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Cragsmann 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 |
|
@Cragsmann: 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
frontend/e2e/tests/console/storage/snapshot.spec.ts (1)
34-36: ⚡ Quick winImprove error handling for VolumeSnapshotClass conflict detection.
The error handling uses string matching (
String(e).includes('409')) to detect HTTP 409 Conflict responses. This is brittle because:
- It will match "409" in any error message, not just status codes
- Different client libraries may format errors differently
Consider checking for a structured error property instead.
♻️ Suggested improvement
.catch((e) => { - if (!String(e).includes('409')) throw e; + if (e?.response?.statusCode !== 409 && e?.statusCode !== 409) throw e; });Note: Adjust the property path based on the actual error structure returned by
k8sClient.createClusterCustomResource().Also applies to: 137-139
🤖 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/console/storage/snapshot.spec.ts` around lines 34 - 36, Replace brittle string matching in the catch blocks that handle VolumeSnapshotClass creation by checking a structured status/code property on the thrown error (e.g., e.status, e.statusCode, e.response?.statusCode or e.code) instead of String(e).includes('409'); specifically update the catch after k8sClient.createClusterCustomResource() calls to test for a 409 numeric status via optional chaining (and only swallow the error for 409), otherwise rethrow the error so non-conflict failures propagate.frontend/e2e/tests/console/storage/create-storage-class.spec.ts (1)
66-72: ⚡ Quick winConsider adding cleanup fallback for cluster-scoped StorageClass resources.
The test deletes the StorageClass via UI action (lines 66-72), but if this fails, the cluster-scoped resource will be left behind and may interfere with subsequent test runs. Consider adding a cleanup step that uses
k8sClient.deleteClusterCustomResource().catch(() => {})after the UI deletion to ensure resources are cleaned up even if the UI flow fails.🤖 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/console/storage/create-storage-class.spec.ts` around lines 66 - 72, The UI deletion step may fail and leave the cluster-scoped StorageClass behind; after the existing test.step that clicks the actions menu, triggers '[data-test-action="Delete StorageClass"]' and uses modal.shouldBeOpened()/submit()/shouldBeClosed(), add a fallback cleanup call to k8sClient.deleteClusterCustomResource(...) (or the appropriate deleteClusterCustomResource helper used in tests) and swallow errors with .catch(() => {}) to ensure the StorageClass is removed if the UI flow fails; place this fallback in a finally/after block following the modal interactions and reference the same StorageClass identifier used by the test.frontend/e2e/tests/console/storage/clone.spec.ts (1)
22-30: ⚡ Quick winConsider using proper Kubernetes types instead of
as any.The
as anytype assertions bypass TypeScript's type checking. If the Kubernetes client methods accept specific resource types, consider defining or importing those types to improve type safety and catch potential errors at compile time.🤖 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/console/storage/clone.spec.ts` around lines 22 - 30, The calls to k8sClient.createPVC and k8sClient.createDeployment use unsafe "as any" casts; replace those with the proper Kubernetes resource types (e.g., V1PersistentVolumeClaim for PVC/PVCGP3 and V1Deployment for testerDeployment or the specific types your k8sClient expects), import those types from `@kubernetes/client-node` (or your k8s typings), and pass the objects typed accordingly (ensure metadata.namespace is typed as string on the resource metadata); update the calls to k8sClient.createPVC(ns, pvcObject) and k8sClient.createDeployment(ns, deploymentObject) using the correct types instead of "as any".frontend/e2e/tests/console/storage/volume-attributes-class.spec.ts (1)
62-62: 💤 Low valueConsider using
ListPage.navigateTo()for consistency.This test uses
page.goto()directly, while other storage tests in this cohort uselistPage.navigateTo()(e.g., clone.spec.ts line 34, snapshot.spec.ts line 40). Using the page object method provides consistency and centralizes navigation logic.♻️ Proposed fix
- await page.goto(`/k8s/ns/${ns}/persistentvolumeclaims/~new/form`); + const listPage = new ListPage(page); + await listPage.navigateTo(`/k8s/ns/${ns}/persistentvolumeclaims/~new/form`);However, note that
ListPageis not instantiated above line 62, so you would need to add:+ const listPage = new ListPage(page); const modal = new ModalPage(page);Based on learnings: Page object patterns centralize navigation methods in base classes to provide consistent navigation behavior across test suites.
🤖 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/console/storage/volume-attributes-class.spec.ts` at line 62, Replace the direct page.goto call with the page-object navigation method: create/instantiate a ListPage (e.g., const listPage = new ListPage(page)) before the navigation and call listPage.navigateTo(`/k8s/ns/${ns}/persistentvolumeclaims/~new/form`) instead of page.goto; also add the necessary import for ListPage at the top of the test file so navigation logic is centralized and consistent with other storage tests using ListPage.navigateTo().frontend/e2e/tests/console/crud/secrets/add-to-workload.spec.ts (1)
19-43: ⚡ Quick winExtract the duplicated Deployment/Secret setup into a shared helper or fixture.
The Deployment manifest (lines 19-37) and Secret manifest (lines 38-43) are duplicated verbatim in the second test (lines 74-98). A small factory (or a fixture under
e2e/fixtures/) parameterized by name/namespace would remove the duplication and keep the two tests focused on their workload-attachment behavior.🤖 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/console/crud/secrets/add-to-workload.spec.ts` around lines 19 - 43, Extract the duplicated Deployment/Secret creation into a reusable helper (e.g., a factory function) and call it from both specs: create a helper that accepts parameters (namespace, deployName, secretName) and internally calls k8sClient.createDeployment and k8sClient.createSecret with the same manifests currently inline, then replace the two verbatim blocks with a call to that helper; reference the existing symbols k8sClient.createDeployment, k8sClient.createSecret, deployName and secretName when implementing and invoking the fixture so tests only assert workload-attachment behavior.frontend/e2e/tests/console/crud/secrets/image-pull.spec.ts (1)
24-32: 💤 Low valueAssert the credential form count before iterating.
If the
create-image-secret-formselector ever changes andcountis0, the loop silently fills nothing and the failure only surfaces later as a confusing dockerconfig assertion. A quickexpect(count).toBe(2)(the two entries created byadd-credentials-button) fails fast with a clear message.🛡️ Suggested guard
const forms = page.locator('[data-test-id="create-image-secret-form"]'); const count = await forms.count(); + expect(count).toBe(2); for (let i = 0; i < count; i++) {🤖 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/console/crud/secrets/image-pull.spec.ts` around lines 24 - 32, Before iterating the forms locator, add a precondition assertion to fail fast when no credential forms are present: call an assertion like expect(count).toBe(2) (or the expected number created by the add-credentials-button) right after computing count so the test fails with a clear message if the '[data-test-id="create-image-secret-form"]' selector returns 0 or an unexpected number; this change should be made near the code using the forms locator and the count variable in the image-pull.spec.ts test.frontend/e2e/tests/console/crud/secrets/key-value.spec.ts (1)
54-114: ⚡ Quick winConsider a table-driven test for the ASCII and Unicode file secrets.
The
creates an ascii file secret(54-83) andcreates a unicode file secret(85-114) tests are structurally identical apart from the fixture filename and content. Parameterizing over a{ name, fixture }table would remove the duplication and make adding further encodings trivial.As per coding guidelines: "Tests should follow a table-driven testing convention similar to Go where applicable".
🤖 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/console/crud/secrets/key-value.spec.ts` around lines 54 - 114, Combine the two nearly identical tests into a single table-driven test that iterates over cases (e.g., [{name: 'ascii', fixture: 'asciisecret.txt'}, {name: 'unicode', fixture: 'unicodesecret.utf8'}]); for each case create a unique namespace and secretName, read fixture content (like unicodeContent/asciiContent currently), perform the same steps using SecretPage methods (clickCreateSecretDropdownButton, fillName, page.getByTestId('secret-key').fill, secretPage.uploadFile, expect file-input-textarea, expect file-input-binary-alert hidden, secretPage.save) and then verify via k8sClient.getSecret and Buffer.from(...).toString('utf-8'); ensure you preserve cleanup.trackNamespace(ns) and use the case-specific fixture filename and expected content when asserting.
🤖 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/mocks/storage.ts`:
- Around line 172-185: VAC_LOW_IOPS and VAC_HIGH_IOPS in the storage mocks use
identical parameters so tests that switch a PVC between them don't exercise a
parameter change; update the VAC_HIGH_IOPS mock (symbol: VAC_HIGH_IOPS) to use
distinctly higher IOPS/throughput values than VAC_LOW_IOPS (e.g., increase the
iops and throughput parameters and keep type 'gp3') so assertions comparing
before/after VAC changes actually detect a difference.
- Around line 1-33: The testerDeployment uses volumeDevices (block device) but
the PVC fixture lacks spec.volumeMode and defaults to Filesystem; update the PVC
fixture to include spec.volumeMode: 'Block' so it matches testerDeployment, or
alternatively change testerDeployment to use volumeMounts if a filesystem volume
was intended; locate the PVC fixture used by the tests and add spec.volumeMode:
'Block' (or switch volumeDevices -> volumeMounts in testerDeployment) to keep
volumeMode and mount type consistent.
In `@frontend/e2e/pages/modal-page.ts`:
- Around line 19-21: The method submitShouldBeEnabled currently only waits for
visibility on this.submitButton; change it to assert enabled state by importing
expect from '`@playwright/test`' and use expect(this.submitButton).toBeEnabled()
(optionally after waiting for visible) inside submitShouldBeEnabled so a
visible-but-disabled button fails the test. Ensure you update imports to include
expect and keep the waitFor call if needed for timing.
In `@frontend/e2e/tests/console/storage/create-storage-class.spec.ts`:
- Line 78: The function signature for fillParameter uses an inline type import
import('`@playwright/test`').Page; replace that with a top-level type-only
import—add "import type { Page } from '`@playwright/test`'" at the top of the file
and update the fillParameter signature to use Page (i.e., fillParameter(page:
Page, parameter: Parameter)): Promise<void>; keep Parameter as-is if it's a
runtime type or also use import type if it's a type-only import.
---
Nitpick comments:
In `@frontend/e2e/tests/console/crud/secrets/add-to-workload.spec.ts`:
- Around line 19-43: Extract the duplicated Deployment/Secret creation into a
reusable helper (e.g., a factory function) and call it from both specs: create a
helper that accepts parameters (namespace, deployName, secretName) and
internally calls k8sClient.createDeployment and k8sClient.createSecret with the
same manifests currently inline, then replace the two verbatim blocks with a
call to that helper; reference the existing symbols k8sClient.createDeployment,
k8sClient.createSecret, deployName and secretName when implementing and invoking
the fixture so tests only assert workload-attachment behavior.
In `@frontend/e2e/tests/console/crud/secrets/image-pull.spec.ts`:
- Around line 24-32: Before iterating the forms locator, add a precondition
assertion to fail fast when no credential forms are present: call an assertion
like expect(count).toBe(2) (or the expected number created by the
add-credentials-button) right after computing count so the test fails with a
clear message if the '[data-test-id="create-image-secret-form"]' selector
returns 0 or an unexpected number; this change should be made near the code
using the forms locator and the count variable in the image-pull.spec.ts test.
In `@frontend/e2e/tests/console/crud/secrets/key-value.spec.ts`:
- Around line 54-114: Combine the two nearly identical tests into a single
table-driven test that iterates over cases (e.g., [{name: 'ascii', fixture:
'asciisecret.txt'}, {name: 'unicode', fixture: 'unicodesecret.utf8'}]); for each
case create a unique namespace and secretName, read fixture content (like
unicodeContent/asciiContent currently), perform the same steps using SecretPage
methods (clickCreateSecretDropdownButton, fillName,
page.getByTestId('secret-key').fill, secretPage.uploadFile, expect
file-input-textarea, expect file-input-binary-alert hidden, secretPage.save) and
then verify via k8sClient.getSecret and Buffer.from(...).toString('utf-8');
ensure you preserve cleanup.trackNamespace(ns) and use the case-specific fixture
filename and expected content when asserting.
In `@frontend/e2e/tests/console/storage/clone.spec.ts`:
- Around line 22-30: The calls to k8sClient.createPVC and
k8sClient.createDeployment use unsafe "as any" casts; replace those with the
proper Kubernetes resource types (e.g., V1PersistentVolumeClaim for PVC/PVCGP3
and V1Deployment for testerDeployment or the specific types your k8sClient
expects), import those types from `@kubernetes/client-node` (or your k8s typings),
and pass the objects typed accordingly (ensure metadata.namespace is typed as
string on the resource metadata); update the calls to k8sClient.createPVC(ns,
pvcObject) and k8sClient.createDeployment(ns, deploymentObject) using the
correct types instead of "as any".
In `@frontend/e2e/tests/console/storage/create-storage-class.spec.ts`:
- Around line 66-72: The UI deletion step may fail and leave the cluster-scoped
StorageClass behind; after the existing test.step that clicks the actions menu,
triggers '[data-test-action="Delete StorageClass"]' and uses
modal.shouldBeOpened()/submit()/shouldBeClosed(), add a fallback cleanup call to
k8sClient.deleteClusterCustomResource(...) (or the appropriate
deleteClusterCustomResource helper used in tests) and swallow errors with
.catch(() => {}) to ensure the StorageClass is removed if the UI flow fails;
place this fallback in a finally/after block following the modal interactions
and reference the same StorageClass identifier used by the test.
In `@frontend/e2e/tests/console/storage/snapshot.spec.ts`:
- Around line 34-36: Replace brittle string matching in the catch blocks that
handle VolumeSnapshotClass creation by checking a structured status/code
property on the thrown error (e.g., e.status, e.statusCode,
e.response?.statusCode or e.code) instead of String(e).includes('409');
specifically update the catch after k8sClient.createClusterCustomResource()
calls to test for a 409 numeric status via optional chaining (and only swallow
the error for 409), otherwise rethrow the error so non-conflict failures
propagate.
In `@frontend/e2e/tests/console/storage/volume-attributes-class.spec.ts`:
- Line 62: Replace the direct page.goto call with the page-object navigation
method: create/instantiate a ListPage (e.g., const listPage = new
ListPage(page)) before the navigation and call
listPage.navigateTo(`/k8s/ns/${ns}/persistentvolumeclaims/~new/form`) instead of
page.goto; also add the necessary import for ListPage at the top of the test
file so navigation logic is centralized and consistent with other storage tests
using ListPage.navigateTo().
🪄 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: 07944aa3-94ac-4d21-82f8-b3805af13633
⛔ Files ignored due to path filters (1)
frontend/e2e/fixtures/secrets/binarysecret.binis excluded by!**/*.bin
📒 Files selected for processing (26)
frontend/e2e/clients/kubernetes-client.tsfrontend/e2e/fixtures/secrets/asciisecret.txtfrontend/e2e/fixtures/secrets/unicodesecret.utf8frontend/e2e/mocks/storage.tsfrontend/e2e/pages/base-page.tsfrontend/e2e/pages/list-page.tsfrontend/e2e/pages/modal-page.tsfrontend/e2e/pages/secret-page.tsfrontend/e2e/tests/console/crud/secrets/add-to-workload.spec.tsfrontend/e2e/tests/console/crud/secrets/image-pull.spec.tsfrontend/e2e/tests/console/crud/secrets/key-value.spec.tsfrontend/e2e/tests/console/crud/secrets/source.spec.tsfrontend/e2e/tests/console/crud/secrets/webhook.spec.tsfrontend/e2e/tests/console/storage/clone.spec.tsfrontend/e2e/tests/console/storage/create-storage-class.spec.tsfrontend/e2e/tests/console/storage/snapshot.spec.tsfrontend/e2e/tests/console/storage/volume-attributes-class.spec.tsfrontend/packages/integration-tests/mocks/snapshot.tsfrontend/packages/integration-tests/mocks/storage-class.tsfrontend/packages/integration-tests/mocks/volume-attributes-class.tsfrontend/packages/integration-tests/tests/storage/clone.cy.tsfrontend/packages/integration-tests/tests/storage/create-storage-class.cy.tsfrontend/packages/integration-tests/tests/storage/snapshot.cy.tsfrontend/packages/integration-tests/tests/storage/volume-attributes-class.cy.tsfrontend/packages/integration-tests/views/storage/create-storage-class.tsfrontend/packages/integration-tests/views/storage/snapshot.ts
💤 Files with no reviewable changes (9)
- frontend/packages/integration-tests/tests/storage/create-storage-class.cy.ts
- frontend/packages/integration-tests/views/storage/create-storage-class.ts
- frontend/packages/integration-tests/tests/storage/clone.cy.ts
- frontend/packages/integration-tests/views/storage/snapshot.ts
- frontend/packages/integration-tests/mocks/volume-attributes-class.ts
- frontend/packages/integration-tests/tests/storage/volume-attributes-class.cy.ts
- frontend/packages/integration-tests/mocks/snapshot.ts
- frontend/packages/integration-tests/mocks/storage-class.ts
- frontend/packages/integration-tests/tests/storage/snapshot.cy.ts
| export const testerDeployment = { | ||
| apiVersion: 'apps/v1', | ||
| kind: 'Deployment', | ||
| metadata: { | ||
| name: 'busybox-deployment', | ||
| labels: { app: 'busybox' }, | ||
| }, | ||
| spec: { | ||
| replicas: 1, | ||
| strategy: { type: 'RollingUpdate' }, | ||
| selector: { matchLabels: { app: 'busybox' } }, | ||
| template: { | ||
| metadata: { labels: { app: 'busybox' } }, | ||
| spec: { | ||
| volumes: [ | ||
| { | ||
| name: 'testpvc', | ||
| persistentVolumeClaim: { claimName: 'testpvc' }, | ||
| }, | ||
| ], | ||
| containers: [ | ||
| { | ||
| name: 'busybox', | ||
| image: 'busybox', | ||
| imagePullPolicy: 'IfNotPresent', | ||
| volumeDevices: [{ name: 'testpvc', devicePath: '/data' }], | ||
| command: ['sh', '-c', 'echo Container 1 is Running ; sleep 3600'], | ||
| }, | ||
| ], | ||
| }, | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP 'testerDeployment|volumeMode|volumeDevices' --type=ts -C2Repository: openshift/console
Length of output: 28032
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the PVC fixture in the referenced mock file
nl -ba frontend/e2e/mocks/storage.ts | sed -n '1,120p'
# Check whether other e2e storage mocks/tests set Block volumeMode
rg -n "volumeMode:\s*['\"]Block['\"]|volumeMode:\s*Block|spec:\s*\{[^}]*volumeMode" frontend/e2e -SRepository: openshift/console
Length of output: 104
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== frontend/e2e/mocks/storage.ts (first 120 lines) =="
cat -n frontend/e2e/mocks/storage.ts | sed -n '1,120p'
echo
echo "== volumeMode occurrences in e2e mocks/tests (storage) =="
rg -n "volumeMode\s*:\s*['\"]?Block['\"]?|volumeMode\s*:\s*['\"]?Filesystem['\"]?|volumeDevices|devicePath" frontend/e2e -SRepository: openshift/console
Length of output: 4874
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== volumeDevices usage in e2e storage tests =="
rg -n "volumeDevices|devicePath" frontend/e2e/tests/console/storage -S
echo
echo "== volumeMode usage in e2e storage tests =="
rg -n "volumeMode\s*:\s*['\"]?(Block|Filesystem)['\"]?" frontend/e2e/tests/console/storage -S
echo
echo "== volumeMode usage in e2e mocks (broader, but limited to e2e folder) =="
rg -n "volumeMode\s*:\s*['\"]?(Block|Filesystem)['\"]?" frontend/e2e/mocks -SRepository: openshift/console
Length of output: 108
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "== volumeDevices usage in e2e storage tests =="
rg -n "volumeDevices|devicePath" frontend/e2e/tests/console/storage -S || true
echo
echo "== volumeMode usage in e2e storage tests =="
rg -n "volumeMode\s*:\s*['\"]?(Block|Filesystem)['\"]?" frontend/e2e/tests/console/storage -S || true
echo
echo "== volumeMode usage in e2e mocks (limited to e2e) =="
rg -n "volumeMode\s*:\s*['\"]?(Block|Filesystem)['\"]?" frontend/e2e/mocks -S || trueRepository: openshift/console
Length of output: 207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show where testerDeployment/PVC are used in the storage e2e tests
rg -n "testerDeployment|createDeployment\(|createPVC\(|\bPVC\b" frontend/e2e/tests/console/storage -S
echo
echo "== snapshot.spec.ts (relevant sections) =="
sed -n '1,260p' frontend/e2e/tests/console/storage/snapshot.spec.ts
echo
echo "== clone.spec.ts (relevant sections) =="
sed -n '1,220p' frontend/e2e/tests/console/storage/clone.spec.tsRepository: openshift/console
Length of output: 18799
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== testerDeployment usage across e2e =="
rg -n "testerDeployment" frontend/e2e -SRepository: openshift/console
Length of output: 1402
Align PVC spec.volumeMode with testerDeployment’s volumeDevices usage.
In frontend/e2e/mocks/storage.ts, testerDeployment mounts testpvc via volumeDevices (block-mode), but the PVC fixture omits spec.volumeMode (defaults to Filesystem). This mismatch can prevent the Deployment pod from scheduling/attaching even though the clone/snapshot tests only wait for the PVC to become Bound. Set PVC.spec.volumeMode: 'Block' for this test (or switch testerDeployment to volumeMounts if the intent is filesystem).
🤖 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/mocks/storage.ts` around lines 1 - 33, The testerDeployment uses
volumeDevices (block device) but the PVC fixture lacks spec.volumeMode and
defaults to Filesystem; update the PVC fixture to include spec.volumeMode:
'Block' so it matches testerDeployment, or alternatively change testerDeployment
to use volumeMounts if a filesystem volume was intended; locate the PVC fixture
used by the tests and add spec.volumeMode: 'Block' (or switch volumeDevices ->
volumeMounts in testerDeployment) to keep volumeMode and mount type consistent.
| VAC_LOW_IOPS: { | ||
| apiVersion: 'storage.k8s.io/v1', | ||
| kind: 'VolumeAttributesClass', | ||
| metadata: { name: names.TEST_VAC_LOW_IOPS }, | ||
| driverName: 'ebs.csi.aws.com', | ||
| parameters: { iops: '3000', throughput: '125', type: 'gp3' }, | ||
| }, | ||
| VAC_HIGH_IOPS: { | ||
| apiVersion: 'storage.k8s.io/v1', | ||
| kind: 'VolumeAttributesClass', | ||
| metadata: { name: names.TEST_VAC_HIGH_IOPS }, | ||
| driverName: 'ebs.csi.aws.com', | ||
| parameters: { iops: '3000', throughput: '125', type: 'gp3' }, | ||
| }, |
There was a problem hiding this comment.
VAC_LOW_IOPS and VAC_HIGH_IOPS have identical parameters.
Both define iops: '3000', throughput: '125', type: 'gp3'. Any test that modifies a PVC from the low to the high VAC won't actually exercise a parameter change, so the assertion becomes a no-op. Differentiate the high-IOPS values.
Proposed fix
VAC_HIGH_IOPS: {
apiVersion: 'storage.k8s.io/v1',
kind: 'VolumeAttributesClass',
metadata: { name: names.TEST_VAC_HIGH_IOPS },
driverName: 'ebs.csi.aws.com',
- parameters: { iops: '3000', throughput: '125', type: 'gp3' },
+ parameters: { iops: '6000', throughput: '250', type: 'gp3' },
},📝 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.
| VAC_LOW_IOPS: { | |
| apiVersion: 'storage.k8s.io/v1', | |
| kind: 'VolumeAttributesClass', | |
| metadata: { name: names.TEST_VAC_LOW_IOPS }, | |
| driverName: 'ebs.csi.aws.com', | |
| parameters: { iops: '3000', throughput: '125', type: 'gp3' }, | |
| }, | |
| VAC_HIGH_IOPS: { | |
| apiVersion: 'storage.k8s.io/v1', | |
| kind: 'VolumeAttributesClass', | |
| metadata: { name: names.TEST_VAC_HIGH_IOPS }, | |
| driverName: 'ebs.csi.aws.com', | |
| parameters: { iops: '3000', throughput: '125', type: 'gp3' }, | |
| }, | |
| VAC_LOW_IOPS: { | |
| apiVersion: 'storage.k8s.io/v1', | |
| kind: 'VolumeAttributesClass', | |
| metadata: { name: names.TEST_VAC_LOW_IOPS }, | |
| driverName: 'ebs.csi.aws.com', | |
| parameters: { iops: '3000', throughput: '125', type: 'gp3' }, | |
| }, | |
| VAC_HIGH_IOPS: { | |
| apiVersion: 'storage.k8s.io/v1', | |
| kind: 'VolumeAttributesClass', | |
| metadata: { name: names.TEST_VAC_HIGH_IOPS }, | |
| driverName: 'ebs.csi.aws.com', | |
| parameters: { iops: '6000', throughput: '250', type: 'gp3' }, | |
| }, |
🤖 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/mocks/storage.ts` around lines 172 - 185, VAC_LOW_IOPS and
VAC_HIGH_IOPS in the storage mocks use identical parameters so tests that switch
a PVC between them don't exercise a parameter change; update the VAC_HIGH_IOPS
mock (symbol: VAC_HIGH_IOPS) to use distinctly higher IOPS/throughput values
than VAC_LOW_IOPS (e.g., increase the iops and throughput parameters and keep
type 'gp3') so assertions comparing before/after VAC changes actually detect a
difference.
| async submitShouldBeEnabled(): Promise<void> { | ||
| await this.submitButton.waitFor({ state: 'visible', timeout: 10_000 }); | ||
| } |
There was a problem hiding this comment.
submitShouldBeEnabled checks visibility, not enablement.
The name promises an enabled-state assertion but it only waits for the button to be visible; a visible-but-disabled submit would pass and hide a real regression.
Proposed fix
- async submitShouldBeEnabled(): Promise<void> {
- await this.submitButton.waitFor({ state: 'visible', timeout: 10_000 });
- }
+ async submitShouldBeEnabled(): Promise<void> {
+ await this.submitButton.waitFor({ state: 'visible', timeout: 10_000 });
+ await expect(this.submitButton).toBeEnabled();
+ }Requires importing expect from @playwright/test.
📝 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 submitShouldBeEnabled(): Promise<void> { | |
| await this.submitButton.waitFor({ state: 'visible', timeout: 10_000 }); | |
| } | |
| async submitShouldBeEnabled(): Promise<void> { | |
| await this.submitButton.waitFor({ state: 'visible', timeout: 10_000 }); | |
| await expect(this.submitButton).toBeEnabled(); | |
| } |
🤖 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/modal-page.ts` around lines 19 - 21, The method
submitShouldBeEnabled currently only waits for visibility on this.submitButton;
change it to assert enabled state by importing expect from '`@playwright/test`'
and use expect(this.submitButton).toBeEnabled() (optionally after waiting for
visible) inside submitShouldBeEnabled so a visible-but-disabled button fails the
test. Ensure you update imports to include expect and keep the waitFor call if
needed for timing.
| }, | ||
| ); | ||
|
|
||
| async function fillParameter(page: import('@playwright/test').Page, parameter: Parameter): Promise<void> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use import type syntax for type-only imports.
The inline type import import('@playwright/test').Page should be moved to the top-level imports using the import type syntax for better clarity and to follow TypeScript best practices.
♻️ Proposed fix
At the top of the file:
+import type { Page } from '`@playwright/test`';
import kebabCase from 'lodash/kebabCase.js';
import { test, expect } from '../../../fixtures';Then update the function signature:
-async function fillParameter(page: import('`@playwright/test`').Page, parameter: Parameter): Promise<void> {
+async function fillParameter(page: Page, parameter: Parameter): Promise<void> {As per coding guidelines: "Use import type syntax for type-only imports"
🤖 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/console/storage/create-storage-class.spec.ts` at line 78,
The function signature for fillParameter uses an inline type import
import('`@playwright/test`').Page; replace that with a top-level type-only
import—add "import type { Page } from '`@playwright/test`'" at the top of the file
and update the fillParameter signature to use Page (i.e., fillParameter(page:
Page, parameter: Parameter)): Promise<void>; keep Parameter as-is if it's a
runtime type or also use import type if it's a type-only import.
2 participants
Analysis / Root cause:
Migrate 5 Cypress secret test files (12 tests) to Playwright as part of the broader Playwright e2e migration effort.
Jira: https://redhat.atlassian.net/browse/CONSOLE-5279
Solution description:
Migrates 12 secrets tests across 5 spec files from Cypress to Playwright:
key-value.spec.ts(4 tests): binary/ascii/unicode file secrets, TLS secret editingimage-pull.spec.ts(3 tests): registry credentials CRUD, config file upload, password obfuscationsource.spec.ts(2 tests): basic auth and SSH auth secret CRUDwebhook.spec.ts(1 test): create, regenerate, and delete webhook secretadd-to-workload.spec.ts(2 tests): add secret as environment variables and as volume mountNew files:
e2e/pages/secret-page.ts— page object with methods for secret creation, editing, verification, and add-to-workload modale2e/fixtures/secrets/— binary, ascii, and unicode test fixture filese2e/tests/console/crud/secrets/— 5 spec filesExtends
KubernetesClientwithcreateSecret,getSecret, andgetDeploymentmethods.Each test is self-contained with unique namespaces and automatic cleanup via
cleanup.trackNamespace().Screenshots / screen recording:
Test setup:
Requires a running OpenShift/OKD cluster with
e2e/.envconfigured.Test cases:
All 12 tests validated with 3 consecutive stability runs (0 failures):
Browser conformance:
Additional info:
Part of the Cypress → Playwright migration effort tracked under CONSOLE-5279.
Reviewers and assignees:
Summary by CodeRabbit
Tests
Chores