CONSOLE-5282: Migrate storage e2e tests from Cypress to Playwright#16519
Open
Cragsmann wants to merge 1 commit into
Open
CONSOLE-5282: Migrate storage e2e tests from Cypress to Playwright#16519Cragsmann wants to merge 1 commit into
Cragsmann wants to merge 1 commit 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>
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@Cragsmann: This pull request references CONSOLE-5282 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 migrates CSI storage tests from Cypress integration tests to Playwright e2e, adding Kubernetes API client helpers, page-object infrastructure, storage fixture data, and four comprehensive e2e test suites covering PVC cloning, storage class creation, volume snapshots, and volume attributes class workflows. ChangesStorage E2E Test Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
[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 |
openshift-ci
Bot
added
the
kind/cypress
![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/tests/console/storage/clone.spec.ts (1)
12-141: ⚡ Quick winConsider extracting the shared clone flow to reduce duplication.
The two tests are nearly identical: setup, wait-for-Bound, clone, verify-details, and delete steps are copied verbatim, differing only in the storage-class selection. A small parameterized helper would keep these in sync as the flow evolves.
♻️ Sketch of a shared helper
const runCloneTest = async ( { page, k8sClient, cleanup }: { page: Page; k8sClient: KubernetesClient; cleanup: Cleanup }, nsPrefix: string, storageClass?: string, ) => { const ns = `${nsPrefix}-${Date.now()}`; const pvcName = PVC.metadata.name; const cloneName = `${pvcName}-clone`; const listPage = new ListPage(page); const modal = new ModalPage(page); await test.step('Set up namespace and resources', async () => { await k8sClient.createNamespace(ns); cleanup.trackNamespace(ns); await k8sClient.createPVC(ns, { ...PVC, metadata: { ...PVC.metadata, namespace: ns } } as any); await k8sClient.createPVC(ns, { ...PVCGP3, metadata: { ...PVCGP3.metadata, namespace: ns } } as any); await k8sClient.createDeployment(ns, { ...testerDeployment, metadata: { ...testerDeployment.metadata, namespace: ns } } as any); }); // ...wait-for-Bound, clone (optionally selecting `storageClass`), verify, delete... }; test('creates and deletes a PVC clone', (fixtures) => runCloneTest(fixtures, 'test-clone')); test('creates PVC clone with different storage class', (fixtures) => runCloneTest(fixtures, 'test-clone-sc', 'gp3-csi'));🤖 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 12 - 141, Both tests duplicate the same clone flow; extract a parameterized helper (e.g., runCloneTest) that accepts the fixtures ({ page, k8sClient, cleanup }), an nsPrefix, and optional storageClass, and move the shared steps (namespace/resource setup using PVC, PVCGP3, testerDeployment; wait-for-Bound using ListPage; clone flow using ModalPage and cloneSize; verify via k8sClient.getPVC and page checks; delete) into it; then replace the two tests with calls to runCloneTest(fixtures, 'test-clone') and runCloneTest(fixtures, 'test-clone-sc', 'gp3-csi') to select the alternate storage class when provided.frontend/e2e/clients/kubernetes-client.ts (1)
491-498: 💤 Low valueConsider adding explicit types instead of
as anycast.The
as anycast on line 498 bypasses TypeScript type checking. If the Kubernetes client library types are too strict, consider defining an explicit interface for the patch options to maintain type safety while satisfying the compiler.🔍 Similar pattern at line 542
The same
as anycast pattern appears inpatchDeploymentat line 542. If the types are updated here, consider updating both locations for consistency.🤖 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/clients/kubernetes-client.ts` around lines 491 - 498, Replace the unsafe `as any` cast on the argument passed to `this.coApi.patchClusterCustomObject` by defining and using an explicit options interface (e.g., PatchClusterCustomObjectOptions) that matches the shape required by the k8s client (properties: body, group, name, plural, version, contentType) and use that type for the object literal; then update the call signature to pass the strongly-typed object instead of `as any`. Also apply the same change to the similar `patchDeployment` call to keep both `patchClusterCustomObject` and `patchDeployment` consistent and type-safe.frontend/e2e/tests/console/storage/snapshot.spec.ts (1)
19-56: ⚡ Quick winConsider extracting the shared setup to reduce duplication.
The namespace/PVC/deployment/
VolumeSnapshotClasscreation (Lines 19-37) and the wait-for-Bound + create-snapshot steps (Lines 39-56) are duplicated almost verbatim in the second test (Lines 122-164). A small helper (e.g.,setupSnapshotEnv(k8sClient, cleanup, ns)) would keep the two scenarios in sync and ease future maintenance.🤖 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 19 - 56, Extract the duplicated setup into a single helper like setupSnapshotEnv(k8sClient, cleanup, ns) that performs the namespace creation (k8sClient.createNamespace), PVC creation (k8sClient.createPVC), deployment creation (k8sClient.createDeployment), VolumeSnapshotClass creation (k8sClient.createClusterCustomResource for 'snapshot.storage.k8s.io' v1 volumesnapshotclasses), waits for the PVC to become Bound using listPage.navigateTo + listPage.filterByName + expect on the status locator, and performs the snapshot creation steps (navigate to VolumeSnapshot list, open dropdowns and submit modal). Replace the two duplicated blocks in both tests with calls to this helper, passing any needed identifiers like pvcName or ns so both tests share the same implementation.
🤖 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/console/storage/create-storage-class.spec.ts`:
- Around line 47-49: The option lookup for the provisioner is using
page.getByRole('option', { name: provisionerName }) which does
substring/case-insensitive matching; update the selector to use exact: true so
the test picks the exact provisioner string. Locate the block that clicks
'storage-class-provisioner-dropdown' and replace the getByRole call that uses
provisionerName with one that passes { name: provisionerName, exact: true } to
ensure the correct option is selected.
In `@frontend/e2e/tests/console/storage/snapshot.spec.ts`:
- Around line 104-111: Move the cluster-scoped VolumeSnapshotClass deletion out
of the final test.step and into a failure-safe teardown (e.g., an afterEach hook
or a try-finally surrounding each test) so it always runs even if assertions
fail; call k8sClient.deleteClusterCustomResource for
'snapshot.storage.k8s.io'/'v1'/'volumesnapshotclasses' with
SnapshotClass.metadata.name inside that teardown and handle "already deleted"
cases by catching the error and ignoring 404/NotFound responses. Ensure you
remove the existing deletion from the test.step to avoid double-deletes and keep
the teardown idempotent.
In `@frontend/e2e/tests/console/storage/volume-attributes-class.spec.ts`:
- Around line 31-59: The cluster-scoped resources created via
k8sClient.createClusterCustomResource (STORAGE_CLASS, VAC_LOW_IOPS,
VAC_HIGH_IOPS, VAC_INVALID) are only cleaned up in the test body and will leak
if the test fails early; move deletion of those cluster-scoped resources into a
guaranteed teardown (e.g., an afterAll hook) or wrap the setup/test in a
try/finally so that you call the corresponding
k8sClient.deleteClusterCustomResource for each resource on teardown; keep
cleanup.trackNamespace(ns) for the namespace but ensure cluster-scoped deletes
run in afterAll (or finally) so they always execute even on assertion failures.
---
Nitpick comments:
In `@frontend/e2e/clients/kubernetes-client.ts`:
- Around line 491-498: Replace the unsafe `as any` cast on the argument passed
to `this.coApi.patchClusterCustomObject` by defining and using an explicit
options interface (e.g., PatchClusterCustomObjectOptions) that matches the shape
required by the k8s client (properties: body, group, name, plural, version,
contentType) and use that type for the object literal; then update the call
signature to pass the strongly-typed object instead of `as any`. Also apply the
same change to the similar `patchDeployment` call to keep both
`patchClusterCustomObject` and `patchDeployment` consistent and type-safe.
In `@frontend/e2e/tests/console/storage/clone.spec.ts`:
- Around line 12-141: Both tests duplicate the same clone flow; extract a
parameterized helper (e.g., runCloneTest) that accepts the fixtures ({ page,
k8sClient, cleanup }), an nsPrefix, and optional storageClass, and move the
shared steps (namespace/resource setup using PVC, PVCGP3, testerDeployment;
wait-for-Bound using ListPage; clone flow using ModalPage and cloneSize; verify
via k8sClient.getPVC and page checks; delete) into it; then replace the two
tests with calls to runCloneTest(fixtures, 'test-clone') and
runCloneTest(fixtures, 'test-clone-sc', 'gp3-csi') to select the alternate
storage class when provided.
In `@frontend/e2e/tests/console/storage/snapshot.spec.ts`:
- Around line 19-56: Extract the duplicated setup into a single helper like
setupSnapshotEnv(k8sClient, cleanup, ns) that performs the namespace creation
(k8sClient.createNamespace), PVC creation (k8sClient.createPVC), deployment
creation (k8sClient.createDeployment), VolumeSnapshotClass creation
(k8sClient.createClusterCustomResource for 'snapshot.storage.k8s.io' v1
volumesnapshotclasses), waits for the PVC to become Bound using
listPage.navigateTo + listPage.filterByName + expect on the status locator, and
performs the snapshot creation steps (navigate to VolumeSnapshot list, open
dropdowns and submit modal). Replace the two duplicated blocks in both tests
with calls to this helper, passing any needed identifiers like pvcName or ns so
both tests share the same implementation.
🪄 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: b7421ca8-28e4-4cc9-9244-c0b333a06ba2
📒 Files selected for processing (18)
frontend/e2e/clients/kubernetes-client.tsfrontend/e2e/mocks/storage.tsfrontend/e2e/pages/base-page.tsfrontend/e2e/pages/list-page.tsfrontend/e2e/pages/modal-page.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/views/storage/create-storage-class.ts
- frontend/packages/integration-tests/mocks/snapshot.ts
- frontend/packages/integration-tests/tests/storage/volume-attributes-class.cy.ts
- frontend/packages/integration-tests/mocks/volume-attributes-class.ts
- frontend/packages/integration-tests/tests/storage/create-storage-class.cy.ts
- frontend/packages/integration-tests/mocks/storage-class.ts
- frontend/packages/integration-tests/tests/storage/clone.cy.ts
- frontend/packages/integration-tests/views/storage/snapshot.ts
- frontend/packages/integration-tests/tests/storage/snapshot.cy.ts
Comment on lines
+47
to
+49
| await page.getByTestId('storage-class-provisioner-dropdown').click(); | ||
| await page.getByTestId('console-select-search-input').locator('input').fill(provisionerName); | ||
| await page.getByRole('option', { name: provisionerName }).click(); |
Contributor
There was a problem hiding this comment.
Use exact: true when selecting the provisioner option.
getByRole('option', { name: provisionerName }) does case-insensitive substring matching by default. If one provisioner name is a substring of another that survives the search filter, this can resolve to multiple options (strict-mode failure) or the wrong one. The dropdown selection at Line 88 already uses exact: true; mirror that here for consistency and safety.
🔧 Proposed fix
- await page.getByRole('option', { name: provisionerName }).click();
+ await page.getByRole('option', { name: provisionerName, exact: true }).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
| await page.getByTestId('storage-class-provisioner-dropdown').click(); | |
| await page.getByTestId('console-select-search-input').locator('input').fill(provisionerName); | |
| await page.getByRole('option', { name: provisionerName }).click(); | |
| await page.getByTestId('storage-class-provisioner-dropdown').click(); | |
| await page.getByTestId('console-select-search-input').locator('input').fill(provisionerName); | |
| await page.getByRole('option', { name: provisionerName, exact: true }).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/tests/console/storage/create-storage-class.spec.ts` around lines
47 - 49, The option lookup for the provisioner is using page.getByRole('option',
{ name: provisionerName }) which does substring/case-insensitive matching;
update the selector to use exact: true so the test picks the exact provisioner
string. Locate the block that clicks 'storage-class-provisioner-dropdown' and
replace the getByRole call that uses provisionerName with one that passes {
name: provisionerName, exact: true } to ensure the correct option is selected.
Comment on lines
+104
to
+111
| await test.step('Clean up cluster-scoped resources', async () => { | ||
| await k8sClient.deleteClusterCustomResource( | ||
| 'snapshot.storage.k8s.io', | ||
| 'v1', | ||
| 'volumesnapshotclasses', | ||
| SnapshotClass.metadata.name, | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Playwright parallelism config and any per-file overrides
fd -t f 'playwright.config.*' frontend/e2e --exec cat {}
rg -nP '(fullyParallel|test\.describe\.(parallel|serial)|workers)' frontend/e2eRepository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Playwright config files under frontend/e2e =="
fd -t f 'playwright.config.*' frontend/e2e | sort | sed 's/^/FILE: /'
echo "== Contents of any playwright.config.* files (limited) =="
for f in $(fd -t f 'playwright.config.*' frontend/e2e | sort); do
echo "--- $f (first 220 lines) ---"
sed -n '1,220p' "$f"
done
echo "== Search for parallel/serial controls under frontend/e2e (config/tests) =="
rg -nP '(fullyParallel|test\.describe\.(parallel|serial)|workers)' frontend/e2e || true
echo "== Inspect snapshot.spec.ts cleanup/teardown context =="
# Print a manageable slice around the referenced line numbers
# (Using sed with guessed line windows; adjust if file shorter)
file="frontend/e2e/tests/console/storage/snapshot.spec.ts"
if [ -f "$file" ]; then
echo "--- $file (lines 1-220) ---"
sed -n '1,220p' "$file"
echo "--- $file (lines 220-520) ---"
sed -n '220,520p' "$file"
else
echo "Missing file: $file" >&2
fiRepository: openshift/console
Length of output: 8126
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find Playwright config files repo-wide =="
fd -t f 'playwright.config.*' . | sort | sed 's/^/FILE: /'
echo "== Search for parallelism controls repo-wide =="
rg -n --hidden --no-ignore -S '(fullyParallel|test\.describe\.configure\(|test\.describe\.parallel|test\.describe\.serial|workers:|maxWorkers|projects:|shard|mode:\s*["'\'']parallel["'\''])' . \
|| true
echo "== Inspect Playwright fixtures used by snapshot.spec.ts =="
# The spec imports ../../../fixtures from frontend/e2e/tests/console/storage
# => frontend/e2e/fixtures.* (but verify existence)
fd -t f 'fixtures.*' frontend/e2e | sort | head -n 50
# Print the fixtures file(s) most likely to contain cleanup teardown
for f in $(fd -t f 'fixtures.*' frontend/e2e | sort); do
echo "--- $f (first 220 lines) ---"
sed -n '1,220p' "$f"
doneRepository: openshift/console
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== frontend/playwright.config.ts (key parallelism-related sections) =="
sed -n '1,200p' frontend/playwright.config.ts
echo "----"
sed -n '200,320p' frontend/playwright.config.ts
echo "----"
rg -n --hidden -S '(fullyParallel|test\.describe\.configure|test\.describe\.(parallel|serial)|mode:\s*["'"'"'](parallel|serial)["'"'"']|workers:|maxWorkers|projects:\s*\[)' frontend/playwright.config.ts
echo "== snapshot.spec.ts parallelism keywords =="
rg -n '(fullyParallel|test\.describe\.configure|test\.describe\.(parallel|serial)|mode:\s*["'"'"'](parallel|serial)["'"'"'])' frontend/e2e/tests/console/storage/snapshot.spec.ts || true
echo "== Locate fixtures entrypoint imported as ../../../fixtures =="
# From frontend/e2e/tests/console/storage/snapshot.spec.ts -> frontend/e2e/tests/fixtures? Actually ../../../fixtures
# We'll resolve by searching for 'export const cleanup' or 'cleanup' fixture in frontend/e2e/fixtures
fd -t f 'fixtures.*' frontend/e2e | sort | head -n 50
fd -t f 'index.*' frontend/e2e/fixtures frontend/e2e/tests frontend/e2e | sort | head -n 50
echo "== Search for cleanup fixture implementation (limit to frontend/e2e) =="
rg -n --hidden -S 'fixture\(\s*['"'"'"]cleanup['"'"'"]|export const cleanup|cleanup:\s*async|cleanup\s*\)' frontend/e2e --glob '*.{ts,js,mjs,cjs}' || trueRepository: openshift/console
Length of output: 4485
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="frontend/e2e/fixtures/index.ts"
echo "== $file (cleanup + trackNamespace sections) =="
# show around cleanup fixture
sed -n '1,140p' "$file"
echo "----"
rg -n --hidden -S 'trackNamespace|cleanup|deleteClusterCustomResource|deleteCluster|cluster-scoped|VolumeSnapshotClass|volumesnapshotclasses' "$file" || trueRepository: openshift/console
Length of output: 2682
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="frontend/e2e/fixtures/cleanup-fixture.ts"
echo "== $file =="
wc -l "$file"
sed -n '1,220p' "$file"
echo "----"
sed -n '220,520p' "$file" || true
echo "----"
rg -n --hidden -S 'trackNamespace|count|shouldSkipCleanup|executeCleanup|deleteClusterCustomResource|volumesnapshotclasses|SnapshotClass|VolumeSnapshotClass|cluster-scoped' frontend/e2e/fixtures/cleanup-fixture.tsRepository: openshift/console
Length of output: 5338
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden -S '(fullyParallel|test\.describe\.configure\(|mode:\s*["'"'"']parallel["'"'"'])' frontend/e2e/tests --glob '*.{ts,js,mjs,cjs}' || trueRepository: openshift/console
Length of output: 43
Move VolumeSnapshotClass deletion into failure-safe teardown.
Right now the cluster-scoped volumesnapshotclasses (SnapshotClass.metadata.name) is deleted only in the last test.step, so any earlier assertion/wait failure prevents the delete and leaves the VSC behind (the cleanup fixture only tracks namespaces and won’t delete an untracked cluster-scoped resource).
test.describe parallel mode (fullyParallel / mode: 'parallel') isn’t used here, and CI runs with workers: 1, so the shared-VSC teardown race between the two tests is unlikely in CI; the primary fix is still making teardown run via afterEach/try-finally and handling “already deleted” cases (e.g., ignore 404).
🤖 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 104 - 111,
Move the cluster-scoped VolumeSnapshotClass deletion out of the final test.step
and into a failure-safe teardown (e.g., an afterEach hook or a try-finally
surrounding each test) so it always runs even if assertions fail; call
k8sClient.deleteClusterCustomResource for
'snapshot.storage.k8s.io'/'v1'/'volumesnapshotclasses' with
SnapshotClass.metadata.name inside that teardown and handle "already deleted"
cases by catching the error and ignoring 404/NotFound responses. Ensure you
remove the existing deletion from the test.step to avoid double-deletes and keep
the teardown idempotent.
Comment on lines
+31
to
+59
| await test.step('Set up namespace and cluster-scoped resources', async () => { | ||
| await k8sClient.createNamespace(ns); | ||
| cleanup.trackNamespace(ns); | ||
| await k8sClient.createClusterCustomResource( | ||
| 'storage.k8s.io', | ||
| 'v1', | ||
| 'storageclasses', | ||
| STORAGE_CLASS as any, | ||
| ); | ||
| await k8sClient.createClusterCustomResource( | ||
| 'storage.k8s.io', | ||
| 'v1', | ||
| 'volumeattributesclasses', | ||
| VAC_LOW_IOPS as any, | ||
| ); | ||
| await k8sClient.createClusterCustomResource( | ||
| 'storage.k8s.io', | ||
| 'v1', | ||
| 'volumeattributesclasses', | ||
| VAC_HIGH_IOPS as any, | ||
| ); | ||
| await k8sClient.createClusterCustomResource( | ||
| 'storage.k8s.io', | ||
| 'v1', | ||
| 'volumeattributesclasses', | ||
| VAC_INVALID as any, | ||
| ); | ||
| await k8sClient.createDeployment(ns, getDeployment(ns, TEST_PVC) as any); | ||
| }); |
Contributor
There was a problem hiding this comment.
Cluster-scoped resources will leak if test fails before cleanup step.
The test creates cluster-scoped resources (StorageClass and VolumeAttributesClasses) in the setup step but only cleans them up in a test step at the end. If any assertion fails before line 137, these cluster-scoped resources will remain in the cluster because:
- They are not namespace-scoped (won't be deleted with the namespace).
- The cleanup fixture only tracks the namespace (line 33), not the cluster-scoped resources.
- The manual cleanup step (lines 137-153) is part of the test body, not a guaranteed teardown hook.
Move cluster-scoped resource cleanup to an afterAll hook or use a try/finally pattern to ensure cleanup runs even on test failure. Based on learnings, manual suite setup/teardown using afterAll with a KubernetesClient instance is the documented pattern for resources that need guaranteed cleanup.
♻️ Recommended fix: Move cleanup to afterAll
test.describe('VolumeAttributesClass E2E tests', { tag: ['`@admin`', '`@storage`'] }, () => {
test.skip(!isAws, 'Requires AWS platform with EBS CSI driver');
+ let clusterResources: {
+ vacNames: string[];
+ storageClassName: string;
+ } | null = null;
+
+ test.afterAll(async ({ k8sClient }) => {
+ if (!clusterResources) return;
+
+ const { vacNames, storageClassName } = clusterResources;
+
+ for (const vacName of vacNames) {
+ await k8sClient
+ .patchClusterCustomResource('storage.k8s.io', 'v1', 'volumeattributesclasses', vacName, {
+ metadata: { finalizers: [] },
+ })
+ .catch(() => {});
+ }
+ for (const vacName of vacNames) {
+ await k8sClient
+ .deleteClusterCustomResource('storage.k8s.io', 'v1', 'volumeattributesclasses', vacName)
+ .catch(() => {});
+ }
+ await k8sClient
+ .deleteClusterCustomResource('storage.k8s.io', 'v1', 'storageclasses', storageClassName)
+ .catch(() => {});
+ });
+
test('creates PVC with VAC, modifies VAC, and handles invalid VAC', async ({
page,
k8sClient,
cleanup,
}) => {
const ns = `test-vac-${Date.now()}`;
const fixtures = getVACFixtures(ns);
const {
VAC_LOW_IOPS,
VAC_HIGH_IOPS,
VAC_INVALID,
STORAGE_CLASS,
TEST_VAC_LOW_IOPS,
TEST_VAC_HIGH_IOPS,
TEST_VAC_INVALID,
TEST_PVC,
TEST_STORAGECLASS,
getDeployment,
} = fixtures;
const modal = new ModalPage(page);
+ clusterResources = {
+ vacNames: [TEST_VAC_LOW_IOPS, TEST_VAC_HIGH_IOPS, TEST_VAC_INVALID],
+ storageClassName: TEST_STORAGECLASS,
+ };
+
await test.step('Set up namespace and cluster-scoped resources', async () => {
// ... existing setup code ...
});
// ... rest of test steps ...
- await test.step('Clean up cluster-scoped resources', async () => {
- for (const vacName of [TEST_VAC_LOW_IOPS, TEST_VAC_HIGH_IOPS, TEST_VAC_INVALID]) {
- await k8sClient
- .patchClusterCustomResource('storage.k8s.io', 'v1', 'volumeattributesclasses', vacName, {
- metadata: { finalizers: [] },
- })
- .catch(() => {});
- }
- for (const vacName of [TEST_VAC_LOW_IOPS, TEST_VAC_HIGH_IOPS, TEST_VAC_INVALID]) {
- await k8sClient
- .deleteClusterCustomResource('storage.k8s.io', 'v1', 'volumeattributesclasses', vacName)
- .catch(() => {});
- }
- await k8sClient
- .deleteClusterCustomResource('storage.k8s.io', 'v1', 'storageclasses', TEST_STORAGECLASS)
- .catch(() => {});
- });
});
});Also applies to: 137-153
🤖 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` around
lines 31 - 59, The cluster-scoped resources created via
k8sClient.createClusterCustomResource (STORAGE_CLASS, VAC_LOW_IOPS,
VAC_HIGH_IOPS, VAC_INVALID) are only cleaned up in the test body and will leak
if the test fails early; move deletion of those cluster-scoped resources into a
guaranteed teardown (e.g., an afterAll hook) or wrap the setup/test in a
try/finally so that you call the corresponding
k8sClient.deleteClusterCustomResource for each resource on teardown; keep
cleanup.trackNamespace(ns) for the namespace but ensure cluster-scoped deletes
run in afterAll (or finally) so they always execute even on assertion failures.
Contributor
|
@Cragsmann: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Analysis / Root cause:
Migrate 4 Cypress storage test files (14 tests) to Playwright as part of the console e2e migration effort (CONSOLE-5282).
Solution description:
create-storage-class.cy.ts→ 9 provisioner-parameterized tests (all passing)clone.cy.ts→ 2 PVC clone tests (skip on non-AWS)snapshot.cy.ts→ 2 VolumeSnapshot tests (skip on non-AWS)volume-attributes-class.cy.ts→ 1 VAC lifecycle test (skip on non-AWS)ListPageandModalPagepage objectsKubernetesClientwith cluster-scoped custom resource, PVC, and Deployment methodsListPagefilter selector: PF6TextInputdoes not forwarddata-testto the DOM input, so usedata-test-id="item-filter"insteadScreenshots / screen recording:
N/A (e2e test migration, no UI changes)
Test setup:
Requires
BRIDGE_AWS=trueine2e/.envand an AWS cluster with EBS CSI driver for clone, snapshot, and VAC tests. Thecreate-storage-classtests run on any platform.Test cases:
create-storage-class: 9 tests validated passing on GCP clusterclone,snapshot,volume-attributes-class: correctly skip on non-AWS clusters viaBRIDGE_AWSenv varBrowser conformance:
N/A (e2e test migration, no UI changes)
Additional info:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests