[CONSOLE-5237] Migrate OLM Cypress tests to Playwright - #16899
Open
trgeiger wants to merge 3 commits into
Open
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
needs-rebase
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: trgeiger The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
component/olm
Contributor
WalkthroughThe PR expands Kubernetes helpers, adds reusable OLM cleanup utilities, introduces Playwright page objects, and adds end-to-end coverage for catalog, installation, operand, uninstall, and PackageManifest workflows. It also adds test selectors and updates existing CRUD tests. ChangesOLM E2E coverage
Console testability and regressions
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant OperatorInstallPage
participant InstalledOperatorsPage
participant KubernetesClient
Admin->>OperatorInstallPage: configure and submit installation
OperatorInstallPage->>InstalledOperatorsPage: open installed operators
InstalledOperatorsPage->>KubernetesClient: poll operator status
KubernetesClient-->>InstalledOperatorsPage: return installation status
InstalledOperatorsPage-->>Admin: show operator details
Admin->>KubernetesClient: create or delete operand
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ 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 |
![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: 20
🧹 Nitpick comments (28)
frontend/e2e/clients/kubernetes-client.ts (3)
661-685: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog the swallowed errors.
Both methods return
[]for every failure, including RBAC denials and network errors. The callers infrontend/e2e/test-utils/operator-cleanup.tsandfrontend/e2e/test-utils/olm-test-cleanup.tsthen treat the empty result as "nothing to clean up", so cleanup silently becomes a no-op. The callers already wrap these calls intry/catch, so logging the error keeps the current control flow and makes the failure visible in the test output.♻️ Proposed refactor
return (response as any)?.items || []; - } catch { + } catch (err) { + console.log(`listClusterCustomResources(${group}/${version}/${plural}) failed: ${err}`); return []; } } async listNamespaces(): Promise<unknown[]> { try { const response = await this.k8sApi.listNamespace(); return (response?.items || []); - } catch { + } catch (err) { + console.log(`listNamespaces failed: ${err}`); return []; } }🤖 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 661 - 685, Update listClusterCustomResources and listNamespaces to log the caught error before returning the existing empty-array fallback. Preserve the current return behavior and use the client’s established logging mechanism so RBAC, network, and other failures are visible to test callers.
282-288: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the placeholder fallback value.
getCurrentUser()returns{ name: 'idk' }when kubeconfig retrieval fails. A caller cannot distinguish this fake user from a real one. Returnundefinedinstead, and use the typedk8s.Userreturn type for consistency withgetCurrentUserToken().♻️ Proposed refactor
- getCurrentUser(): any { + getCurrentUser(): k8s.User { try { return this.kubeConfig.getCurrentUser(); } catch { - return { name: 'idk' }; + return undefined; } }🤖 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 282 - 288, Update getCurrentUser() to return undefined when kubeConfig.getCurrentUser() throws instead of the placeholder user object, and change its return type from any to the typed k8s.User-compatible optional return type used consistently with getCurrentUserToken().Source: Learnings
571-586: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
k8s.PatchStrategy.JsonPatchinpatchClusterCustomResource.
patchCustomResourcealready uses the client enum for the JSON Patch content type. Use the same value here to avoid duplicating the literal'application/json-patch+json'.♻️ Proposed refactor
body: patch, - contentType: 'application/json-patch+json', + contentType: k8s.PatchStrategy.JsonPatch, } as any);🤖 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 571 - 586, Update patchClusterCustomResource to use k8s.PatchStrategy.JsonPatch for the contentType value, matching the existing patchCustomResource implementation and removing the duplicated literal.Source: Linters/SAST tools
frontend/e2e/test-utils/cluster-cleanup.ts (2)
154-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the operand types configurable and log the swallowed error.
Two points in this block:
- The operand types are hardcoded to
infinispan.org. The function accepts a configurabletargetOperator, so the operand types should also come fromClusterCleanupOptions. Otherwise this "comprehensive" utility only cleans one operator's operands.- The
catchon Lines 182-184 discards every error. A missing CRD and an RBAC denial produce the same silent result. Log the error so a failed cleanup is visible in the test output.♻️ Proposed refactor
export interface ClusterCleanupOptions { dryRun?: boolean; targetOperator?: string; olderThanMinutes?: number; + operandTypes?: { group: string; version: string; plural: string }[]; }- const operandTypes = [ - { group: 'infinispan.org', version: 'v1', plural: 'infinispans' }, - { group: 'infinispan.org', version: 'v1', plural: 'backups' }, - ]; + const operandTypes = options.operandTypes ?? [ + { group: 'infinispan.org', version: 'v1', plural: 'infinispans' }, + { group: 'infinispan.org', version: 'v1', plural: 'backups' }, + ];- } catch (error) { - // Ignore - operand type may not exist - } + } catch (error) { + console.log( + ` Skipping ${operandType.plural} in ${namespaceName}: ${error.message}`, + ); + }🤖 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/test-utils/cluster-cleanup.ts` around lines 154 - 185, Update the operand cleanup flow around operandTypes to derive the operand group, version, and plurals from the configurable targetOperator in ClusterCleanupOptions instead of hardcoding infinispan.org. In the catch block surrounding listCustomResources and deleteCustomResource, log the caught error with enough context to identify the operand type and namespace while preserving the existing cleanup continuation behavior.
59-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated resource cleanup block into a helper.
The CSV, Subscription, and InstallPlan blocks are identical except for the plural name and the match predicate. A single helper removes about 70 duplicated lines and gives one place to fix the logging and error handling.
♻️ Proposed refactor
+ const cleanupResourceType = async ( + plural: string, + version: string, + matches: (item: any) => boolean, + ): Promise<void> => { + try { + const items = await k8sClient.listCustomResources( + 'operators.coreos.com', + version, + namespaceName, + plural, + ); + const targets = (items || []).filter(matches); + console.log(`Found ${targets.length} ${targetOperator} ${plural} in ${namespaceName}`); + for (const item of targets) { + console.log(` ${dryRun ? 'Would delete' : 'Deleting'} ${plural}: ${item.metadata.name}`); + if (!dryRun) { + await k8sClient.deleteCustomResource( + 'operators.coreos.com', + version, + namespaceName, + plural, + item.metadata.name, + ); + } + } + } catch (error) { + console.log(` Error checking ${plural} in ${namespaceName}: ${error.message}`); + } + };Then call it for each type:
await cleanupResourceType('clusterserviceversions', 'v1alpha1', (csv) => Boolean(csv.metadata.name?.includes(targetOperator)), ); await cleanupResourceType( 'subscriptions', 'v1alpha1', (sub) => Boolean(sub.metadata.name?.includes(targetOperator)) || Boolean(sub.spec?.name?.includes(targetOperator)), ); await cleanupResourceType('installplans', 'v1alpha1', (ip) => (ip.spec?.clusterServiceVersionNames || []).some((n: string) => n.includes(targetOperator)), );🤖 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/test-utils/cluster-cleanup.ts` around lines 59 - 151, Extract the repeated CSV, Subscription, and InstallPlan cleanup logic into a shared cleanupResourceType helper that accepts the resource plural, API version, and match predicate, while preserving dry-run behavior, logging, listing, deletion, and error handling. Replace the three inline try/catch blocks with calls to this helper using the existing resource-specific predicates and targetOperator matching.frontend/e2e/test-utils/operator-cleanup.ts (3)
98-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMatch predicate differs from the other cleanup modules.
This module matches subscriptions with
===.frontend/e2e/test-utils/olm-test-cleanup.ts(Line 119) andfrontend/e2e/test-utils/olm-cleanup.ts(Line 40) match withincludesandstartsWith. The three modules therefore select different resource sets for the samepackageName.The exact match used here is the safest of the three. Align the other modules to it, or document why each module needs a different matching rule.
🤖 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/test-utils/operator-cleanup.ts` around lines 98 - 100, Align the subscription matching predicates in the cleanup functions of olm-test-cleanup.ts and olm-cleanup.ts with the exact-match behavior used by operator-cleanup.ts, replacing broader includes/startsWith checks where appropriate. If either module must retain broader matching, document the specific reason and intended resource-selection difference.
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the
k8sClientparameter asKubernetesClientin all four cleanup modules. Every new cleanup function acceptsk8sClient: any. The shared root cause is that no module imports the concrete client type, so the compiler cannot check any method name, argument order, or return type. This matters here because the modules call positional helpers such asdeleteCustomResource(group, version, namespace, plural, name); a swappednamespaceandpluralargument compiles today and silently deletes nothing at runtime.KubernetesClientis a default export fromfrontend/e2e/clients/kubernetes-client.tsand already declares every method these modules use.
frontend/e2e/test-utils/operator-cleanup.ts#L13-L13: addimport type KubernetesClient from '../clients/kubernetes-client';and change bothcleanupAllOperatorsByPackageNameandcleanupOperatorResourcesto acceptk8sClient: KubernetesClient.frontend/e2e/test-utils/cluster-cleanup.ts#L15-L15: import the same type and changecleanupClusterTestResourcesto acceptk8sClient: KubernetesClient.frontend/e2e/test-utils/olm-cleanup.ts#L17-L20: import the same type and change bothcleanupOLMOperatorCompletelyandcleanupOperatorWithOLMResources(Line 96) to acceptk8sClient: KubernetesClient.frontend/e2e/test-utils/olm-test-cleanup.ts#L15-L15: import the same type and changeperformOperatorCleanup,performAggressiveOperatorCleanup(Line 47),cleanupTestNamespaces(Line 75), andverifyAndForceCleanup(Line 115) to acceptk8sClient: KubernetesClient. Also type the hook fixtures at Lines 149-171 instead ofany.The list methods return
unknown[], so eachfiltercallback keeps its(item: any)annotation or gains a narrow local interface.🤖 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/test-utils/operator-cleanup.ts` at line 13, Replace the any-typed Kubernetes client parameters with the default-imported KubernetesClient type across frontend/e2e/test-utils/operator-cleanup.ts#L13-L13, frontend/e2e/test-utils/cluster-cleanup.ts#L15-L15, frontend/e2e/test-utils/olm-cleanup.ts#L17-L20, and frontend/e2e/test-utils/olm-test-cleanup.ts#L15-L15: update all named cleanup functions in those files, and type the olm-test-cleanup hook fixtures at Lines 149-171 instead of any. Preserve (item: any) in filter callbacks or use a narrow local interface because KubernetesClient list methods return unknown[].
17-52: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReplace the per-namespace subscription list with the cluster-wide method.
This function currently calls
listCustomResources(..., nsName, ...)for every namespace returned bylistNamespaces(), then silently swallows per-namespace errors.listClusterCustomResources('operators.coreos.com', 'v1alpha1', 'subscriptions')makes one call, and each result includes its namespace, so delete it against thatmetadata.namespace.🤖 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/test-utils/operator-cleanup.ts` around lines 17 - 52, Update the operator cleanup flow to replace the per-namespace loop and listCustomResources calls with a single listClusterCustomResources call for subscriptions. Use each subscription’s metadata.namespace when invoking deleteCustomResource, while preserving matching by operatorPackageName and deletion of all matching subscriptions.frontend/e2e/pages/catalog-page.ts (1)
72-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicate search-input getter.
getSearchInputandgetSearchInputElementreturn the same locator. Keep one accessor to avoid two names for one concept.♻️ Proposed cleanup
getSearchInput(): Locator { return this.searchCatalogInput; } - - getSearchInputElement(): Locator { - return this.searchCatalogInput; - }🤖 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/catalog-page.ts` around lines 72 - 78, Remove the duplicate accessor between getSearchInput and getSearchInputElement in the catalog page object, retaining a single search-input getter and updating any callers to use the retained method.frontend/e2e/pages/operator-install-page.ts (1)
24-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared catalog-to-install-form prologue.
All three install methods repeat the same ten steps: navigate to
/catalog/all-namespaces, select the Operator tab, search, verify and click the card, then verify and click the install button. Extract one private helper and call it from each method. This keeps selector and timeout changes in one place.♻️ Proposed refactor
+ private async openInstallForm(operatorName: string, operatorCardTestID: string): Promise<void> { + await this.goTo('/catalog/all-namespaces'); + await this.catalogPage.clickOperatorTab(); + await this.catalogPage.searchOperators(operatorName); + + const operatorCard = this.page.getByTestId(operatorCardTestID); + await expect(operatorCard).toBeVisible({ timeout: 30_000 }); + await this.robustClick(operatorCard); + + await expect(this.installButton).toBeVisible(); + await expect(this.installButton).toHaveAttribute('href'); + await this.robustClick(this.installButton); + } + async installOperatorGlobally(operatorName: string, operatorCardTestID: string): Promise<void> { - await this.goTo('/catalog/all-namespaces'); - - await this.catalogPage.clickOperatorTab(); - await this.catalogPage.searchOperators(operatorName); - - // Verify operator exists before clicking - const operatorCard = this.page.getByTestId(operatorCardTestID); - await expect(operatorCard).toBeVisible({ timeout: 30_000 }); - await this.robustClick(operatorCard); - - // Wait for install button and verify it has href - await expect(this.installButton).toBeVisible(); - await expect(this.installButton).toHaveAttribute('href'); - await this.robustClick(this.installButton); + await this.openInstallForm(operatorName, operatorCardTestID);🤖 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/operator-install-page.ts` around lines 24 - 38, Extract the repeated catalog-to-install-form flow from installOperatorGlobally and the other two install methods into a private helper: navigate to /catalog/all-namespaces, select the Operator tab, search by operator name, verify and click the operator card, then verify and click the install button. Update all three methods to call this helper, preserving the existing selectors, timeout, and robustClick behavior.frontend/e2e/pages/operator-hub-details-page.ts (1)
78-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the assertions in
toggleSourceAndVerifyto the spec.
toggleSourceAndVerifyperforms the toggle flow and also asserts the modal title and both status values. Keep page objects action-only and assert in the spec file, so failures point at the test intent. Expose the toggle steps and let the spec callexpectongetSourceStatus.Based on learnings: real expectations should be asserted in the spec files (e.g.,
expect(...).toBeVisible()), not in page objects.🤖 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/operator-hub-details-page.ts` around lines 78 - 99, Refactor toggleSourceAndVerify in the operator hub details page object to perform only the source-toggle actions, removing its modal-title and source-status assertions and exposing the necessary steps for callers. Move those expectations into the consuming spec, including both status checks via getSourceStatus and the modal-title checks, so test intent and failures remain in the spec.Source: Learnings
frontend/e2e/pages/operator-details-page.ts (4)
293-299: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove commented-out code.
Lines 294 and 296 contain commented-out selector definitions no longer used. Remove them for clarity.
🤖 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/operator-details-page.ts` around lines 293 - 299, Remove the unused commented-out selector declarations from verifyUninstallAlert, including the alert and modal-title comments, while leaving the dialog visibility and expected-text assertions unchanged.
101-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove commented-out code.
Lines 107-118 contain commented-out navigation logic. Remove it, since it adds no value and clutters the method.
🤖 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/operator-details-page.ts` around lines 101 - 124, Remove the commented-out destructuring and navigation blocks from deleteOperand, including the unused breadcrumb, tab-navigation, and operand-link code, while preserving the URL assertion and deletion flow.
198-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared uninstall-modal setup logic.
uninstallOperatoranduninstallOperatorWithOperandsduplicate the click-page-action, modal-open, title-check, and skeleton-wait sequence. Extract this into a private helper to reduce duplication and the risk of the two methods diverging.🤖 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/operator-details-page.ts` around lines 198 - 228, Extract the shared page-action click, modal-open wait, title assertion, and loading-skeleton wait from uninstallOperator and uninstallOperatorWithOperands into a private helper on the page object. Have both methods call this helper, while preserving their existing submit and delete-all-operands behavior.
51-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared logic between
createOperandandcreateOperandFromTab.
createOperandandcreateOperandFromTabduplicate the same create-form logic.createOperandonly adds a precedingnavigateToOperandTabcall. HavecreateOperandcallnavigateToOperandTaband then delegate tocreateOperandFromTabto avoid future divergence.♻️ Proposed refactor
async createOperand(testOperand: TestOperandProps, isGlobal: boolean = true): Promise<void> { - const { exampleName, createActionID } = testOperand; - await this.navigateToOperandTab(testOperand.name, isGlobal); - - // Verify operand doesn't already exist - await expect(this.getOperandLink(exampleName)).not.toBeAttached(); - - // Click create button - await this.robustClick(this.createItemButton); - - // If specific create action ID is provided, wait for dropdown and click it - if (createActionID) { - // Wait for the dropdown item to be visible before clicking - await expect(this.page.getByTestId(createActionID)).toBeVisible({ timeout: 30_000 }); - await this.robustClick(this.page.getByTestId(createActionID)); - } - - // Verify we're on the create form - await expect(this.page).toHaveURL(/~new/); - - // Fill in the name - await expect(this.nameInput).toBeEnabled(); - await this.nameInput.clear(); - await this.nameInput.fill(exampleName); - - // Submit the form - await this.clickSubmitButton(); - - // Wait for form submission and redirect - await expect(this.page).not.toHaveURL(/~new/, { timeout: 60_000 }); + await this.createOperandFromTab(testOperand); }Also applies to: 139-168
🤖 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/operator-details-page.ts` around lines 51 - 82, Refactor createOperand to only call navigateToOperandTab with the operand name and isGlobal, then delegate the remaining creation flow to createOperandFromTab using the same testOperand argument. Move or reuse the shared create-form logic through createOperandFromTab so both methods cannot diverge.frontend/e2e/tests/olm/operator-install-global.spec.ts (2)
41-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract duplicated cluster-operator cleanup logic.
The
beforeEachandafterEachhooks both listoperators.coreos.comresources, filter byoperatorPackageNamesubstring, and delete each match. Extract this into a shared helper function (for example alongsidecleanupDataGridOperatorResources) to avoid the two copies diverging over time.Also applies to: 91-101
🤖 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/olm/operator-install-global.spec.ts` around lines 41 - 51, Extract the duplicated cluster-operator cleanup flow from the beforeEach and afterEach hooks into a shared helper near cleanupDataGridOperatorResources. Have the helper list operators.coreos.com resources, filter names by operatorPackageName, delete each matching resource, and preserve the existing error logging; replace both hook implementations with calls to this helper.
80-80: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace fixed sleeps with a deterministic wait.
Line 80 uses
page.waitForTimeout(5000)and line 88 uses a rawsetTimeoutpromise for 10 seconds to let cleanup propagate. Fixed sleeps make the suite slower than necessary when cleanup finishes early, and flaky when cleanup takes longer than the fixed duration. Poll for the actual absence of the operator/operand resources instead of waiting a fixed duration.Also applies to: 88-88
🤖 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/olm/operator-install-global.spec.ts` at line 80, Replace the fixed delays in the operator cleanup flow around the waitForTimeout call and raw setTimeout promise with deterministic polling that repeatedly checks until the operator and operand resources are absent. Preserve the cleanup sequencing, but allow the wait to finish immediately when resources disappear and continue until the configured polling timeout when propagation is slow.frontend/e2e/tests/olm/catalog-source-details.spec.ts (1)
72-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompute the shared suffix once.
Line 72 and Line 73 call
Date.now()separately. The two names diverge when the calls straddle a millisecond boundary. Compute the timestamp once and reuse it.♻️ Proposed refactor
- const testNs = `test-catsrc-${Date.now()}`; - const catalogSourceName = `test-catsrc-${Date.now()}`; + const suffix = Date.now(); + const testNs = `test-catsrc-${suffix}`; + const catalogSourceName = `test-catsrc-${suffix}`;🤖 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/olm/catalog-source-details.spec.ts` around lines 72 - 73, Compute the timestamp once before the testNs and catalogSourceName declarations, then reuse that shared value in both template strings so the names always have the same suffix.frontend/e2e/tests/olm/create-namespace.spec.ts (3)
27-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the shadowing loop variable.
Line 28 declares
nsNameinside the loop. It shadows the suite-levelnsNamethat Line 17 assigns and thatafterEachuses at Line 58 and Line 66. The shadowing is easy to misread during later edits. Rename the loop variable tostaleNsName.🤖 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/olm/create-namespace.spec.ts` around lines 27 - 42, Rename the loop-local variable in the test namespace cleanup loop from nsName to staleNsName, and update all references within that loop, including logging, cleanupOperatorResources, deleteNamespace, and the error message. Preserve the suite-level nsName used by afterEach unchanged.
47-48: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReplace the fixed 10 s sleep with a readiness poll.
cleanupAllOperatorsByPackageNamealready sleeps 5 s internally. This adds 10 s more to every test in the suite. The hook also takes thepagefixture only to callwaitForTimeout.Poll the API for the absence of the subscriptions and ClusterServiceVersions instead. Then drop
pagefrom the hook signature.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/e2e/tests/olm/create-namespace.spec.ts` around lines 47 - 48, Replace the fixed waitForTimeout call in the cleanup hook with polling against the API until the targeted subscriptions and ClusterServiceVersions are absent, reusing the existing cleanupAllOperatorsByPackageName context and allowing the poll to time out appropriately. Remove the unused page fixture from the hook signature and its callers while preserving the cleanup behavior.
52-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTake
k8sClientfrom the fixture inafterEach.The hook uses the suite-level
k8sClientvariable thatbeforeEachassigns.operator-install-single-namespace.spec.tsdestructures the fixture directly inafterEach. Follow that pattern here and delete the suite-level variable. The hook then works even whenbeforeEachfails early.♻️ Proposed refactor
- test.afterEach(async () => { + test.afterEach(async ({ k8sClient }) => { console.log('=== CREATE NAMESPACE AFTER EACH: Starting cleanup ===');🤖 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/olm/create-namespace.spec.ts` around lines 52 - 59, Update the create-namespace test cleanup hook to destructure k8sClient directly from the afterEach fixture, matching the pattern in operator-install-single-namespace.spec.ts. Remove the suite-level k8sClient variable and its beforeEach assignment, while continuing to pass the fixture client to cleanupOperatorResources.frontend/e2e/tests/olm/edit-default-sources.spec.ts (1)
26-36: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd an explicit timeout to the status assertions.
After the modal submits, the console patches the
OperatorHubresource and waits for the watch update. The default expect timeout can expire before the status text changes. Set an explicit timeout on both assertions.♻️ Proposed change
- await expect(operatorHubPage.getSourceStatus(defaultSourceToBeToggled)).toHaveText('Disabled'); + await expect(operatorHubPage.getSourceStatus(defaultSourceToBeToggled)).toHaveText( + 'Disabled', + { 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/olm/edit-default-sources.spec.ts` around lines 26 - 36, Update both getSourceStatus(defaultSourceToBeToggled) status assertions in the toggle flow to use an explicit timeout long enough for the OperatorHub watch update after modal submission, preserving the existing Disabled and Enabled expectations.frontend/e2e/pages/installed-operators-page.ts (3)
111-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelete the two
waitForFunctionblocks. They do not wait for what the comments claim.The first block waits for
!input.disabled. The filter input is never disabled, so the predicate is true on the first evaluation. It does not wait for the 250 ms debounce. The followingexpect(...).toBeVisible()performs the real wait.The second block re-checks opacity and visibility.
toBeVisible()already covers that, andaria-busyis checked on the row element only.The comment at Line 111 says the namespace is selected only when it is not
openshift-operators, butselectNamespacealways runs. Update the comment or the logic.♻️ Proposed simplification
- // Select namespace if not openshift-operators + // Scope the list to the target namespace await this.selectNamespace(namespace); await this.filterByName(operatorName); - // Wait for debounce to complete before clicking (filter-toolbar.tsx uses 250ms debounce) - await this.page.waitForFunction(() => { - const input = document.querySelector('[data-test="name-filter-input"]') as HTMLInputElement; - return input && !input.disabled; - }); - // Wait for the operator row to be visible await expect(this.getOperatorRow(operatorName)).toBeVisible({ timeout: 30_000 }); - - // Additional wait to ensure the table row is stable and ready for interaction - await this.page.waitForFunction( - (name) => { - const row = document.querySelector(`[data-test="operator-row-${name}"]`); - if (!row) return false; - // Check that row is fully rendered and stable - const style = window.getComputedStyle(row); - return style.opacity === '1' && style.visibility === 'visible' && !row.hasAttribute('aria-busy'); - }, - operatorName, - { timeout: 10_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/pages/installed-operators-page.ts` around lines 111 - 136, Remove both waitForFunction blocks surrounding getOperatorRow(operatorName), along with their misleading comments, because toBeVisible already provides the necessary wait and visibility check. Also reconcile the namespace comment with the behavior of selectNamespace: either update the comment to state that it always runs or conditionally call selectNamespace only when namespace is not openshift-operators.
63-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the manual polling loop with a web-first assertion.
The loop reimplements Playwright retry logic. It also re-creates the
status-textlocator at Line 72, although the class already holdsstatusTextat Line 11.expect.pollkeeps the fail-fast behavior onFailedand produces better trace output.If the filtered table can render more than one row,
statusElement.textContent()raises a strict-mode error. The current code swallows that error and only fails after 3 minutes. Scope the locator to the operator row to avoid this.♻️ Proposed refactor
- // Debug the status element by polling its text content - const statusElement = this.page.getByTestId('status-text'); - - console.log(`Waiting for ${operatorName} operator status to be 'Succeeded'...`); - - // Poll the status text every 5 seconds and log what we see - let attempts = 0; - const maxAttempts = 36; // 3 minutes worth of 5-second polls - - while (attempts < maxAttempts) { - let currentText: string | null = null; - try { - currentText = await statusElement.textContent({ timeout: 5000 }); - console.log(`Attempt ${attempts + 1}: Status text is "${currentText}"`); - } catch (error) { - console.log(`Attempt ${attempts + 1}: Could not read status text: ${error.message}`); - } - - if (currentText?.includes('Succeeded')) { - console.log('✅ Found "Succeeded" in status text!'); - return; // Success! - } - - if (currentText?.includes('Failed')) { - throw new Error(`Operator installation failed. Status: ${currentText}`); - } - - attempts++; - await this.page.waitForTimeout(5000); // Wait 5 seconds between polls - } - - throw new Error(`Timeout waiting for operator status to be 'Succeeded' after ${maxAttempts * 5} seconds`); + const statusElement = operatorRow.getByTestId('status-text'); + + await expect + .poll( + async () => { + const text = await statusElement.textContent().catch(() => null); + if (text?.includes('Failed')) { + throw new Error(`Operator installation failed. Status: ${text}`); + } + return text ?? ''; + }, + { intervals: [5_000], timeout: 180_000 }, + ) + .toContain('Succeeded');🤖 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/installed-operators-page.ts` around lines 63 - 103, Replace the manual polling in verifyOperatorInstallationSucceeded with Playwright expect.poll, reusing the class-level statusText locator where applicable. Scope statusText to the located operatorRow so multiple table rows cannot cause a strict-mode error, preserve immediate failure when the status contains “Failed,” and assert successful completion when it contains “Succeeded.”
47-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
clickOperatorRowignoresoperatorURLName, which makes the pinned operator version dead data. The parameter is threaded from the spec throughnavigateToOperatorDetailsintoclickOperatorRowand is never read. The pinned version therefore implies a coupling that does not exist.
frontend/e2e/pages/installed-operators-page.ts#L47-L58: remove theoperatorURLNameparameter fromclickOperatorRow, and remove it fromnavigateToOperatorDetails.frontend/e2e/tests/olm/operator-install-single-namespace.spec.ts#L8-L12: remove theurlNamefield fromtestOperatorand drop the argument from thenavigateToOperatorDetailscalls at Lines 129, 157, and 177.🤖 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/installed-operators-page.ts` around lines 47 - 58, Remove the unused operatorURLName parameter from clickOperatorRow and navigateToOperatorDetails in frontend/e2e/pages/installed-operators-page.ts, updating their call chain accordingly. In frontend/e2e/tests/olm/operator-install-single-namespace.spec.ts, remove testOperator.urlName and omit that argument from navigateToOperatorDetails calls at lines 129, 157, and 177.frontend/e2e/tests/olm/packageserver-tabs.spec.ts (1)
12-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConstruct the page objects once per test.
Each
test.stepcreates a newDetailsPageorYamlEditorPagefor the samepage. The test at Line 67 already builds them once outside the steps. Follow that pattern here.♻️ Proposed refactor for the Details tab test
test('renders Details tab correctly', async ({ page }) => { + const detailsPage = new DetailsPage(page); + await test.step('Navigate to PackageManifest Details tab', async () => { - const detailsPage = new DetailsPage(page); await detailsPage.navigateToDetailsUrl(baseUrl); }); await test.step('Verify page title shows package name', async () => { - const detailsPage = new DetailsPage(page); await expect(detailsPage.title).toContainText(packageManifestName); });🤖 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/olm/packageserver-tabs.spec.ts` around lines 12 - 53, The Details and YAML tests recreate page objects inside each test.step; instantiate each test’s DetailsPage or YamlEditorPage once near the start of the test and reuse it across all steps, matching the existing pattern used by the later test.frontend/e2e/tests/olm/operator-uninstall.spec.ts (1)
9-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated Data Grid test data.
testOperatorandtestOperandhere repeat the definitions infrontend/e2e/tests/olm/operator-install-single-namespace.spec.tsat Lines 8-21. OnlycreateActionIDdiffers. Move the shared constants into a helper module underfrontend/e2e/test-utils/and import them in both specs. The pinnedurlNamethen has one owner.🤖 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/olm/operator-uninstall.spec.ts` around lines 9 - 34, Extract the shared Data Grid definitions from testOperator and testOperand in both operator-uninstall.spec.ts and operator-install-single-namespace.spec.ts into a helper module under frontend/e2e/test-utils/. Export and reuse the common operator and operand constants in both specs, while allowing each spec to provide its distinct createActionID; keep the pinned urlName defined only in the helper.frontend/e2e/tests/olm/operator-install-single-namespace.spec.ts (1)
76-92: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winResolve the deferred cleanup decision in
afterEach.The hook only logs when operators remain. The comment at Line 86 leaves the decision open. When the test fails before the UI uninstall step, the Subscription in
openshift-operatorsand the cluster-scopedOperatorobject survive the run. Thecleanupfixture removes only the tracked namespace.Call
cleanupAllOperatorsByPackageNamewhenstillThere.length > 0, and keep the log line for diagnostics. I can prepare that change or open a tracking issue.🤖 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/olm/operator-install-single-namespace.spec.ts` around lines 76 - 92, The afterEach verification must perform deferred cleanup when operators remain after UI uninstall. In the stillThere.length > 0 branch, retain the diagnostic log and call cleanupAllOperatorsByPackageName with the relevant operator package name so both the Subscription and cluster-scoped Operator are removed.
🤖 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/installed-operators-page.ts`:
- Around line 153-156: Update frontend/e2e/pages/installed-operators-page.ts at
lines 153-156 in verifyOperatorNotExists to wait for .loading-skeleton--table to
detach, then call filterByName before asserting the operator row is not
attached; apply the same loading-skeleton wait after selectNamespace and
filterByName in verifyOperatorNotInstalledInNamespace at lines 175-181 before
its negative assertion.
- Around line 197-205: Update the namespaceOption locator in the
installed-operators page flow to use an anchored exact-text match for namespace
rather than substring filtering and first-item selection. Ensure the final
namespace-bar-dropdown assertion also verifies the complete namespace value,
preventing similarly prefixed generated namespaces from being treated as
matches.
In `@frontend/e2e/pages/operator-install-page.ts`:
- Around line 131-134: Update the namespace-radio branch in the operator install
page to use the same bounded wait as installOperatorInNamespace instead of an
immediate selectNamespaceRadio.count() probe. Preserve the conditional
check-and-check behavior while allowing the radio to appear asynchronously.
- Around line 85-88: Update the conditional around selectNamespaceRadio in the
operator-install page to wait for the radio element with a bounded timeout
instead of using count(). Preserve the existing check behavior when the element
becomes available, while allowing the flow to continue if it does not appear
within the timeout.
In `@frontend/e2e/pages/yaml-editor-page.ts`:
- Around line 121-126: Update getEditorContent so the browser evaluation safely
checks that window.monaco and its editor API exist before calling getModels,
returning an empty string when Monaco is not yet initialized; preserve the
existing model-content behavior when the editor is available.
In `@frontend/e2e/test-utils/cluster-cleanup.ts`:
- Around line 26-40: Update the namespace discovery in the cleanup function to
call KubernetesClient.listNamespaces() and use its returned namespace
collection, removing the kubectl child_process imports, execution, JSON parsing,
and obsolete limitation comments. Keep filtering to names beginning with “test-”
and remove the redundant test-operator- predicate before continuing with the
existing cleanup flow.
In `@frontend/e2e/test-utils/olm-cleanup.ts`:
- Around line 112-115: Update the cleanupOLMOperatorCompletely call to always
scope namespacePattern to the caller’s namespace instead of passing undefined
for non-test namespaces. Preserve the existing test-namespace behavior while
ensuring cleanup cannot match resources across the entire cluster.
- Around line 63-71: Update the forceDelete branch in the cleanup deletion catch
block to perform the force deletion using
KubernetesClient.patchClusterCustomResource to strip finalizers, then retry
deletion; otherwise remove or disable forceDelete and log the original error
instead of claiming a retry occurred. Ensure failures in the force path also
surface the relevant error.
- Around line 76-82: Replace the namespacePattern no-op in the cleanup flow with
k8sClient.listNamespaces(), filter results to names starting with test-, and
delete each matching namespace through the client. Remove the stale limitation
note and manual kubectl command, while preserving the existing cleanup
completion behavior.
In `@frontend/e2e/test-utils/olm-test-cleanup.ts`:
- Around line 79-83: Update the testNamespaces predicate in
performOperatorCleanup to match only namespaces owned by this helper, removing
the broad name.includes(packageName) condition while preserving the test- prefix
matching; continue relying on the existing explicit cleanup of globalNamespace
and openshift-marketplace.
- Line 61: Replace the fixed cleanup sleeps in the hooks around
verifyAndForceCleanup and the waits at the referenced cleanup points with
timeout-backed polling using the existing listClusterCustomResources(...,
'operators') call. Continue polling until matching Operator resources disappear,
then force-clean only remaining stragglers, while preserving the existing
timeout behavior and cleanup flow.
In `@frontend/e2e/test-utils/operator-cleanup.ts`:
- Around line 136-141: Update the cleanup flow around the try/catch in the
operator cleanup function so the “✅ Cleanup complete” message is emitted only
after successful cleanup. Move that log into the try block or otherwise prevent
it from running after the catch handles an error, while preserving the existing
error logging.
In `@frontend/e2e/tests/console/crud/other-routes.spec.ts`:
- Around line 141-142: Update the URL assertion around expectedPath to prevent
substring matches by anchoring the regular expression with ^ and $, while
allowing an optional query suffix; alternatively compare the parsed URL pathname
exactly. Preserve the existing route.path query stripping and escaping behavior.
In `@frontend/e2e/tests/olm/create-namespace.spec.ts`:
- Around line 22-45: Update the stale namespace filter in the cleanup block to
match the prefix produced by generateTestNamespace(), such as test-, so leftover
namespaces are discovered and deleted; keep the existing
cleanupOperatorResources and deleteNamespace flow unchanged.
In `@frontend/e2e/tests/olm/edit-default-sources.spec.ts`:
- Around line 19-37: Add an afterEach teardown for the test that uses k8sClient
to patch the OperatorHub cluster resource, ensuring the redhat-operators default
source is enabled even when assertions or toggling fail. Keep the existing
toggle verification flow unchanged and make the cleanup unconditional.
In `@frontend/e2e/tests/olm/operator-hub.spec.ts`:
- Around line 39-53: Validate that originalTileText from
getFirstCatalogTileTitle().textContent() is non-empty before passing it to
verifyTileTextChanged; do not use an empty-string fallback that can make the
comparison vacuously succeed. Preserve the existing Community-to-Certified
filter flow and changed-title assertion once the captured title is confirmed.
- Around line 77-97: Update the empty-search verification in the operator hub
test to use toHaveCount(0) for catalogPage.getCatalogTiles(), avoiding
strict-mode issues with the multi-element locator. Remove the clearButton count
conditional and assert the clear-filters button is visible directly, then click
it and verify the search input is empty and catalog tiles return.
In `@frontend/e2e/tests/olm/operator-install-single-namespace.spec.ts`:
- Around line 48-66: Scope the namespace cleanup around generateTestNamespace to
only namespaces created by this test worker, rather than every name beginning
with test-. Track generated namespace names in the suite (or include and filter
by a unique per-run identifier), then use that owned set when invoking
cleanupOperatorResources; preserve cleanup of all resources within those owned
namespaces.
- Around line 133-144: Replace the empty-state assertion in the isolation
verification step with
InstalledOperatorsPage.verifyOperatorNotInstalledInNamespace, passing the
operator under test and globalNamespace. Keep the existing navigation and
namespace selection, and assert only that this specific operator is absent from
the global namespace.
In `@frontend/e2e/tests/olm/operator-uninstall.spec.ts`:
- Around line 116-137: The uninstall flow in the “Successfully uninstall
operator (without operands)” step conflicts with the created example-backup and
subsequent deletion assertion. Update the step to call
operatorDetailsPage.uninstallOperatorWithOperands() so the operand is explicitly
deleted, and revise the step title/comment to reflect that an operand exists and
is removed.
---
Nitpick comments:
In `@frontend/e2e/clients/kubernetes-client.ts`:
- Around line 661-685: Update listClusterCustomResources and listNamespaces to
log the caught error before returning the existing empty-array fallback.
Preserve the current return behavior and use the client’s established logging
mechanism so RBAC, network, and other failures are visible to test callers.
- Around line 282-288: Update getCurrentUser() to return undefined when
kubeConfig.getCurrentUser() throws instead of the placeholder user object, and
change its return type from any to the typed k8s.User-compatible optional return
type used consistently with getCurrentUserToken().
- Around line 571-586: Update patchClusterCustomResource to use
k8s.PatchStrategy.JsonPatch for the contentType value, matching the existing
patchCustomResource implementation and removing the duplicated literal.
In `@frontend/e2e/pages/catalog-page.ts`:
- Around line 72-78: Remove the duplicate accessor between getSearchInput and
getSearchInputElement in the catalog page object, retaining a single
search-input getter and updating any callers to use the retained method.
In `@frontend/e2e/pages/installed-operators-page.ts`:
- Around line 111-136: Remove both waitForFunction blocks surrounding
getOperatorRow(operatorName), along with their misleading comments, because
toBeVisible already provides the necessary wait and visibility check. Also
reconcile the namespace comment with the behavior of selectNamespace: either
update the comment to state that it always runs or conditionally call
selectNamespace only when namespace is not openshift-operators.
- Around line 63-103: Replace the manual polling in
verifyOperatorInstallationSucceeded with Playwright expect.poll, reusing the
class-level statusText locator where applicable. Scope statusText to the located
operatorRow so multiple table rows cannot cause a strict-mode error, preserve
immediate failure when the status contains “Failed,” and assert successful
completion when it contains “Succeeded.”
- Around line 47-58: Remove the unused operatorURLName parameter from
clickOperatorRow and navigateToOperatorDetails in
frontend/e2e/pages/installed-operators-page.ts, updating their call chain
accordingly. In
frontend/e2e/tests/olm/operator-install-single-namespace.spec.ts, remove
testOperator.urlName and omit that argument from navigateToOperatorDetails calls
at lines 129, 157, and 177.
In `@frontend/e2e/pages/operator-details-page.ts`:
- Around line 293-299: Remove the unused commented-out selector declarations
from verifyUninstallAlert, including the alert and modal-title comments, while
leaving the dialog visibility and expected-text assertions unchanged.
- Around line 101-124: Remove the commented-out destructuring and navigation
blocks from deleteOperand, including the unused breadcrumb, tab-navigation, and
operand-link code, while preserving the URL assertion and deletion flow.
- Around line 198-228: Extract the shared page-action click, modal-open wait,
title assertion, and loading-skeleton wait from uninstallOperator and
uninstallOperatorWithOperands into a private helper on the page object. Have
both methods call this helper, while preserving their existing submit and
delete-all-operands behavior.
- Around line 51-82: Refactor createOperand to only call navigateToOperandTab
with the operand name and isGlobal, then delegate the remaining creation flow to
createOperandFromTab using the same testOperand argument. Move or reuse the
shared create-form logic through createOperandFromTab so both methods cannot
diverge.
In `@frontend/e2e/pages/operator-hub-details-page.ts`:
- Around line 78-99: Refactor toggleSourceAndVerify in the operator hub details
page object to perform only the source-toggle actions, removing its modal-title
and source-status assertions and exposing the necessary steps for callers. Move
those expectations into the consuming spec, including both status checks via
getSourceStatus and the modal-title checks, so test intent and failures remain
in the spec.
In `@frontend/e2e/pages/operator-install-page.ts`:
- Around line 24-38: Extract the repeated catalog-to-install-form flow from
installOperatorGlobally and the other two install methods into a private helper:
navigate to /catalog/all-namespaces, select the Operator tab, search by operator
name, verify and click the operator card, then verify and click the install
button. Update all three methods to call this helper, preserving the existing
selectors, timeout, and robustClick behavior.
In `@frontend/e2e/test-utils/cluster-cleanup.ts`:
- Around line 154-185: Update the operand cleanup flow around operandTypes to
derive the operand group, version, and plurals from the configurable
targetOperator in ClusterCleanupOptions instead of hardcoding infinispan.org. In
the catch block surrounding listCustomResources and deleteCustomResource, log
the caught error with enough context to identify the operand type and namespace
while preserving the existing cleanup continuation behavior.
- Around line 59-151: Extract the repeated CSV, Subscription, and InstallPlan
cleanup logic into a shared cleanupResourceType helper that accepts the resource
plural, API version, and match predicate, while preserving dry-run behavior,
logging, listing, deletion, and error handling. Replace the three inline
try/catch blocks with calls to this helper using the existing resource-specific
predicates and targetOperator matching.
In `@frontend/e2e/test-utils/operator-cleanup.ts`:
- Around line 98-100: Align the subscription matching predicates in the cleanup
functions of olm-test-cleanup.ts and olm-cleanup.ts with the exact-match
behavior used by operator-cleanup.ts, replacing broader includes/startsWith
checks where appropriate. If either module must retain broader matching,
document the specific reason and intended resource-selection difference.
- Line 13: Replace the any-typed Kubernetes client parameters with the
default-imported KubernetesClient type across
frontend/e2e/test-utils/operator-cleanup.ts#L13-L13,
frontend/e2e/test-utils/cluster-cleanup.ts#L15-L15,
frontend/e2e/test-utils/olm-cleanup.ts#L17-L20, and
frontend/e2e/test-utils/olm-test-cleanup.ts#L15-L15: update all named cleanup
functions in those files, and type the olm-test-cleanup hook fixtures at Lines
149-171 instead of any. Preserve (item: any) in filter callbacks or use a narrow
local interface because KubernetesClient list methods return unknown[].
- Around line 17-52: Update the operator cleanup flow to replace the
per-namespace loop and listCustomResources calls with a single
listClusterCustomResources call for subscriptions. Use each subscription’s
metadata.namespace when invoking deleteCustomResource, while preserving matching
by operatorPackageName and deletion of all matching subscriptions.
In `@frontend/e2e/tests/olm/catalog-source-details.spec.ts`:
- Around line 72-73: Compute the timestamp once before the testNs and
catalogSourceName declarations, then reuse that shared value in both template
strings so the names always have the same suffix.
In `@frontend/e2e/tests/olm/create-namespace.spec.ts`:
- Around line 27-42: Rename the loop-local variable in the test namespace
cleanup loop from nsName to staleNsName, and update all references within that
loop, including logging, cleanupOperatorResources, deleteNamespace, and the
error message. Preserve the suite-level nsName used by afterEach unchanged.
- Around line 47-48: Replace the fixed waitForTimeout call in the cleanup hook
with polling against the API until the targeted subscriptions and
ClusterServiceVersions are absent, reusing the existing
cleanupAllOperatorsByPackageName context and allowing the poll to time out
appropriately. Remove the unused page fixture from the hook signature and its
callers while preserving the cleanup behavior.
- Around line 52-59: Update the create-namespace test cleanup hook to
destructure k8sClient directly from the afterEach fixture, matching the pattern
in operator-install-single-namespace.spec.ts. Remove the suite-level k8sClient
variable and its beforeEach assignment, while continuing to pass the fixture
client to cleanupOperatorResources.
In `@frontend/e2e/tests/olm/edit-default-sources.spec.ts`:
- Around line 26-36: Update both getSourceStatus(defaultSourceToBeToggled)
status assertions in the toggle flow to use an explicit timeout long enough for
the OperatorHub watch update after modal submission, preserving the existing
Disabled and Enabled expectations.
In `@frontend/e2e/tests/olm/operator-install-global.spec.ts`:
- Around line 41-51: Extract the duplicated cluster-operator cleanup flow from
the beforeEach and afterEach hooks into a shared helper near
cleanupDataGridOperatorResources. Have the helper list operators.coreos.com
resources, filter names by operatorPackageName, delete each matching resource,
and preserve the existing error logging; replace both hook implementations with
calls to this helper.
- Line 80: Replace the fixed delays in the operator cleanup flow around the
waitForTimeout call and raw setTimeout promise with deterministic polling that
repeatedly checks until the operator and operand resources are absent. Preserve
the cleanup sequencing, but allow the wait to finish immediately when resources
disappear and continue until the configured polling timeout when propagation is
slow.
In `@frontend/e2e/tests/olm/operator-install-single-namespace.spec.ts`:
- Around line 76-92: The afterEach verification must perform deferred cleanup
when operators remain after UI uninstall. In the stillThere.length > 0 branch,
retain the diagnostic log and call cleanupAllOperatorsByPackageName with the
relevant operator package name so both the Subscription and cluster-scoped
Operator are removed.
In `@frontend/e2e/tests/olm/operator-uninstall.spec.ts`:
- Around line 9-34: Extract the shared Data Grid definitions from testOperator
and testOperand in both operator-uninstall.spec.ts and
operator-install-single-namespace.spec.ts into a helper module under
frontend/e2e/test-utils/. Export and reuse the common operator and operand
constants in both specs, while allowing each spec to provide its distinct
createActionID; keep the pinned urlName defined only in the helper.
In `@frontend/e2e/tests/olm/packageserver-tabs.spec.ts`:
- Around line 12-53: The Details and YAML tests recreate page objects inside
each test.step; instantiate each test’s DetailsPage or YamlEditorPage once near
the start of the test and reuse it across all steps, matching the existing
pattern used by the later test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
frontend/e2e/test-utils/olm-cleanup.ts (1)
96-99: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThe namespace filter ignores
namespacePatternand deletes everytest-namespace.Line 93 gates this block on
namespacePattern, but Lines 97-99 filter only on thetest-prefix. The requested pattern is never applied.cleanupOperatorWithOLMResourcespasses a concrete namespace at Line 148, so a caller that asks to clean one namespace deletes alltest-namespaces on the cluster. With parallel Playwright workers this destroys namespaces owned by other tests.Match both the ownership prefix and the requested pattern.
🐛 Proposed fix
const namespaces = await k8sClient.listNamespaces(); - const matchingNamespaces = namespaces.filter((ns: any) => - ns.metadata.name.startsWith('test-') - ); + const matchingNamespaces = namespaces.filter((ns: any) => { + const name: string | undefined = ns?.metadata?.name; + // Only ever delete namespaces this suite owns, and only those the caller asked for. + return Boolean(name?.startsWith('test-') && name.includes(namespacePattern)); + });🤖 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/test-utils/olm-cleanup.ts` around lines 96 - 99, Update the namespace filtering in cleanupOperatorWithOLMResources to require both the existing test- ownership prefix and a match against namespacePattern. Preserve the current behavior when namespacePattern is provided, including restricting cleanup to the concrete namespace requested by callers.
🧹 Nitpick comments (3)
frontend/e2e/test-utils/olm-test-cleanup.ts (1)
184-189: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThe
afterEachwait always exhausts its full 15 seconds.
waitForOperatorCleanupat Line 187 polls until no matching Operator resource remains. It runs beforeperformOperatorCleanupat Line 188, so nothing is deleting the operator during the poll. After a test that installed the operator, the poll runs the whole 15 seconds and then returns the operator anyway. Every such test pays a fixed 15-second penalty.Run the cleanup first, then poll for its effect, as
beforeEachdoes at Lines 179-180.♻️ Proposed change
afterEach: async ({ k8sClient }: any) => { console.log(`=== ${config.packageName.toUpperCase()} AFTER EACH: Starting safety cleanup ===`); - // Give UI operations time to complete with polling - await waitForOperatorCleanup(k8sClient, config.packageName, 15_000); await performOperatorCleanup(k8sClient, config); + // Poll until OLM finishes removing the operator resources. + await waitForOperatorCleanup(k8sClient, config.packageName, 15_000); console.log(`=== ${config.packageName.toUpperCase()} AFTER EACH: Cleanup complete ===`); },🤖 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/test-utils/olm-test-cleanup.ts` around lines 184 - 189, Reorder the cleanup operations in the afterEach hook so performOperatorCleanup(k8sClient, config) runs before waitForOperatorCleanup(k8sClient, config.packageName, 15_000). Preserve the existing logging and polling parameters, matching the cleanup-then-poll order used by beforeEach.frontend/e2e/pages/installed-operators-page.ts (1)
188-198: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe
catchblock can turn a real failure into a passing verification.Line 191 places
filterByNameinside thetry.filterByNamecallsfocus,clear, andfill. If any of those fails after the input became visible, control moves to Line 193 and the method asserts the empty state instead. A detached or disabled filter input then reports "verification passed" at Line 196.Keep only the visibility probe in the
try.♻️ Proposed change
- try { - // Try to wait for the name filter input to be available (when operators exist) - await expect(this.nameFilterInput).toBeVisible({ timeout: 10_000 }); - await this.filterByName(operatorName); - } catch (error) { + const hasFilterInput = await this.nameFilterInput + .waitFor({ state: 'visible', timeout: 10_000 }) + .then(() => true) + .catch(() => false); + + if (!hasFilterInput) { // If no filter input, check for empty state (no operators in this namespace) const emptyState = this.page.getByTestId('console-empty-state'); await expect(emptyState.or(this.page.locator('[data-test="msg-box-title"]'))).toBeVisible({ timeout: 10_000 }); console.log(`No operators found in namespace ${namespace} - verification passed`); return; } + + await this.filterByName(operatorName);🤖 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/installed-operators-page.ts` around lines 188 - 198, Update the try/catch in the operator verification method so it wraps only the nameFilterInput visibility probe; move filterByName(operatorName) after the try/catch. Preserve the empty-state assertion for namespaces where the filter is not visible, while allowing filter interaction failures to propagate instead of being treated as a passing verification.frontend/e2e/test-utils/olm-cleanup.ts (1)
68-74: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
addto clear missing finalizers.JSON Patch
replacerequires the target location to exist. Ifmetadata.finalizersis absent, Kubernetes rejects the patch and the cleanup skips the retry delete. Useop: 'add'withpath: '/metadata/finalizers'and an empty array; this also applies tofrontend/e2e/test-utils/operator-cleanup.tsat the matching force-deletion patch calls.♻️ Proposed change
- [{ op: 'replace', path: '/metadata/finalizers', value: [] }] + [{ op: 'add', path: '/metadata/finalizers', value: [] }]🤖 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/test-utils/olm-cleanup.ts` around lines 68 - 74, Update the force-deletion patch calls in the cleanup logic, including the matching calls in operator-cleanup.ts, to use JSON Patch operation add instead of replace when setting metadata.finalizers to an empty array. Keep the existing path and value so the patch works whether finalizers is present or missing.
🤖 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/installed-operators-page.ts`:
- Line 78: Increase the status polling ceiling controlled by maxAttempts in the
installed-operator wait flow so operator installations have sufficient time to
reach Succeeded on loaded CI clusters. Preserve the existing 5-second polling
interval and fast-feedback behavior, and ensure the assertion after the polling
loop uses the expanded timeout.
In `@frontend/e2e/test-utils/cluster-cleanup.ts`:
- Around line 26-29: Update cleanupClusterTestResources to call listNamespaces
on its received k8sClient parameter instead of the undeclared client variable,
while preserving the existing test-namespace filtering.
In `@frontend/e2e/test-utils/operator-cleanup.ts`:
- Around line 17-21: Replace hardcoded Data Grid predicates with
operatorPackageName-based matching across
frontend/e2e/test-utils/operator-cleanup.ts: quickOperatorCheck (17-21), the
subscription probe (31-34), cleanupAllOperatorsByPackageName
matchingSubscriptions (92-93), matchingOperators (144-149),
forceCleanupAllOperatorsByPackageName matchingOperators (201-206), CSV matching
(238-241), subscription matching (255-258), and cleanupOperatorResources
matchingSubscriptions (418-419). Remove the datagrid, Data Grid, and
datagrid.openshift-operators clauses as specified; keep Data Grid-specific
recovery only in deleteStuckDatagridOperator.
- Around line 299-307: Update the error handling in the operator existence check
around getClusterCustomResource so every non-404 failure also returns
immediately and prevents cluster-wide CRD deletion; only a confirmed existing
datagrid operator should continue to the nuclear cleanup path.
In `@frontend/e2e/tests/olm/operator-hub.spec.ts`:
- Around line 40-44: Update the test around getFirstCatalogTileTitle to resolve
the Locator’s text content into a string before validating or storing it.
Replace the incomplete toHaveText assertion and remove the direct trim call on
the Locator, then pass the resolved title string to the comparison at line 57
while preserving the non-empty title validation.
In `@frontend/e2e/tests/olm/operator-install-global.spec.ts`:
- Around line 40-66: Update cleanupAllOperatorsByPackageName,
forceCleanupAllOperatorsByPackageName, deleteStuckDatagridOperator, and
cleanupOperatorResources to propagate API failures or return explicit failure
results instead of only logging them. Update the beforeEach cleanup flow to
detect those results and fail immediately when the required clean state is not
established, without adding another catch around the existing calls.
- Line 51: Restrict all cleanup in
frontend/e2e/tests/olm/operator-install-global.spec.ts at lines 43, 47, 51, 113,
116, and 119 to resources owned and created by this test, using the existing
cleanup helpers’ ownership scoping; do not invoke cluster-wide CRD deletion
during routine or teardown cleanup, including the stuck-operator path.
---
Duplicate comments:
In `@frontend/e2e/test-utils/olm-cleanup.ts`:
- Around line 96-99: Update the namespace filtering in
cleanupOperatorWithOLMResources to require both the existing test- ownership
prefix and a match against namespacePattern. Preserve the current behavior when
namespacePattern is provided, including restricting cleanup to the concrete
namespace requested by callers.
---
Nitpick comments:
In `@frontend/e2e/pages/installed-operators-page.ts`:
- Around line 188-198: Update the try/catch in the operator verification method
so it wraps only the nameFilterInput visibility probe; move
filterByName(operatorName) after the try/catch. Preserve the empty-state
assertion for namespaces where the filter is not visible, while allowing filter
interaction failures to propagate instead of being treated as a passing
verification.
In `@frontend/e2e/test-utils/olm-cleanup.ts`:
- Around line 68-74: Update the force-deletion patch calls in the cleanup logic,
including the matching calls in operator-cleanup.ts, to use JSON Patch operation
add instead of replace when setting metadata.finalizers to an empty array. Keep
the existing path and value so the patch works whether finalizers is present or
missing.
In `@frontend/e2e/test-utils/olm-test-cleanup.ts`:
- Around line 184-189: Reorder the cleanup operations in the afterEach hook so
performOperatorCleanup(k8sClient, config) runs before
waitForOperatorCleanup(k8sClient, config.packageName, 15_000). Preserve the
existing logging and polling parameters, matching the cleanup-then-poll order
used by beforeEach.
🪄 Autofix
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: Pro Plus
Run ID: 33ebf2b2-f6f0-4f0d-8a54-e838b5ce25f6
📒 Files selected for processing (16)
frontend/e2e/pages/installed-operators-page.tsfrontend/e2e/pages/operator-details-page.tsfrontend/e2e/pages/operator-install-page.tsfrontend/e2e/pages/yaml-editor-page.tsfrontend/e2e/test-utils/cluster-cleanup.tsfrontend/e2e/test-utils/olm-cleanup.tsfrontend/e2e/test-utils/olm-test-cleanup.tsfrontend/e2e/test-utils/operator-cleanup.tsfrontend/e2e/tests/console/crud/other-routes.spec.tsfrontend/e2e/tests/olm/catalog-source-details.spec.tsfrontend/e2e/tests/olm/create-namespace.spec.tsfrontend/e2e/tests/olm/edit-default-sources.spec.tsfrontend/e2e/tests/olm/operator-hub.spec.tsfrontend/e2e/tests/olm/operator-install-global.spec.tsfrontend/e2e/tests/olm/operator-install-single-namespace.spec.tsfrontend/e2e/tests/olm/operator-uninstall.spec.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/e2e/tests/olm/edit-default-sources.spec.ts
- frontend/e2e/tests/olm/operator-uninstall.spec.ts
- frontend/e2e/tests/olm/catalog-source-details.spec.ts
- frontend/e2e/tests/console/crud/other-routes.spec.ts
- frontend/e2e/tests/olm/create-namespace.spec.ts
- frontend/e2e/pages/yaml-editor-page.ts
- frontend/e2e/pages/operator-details-page.ts
- frontend/e2e/tests/olm/operator-install-single-namespace.spec.ts
Comment on lines
+40
to
+44
| const originalTileText = catalogPage.getFirstCatalogTileTitle(); | ||
|
|
||
| // Validate that we captured a valid tile title | ||
| await expect(originalTileText).toHaveText(); | ||
| expect(originalTileText?.trim()).not.toBe(''); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Store the catalog title as text.
getFirstCatalogTileTitle() returns a Playwright Locator. Line 43 omits the required expected text, Line 44 calls trim() on a Locator, and Line 57 passes a Locator to the title comparison. Type checking fails before this test runs.
Proposed fix
- const originalTileText = catalogPage.getFirstCatalogTileTitle();
-
- // Validate that we captured a valid tile title
- await expect(originalTileText).toHaveText();
- expect(originalTileText?.trim()).not.toBe('');
+ const originalTileText = (await catalogPage.getFirstCatalogTileTitle().innerText()).trim();
+ expect(originalTileText).not.toBe('');
@@
- await catalogPage.verifyTileTextChanged(originalTileText!);
+ await catalogPage.verifyTileTextChanged(originalTileText);Also applies to: 57-57
🤖 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/olm/operator-hub.spec.ts` around lines 40 - 44, Update the
test around getFirstCatalogTileTitle to resolve the Locator’s text content into
a string before validating or storing it. Replace the incomplete toHaveText
assertion and remove the direct trim call on the Locator, then pass the resolved
title string to the comparison at line 57 while preserving the non-empty title
validation.
… feedback Key improvements: - Fix conditional delete-all-operands checkbox handling (only appears with multiple operands) - Optimize timeouts for faster failure feedback (reduced from 60-180s to 30-60s) - Add proper Monaco editor safety checks to prevent undefined access - Improve operator cleanup with CRD deletion for stuck operators - Add conditional UI element handling for empty states and missing tabs - Fix createActionID handling for operand creation dropdown selection Most changes address coderabbit feedback for better test reliability, reduced timeout values, and proper error handling in edge cases. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
openshift-ci
Bot
removed
the
needs-rebase
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
trgeiger
marked this pull request as ready for review
August 6, 2026 21:45
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/e2e/pages/operator-details-page.ts`:
- Around line 103-104: Replace the suffix-only RegExp URL assertions with
toHaveURL predicates that compare url.pathname to the expected operand path,
avoiding regex interpretation of valid operand names. Apply this at
frontend/e2e/pages/operator-details-page.ts lines 103-104 and 112-113, and
frontend/e2e/tests/olm/operator-install-single-namespace.spec.ts lines 153-156;
preserve the existing expected paths and click flow.
In `@frontend/e2e/tests/olm/operator-install-single-namespace.spec.ts`:
- Around line 97-107: Move cleanup.trackNamespace(testNamespace) before
installPage.installOperatorInNewNamespace within the “Install operator in new
test namespace” step, ensuring the newly generated namespace is tracked even
when installation fails. Keep the existing ownedNamespaces registration and
installation flow unchanged.
🪄 Autofix
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: Pro Plus
Run ID: ce649e54-ecb6-4abb-9945-59c18453487b
📒 Files selected for processing (38)
frontend/e2e/clients/kubernetes-client.tsfrontend/e2e/pages/catalog-page.tsfrontend/e2e/pages/catalog-source-page.tsfrontend/e2e/pages/details-page.tsfrontend/e2e/pages/installed-operators-page.tsfrontend/e2e/pages/operand-page.tsfrontend/e2e/pages/operator-details-page.tsfrontend/e2e/pages/operator-hub-details-page.tsfrontend/e2e/pages/operator-install-page.tsfrontend/e2e/pages/overview-page.tsfrontend/e2e/pages/yaml-editor-page.tsfrontend/e2e/test-utils/cluster-cleanup.tsfrontend/e2e/test-utils/olm-cleanup.tsfrontend/e2e/test-utils/olm-test-cleanup.tsfrontend/e2e/test-utils/operator-cleanup.tsfrontend/e2e/test-utils/test-namespace.tsfrontend/e2e/tests/console/crud/add-storage-crud.spec.tsfrontend/e2e/tests/console/crud/annotations.spec.tsfrontend/e2e/tests/console/crud/customresourcedefinition.spec.tsfrontend/e2e/tests/console/crud/other-routes.spec.tsfrontend/e2e/tests/console/crud/quotas.spec.tsfrontend/e2e/tests/olm/catalog-source-details.spec.tsfrontend/e2e/tests/olm/create-namespace.spec.tsfrontend/e2e/tests/olm/descriptors.spec.tsfrontend/e2e/tests/olm/edit-default-sources.spec.tsfrontend/e2e/tests/olm/operator-hub.spec.tsfrontend/e2e/tests/olm/operator-install-global.spec.tsfrontend/e2e/tests/olm/operator-install-single-namespace.spec.tsfrontend/e2e/tests/olm/operator-uninstall.spec.tsfrontend/e2e/tests/olm/packageserver-tabs.spec.tsfrontend/packages/console-shared/src/components/catalog/catalog-view/CatalogEmptyState.tsxfrontend/packages/operator-lifecycle-manager/integration-tests/tests/catalog-source-details.cy.tsfrontend/packages/operator-lifecycle-manager/integration-tests/tests/deprecated-operator-warnings.cy.tsfrontend/packages/operator-lifecycle-manager/src/components/modals/edit-default-sources-modal.tsxfrontend/packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.tsxfrontend/packages/operator-lifecycle-manager/src/components/registry-poll-interval-details.tsxfrontend/public/components/utils/details-item.tsxfrontend/public/components/utils/details-page.tsx
💤 Files with no reviewable changes (3)
- frontend/e2e/tests/console/crud/customresourcedefinition.spec.ts
- frontend/packages/operator-lifecycle-manager/integration-tests/tests/deprecated-operator-warnings.cy.ts
- frontend/packages/operator-lifecycle-manager/integration-tests/tests/catalog-source-details.cy.ts
🚧 Files skipped from review as they are similar to previous changes (30)
- frontend/public/components/utils/details-page.tsx
- frontend/packages/operator-lifecycle-manager/src/components/registry-poll-interval-details.tsx
- frontend/e2e/pages/details-page.ts
- frontend/e2e/pages/yaml-editor-page.ts
- frontend/e2e/tests/console/crud/add-storage-crud.spec.ts
- frontend/e2e/test-utils/test-namespace.ts
- frontend/e2e/tests/olm/create-namespace.spec.ts
- frontend/e2e/tests/console/crud/quotas.spec.ts
- frontend/e2e/tests/olm/catalog-source-details.spec.ts
- frontend/e2e/pages/operator-install-page.ts
- frontend/packages/operator-lifecycle-manager/src/components/modals/edit-default-sources-modal.tsx
- frontend/public/components/utils/details-item.tsx
- frontend/e2e/tests/olm/descriptors.spec.ts
- frontend/e2e/pages/overview-page.ts
- frontend/e2e/pages/operator-hub-details-page.ts
- frontend/e2e/tests/console/crud/annotations.spec.ts
- frontend/e2e/test-utils/olm-test-cleanup.ts
- frontend/e2e/tests/olm/edit-default-sources.spec.ts
- frontend/e2e/tests/olm/operator-hub.spec.ts
- frontend/e2e/pages/operand-page.ts
- frontend/e2e/tests/olm/operator-uninstall.spec.ts
- frontend/e2e/test-utils/operator-cleanup.ts
- frontend/e2e/pages/catalog-page.ts
- frontend/e2e/tests/olm/packageserver-tabs.spec.ts
- frontend/e2e/test-utils/olm-cleanup.ts
- frontend/packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.tsx
- frontend/e2e/tests/olm/operator-install-global.spec.ts
- frontend/packages/console-shared/src/components/catalog/catalog-view/CatalogEmptyState.tsx
- frontend/e2e/test-utils/cluster-cleanup.ts
- frontend/e2e/clients/kubernetes-client.ts
Simplify cleanup of installed resources
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/e2e/tests/olm/operator-uninstall.spec.ts (1)
73-80: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRegister the namespace before partial install cleanup can miss it.
buildCreateNamespaceis the only namespace-creation step ininstallOperatorInNewNamespace, and later UI install steps can still fail.cleanup.trackNamespace(testNamespace)is only reached after the helper resolves, so a failure after namespace creation is not tracked. Callcleanup.trackNamespace(testNamespace)beforeinstallOperatorInNewNamespace, or add partial namespace cleanup inside the helper.🤖 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/olm/operator-uninstall.spec.ts` around lines 73 - 80, Move cleanup.trackNamespace(testNamespace) to immediately before installPage.installOperatorInNewNamespace in the “Install operator in new test namespace” test step, ensuring the namespace is registered before any installation work can fail; remove the later registration after the helper resolves.frontend/e2e/test-utils/operator-cleanup.ts (2)
346-380: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
deleteStuckOperatorreturnstrueafter a total failure.The signature declares
Promise<boolean>, and Line 305 returnsfalsefor an indeterminate probe. So callers can branch on the result. But Line 379 returnstrueunconditionally on two failure paths:
- Line 348: the operator still exists after the deletion attempt.
- Line 374: the nuclear cleanup threw, and the finalizer-strip fallback also failed.
Track the outcome and return it.
🐛 Proposed fix
+ let deleted = false; try { await k8sClient.getClusterCustomResource('operators.coreos.com', 'v1', 'operators', operatorName); console.log('⚠️ Operator still exists after deletion attempt'); } catch (error) { if (error.message?.includes('404')) { console.log(`✅ Confirmed: ${operatorName} is gone`); + deleted = true; } else { console.log(`Verification error: ${error.message}`); } } } catch (error) { @@ await k8sClient.deleteClusterCustomResource('operators.coreos.com', 'v1', 'operators', operatorName); console.log(`✅ Successfully force deleted ${operatorName}`); + deleted = true; } catch (forceError) { console.log(`Force deletion also failed: ${forceError.message}`); } } console.log(`💣 deleteStuckOperator FINISHED for ${operatorName}`); - return true; + return deleted; }🤖 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/test-utils/operator-cleanup.ts` around lines 346 - 380, Update deleteStuckOperator to track whether cleanup actually succeeds instead of returning true unconditionally. Set the result to false when verification finds the operator still exists or when both nuclear cleanup and finalizer-stripping fallback fail, preserve true only for confirmed deletion, and return the tracked outcome.
233-253: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThe force path matches CSVs and subscriptions more broadly than the standard path.
cleanupAllOperatorsByPackageNamematches subscriptions with strict equality at Lines 88-91. This force path usesincludeson Lines 251-252 instead. The force path deletes without confirmation, so the wider predicate carries more risk, not less.Line 235 also compares
csv.spec.displayNameagainstoperatorPackageName. A display name is human-readable text, for exampleData Grid. A package name is a slug, for exampledatagrid. This clause rarely matches the intended CSV and can match an unrelated one. Thecsv.metadata.nameclause already covers the real case, because OLM names CSVs<packageName>.v<version>.♻️ Proposed fix
const csvs = await k8sClient.listCustomResources('operators.coreos.com', 'v1alpha1', nsName, 'clusterserviceversions'); const matchingCSVs = csvs.filter((csv: any) => - csv.metadata.name?.includes(operatorPackageName) || - csv.spec?.displayName?.includes(operatorPackageName) + csv.metadata.name === operatorPackageName || + csv.metadata.name?.startsWith(`${operatorPackageName}.`) ); @@ const matchingSubs = subscriptions.filter((sub: any) => - sub.metadata.name?.includes(operatorPackageName) || - sub.spec?.name?.includes(operatorPackageName) + sub.metadata.name === operatorPackageName || + sub.spec?.name === operatorPackageName );🤖 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/test-utils/operator-cleanup.ts` around lines 233 - 253, Align the force-cleanup predicates with cleanupAllOperatorsByPackageName: in the CSV filter, match only CSV metadata.name using the package-name-to-version naming pattern, and remove the csv.spec.displayName comparison; in the subscription filter, replace broad includes checks with the same strict package-name equality used by the standard path. Keep the force deletion flow unchanged.
🤖 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/test-utils/operator-cleanup.ts`:
- Around line 141-144: The operator package matching predicate is duplicated and
uses an overly broad includes check. In
frontend/e2e/test-utils/operator-cleanup.ts#L141-L144, define the shared
isOperatorForPackage helper with exact-name, package-prefixed-name, and
spec.packageName matching, then replace the predicates in
cleanupAllOperatorsByPackageName at `#L141-L144`,
forceCleanupAllOperatorsByPackageName at `#L198-L201`, operatorTestCleanup at
`#L453-L456`, its polling loop at `#L494-L497`, final operator check at `#L516-L519`,
and post-CRD verification at `#L597-L600`; all six sites require the shared helper
so deletion and verification use identical matching.
- Around line 141-144: Update the matchingOperators filter to match operator
names only when operatorPackageName is the complete package-name segment before
the namespace suffix, rather than using includes. Preserve the spec.packageName
exact-match fallback and ensure unrelated names such as datagrid-enterprise are
not selected for deletion.
- Around line 423-425: Update operatorTestCleanup so a failed
cleanupOperatorResources result sets cleanupSuccess to false rather than only
logging the failure. Preserve the existing remaining-operators logging, and
return the accumulated cleanupSuccess value at the end of the try block so
callers can distinguish complete from partial cleanup.
---
Outside diff comments:
In `@frontend/e2e/test-utils/operator-cleanup.ts`:
- Around line 346-380: Update deleteStuckOperator to track whether cleanup
actually succeeds instead of returning true unconditionally. Set the result to
false when verification finds the operator still exists or when both nuclear
cleanup and finalizer-stripping fallback fail, preserve true only for confirmed
deletion, and return the tracked outcome.
- Around line 233-253: Align the force-cleanup predicates with
cleanupAllOperatorsByPackageName: in the CSV filter, match only CSV
metadata.name using the package-name-to-version naming pattern, and remove the
csv.spec.displayName comparison; in the subscription filter, replace broad
includes checks with the same strict package-name equality used by the standard
path. Keep the force deletion flow unchanged.
In `@frontend/e2e/tests/olm/operator-uninstall.spec.ts`:
- Around line 73-80: Move cleanup.trackNamespace(testNamespace) to immediately
before installPage.installOperatorInNewNamespace in the “Install operator in new
test namespace” test step, ensuring the namespace is registered before any
installation work can fail; remove the later registration after the helper
resolves.
🪄 Autofix
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: Pro Plus
Run ID: d62df4d9-7bf6-4158-9648-eddc52125bbf
📒 Files selected for processing (6)
frontend/e2e/pages/operator-details-page.tsfrontend/e2e/test-utils/cluster-cleanup.tsfrontend/e2e/test-utils/operator-cleanup.tsfrontend/e2e/tests/olm/operator-install-global.spec.tsfrontend/e2e/tests/olm/operator-install-single-namespace.spec.tsfrontend/e2e/tests/olm/operator-uninstall.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/e2e/tests/olm/operator-install-single-namespace.spec.ts
- frontend/e2e/test-utils/cluster-cleanup.ts
- frontend/e2e/tests/olm/operator-install-global.spec.ts
- frontend/e2e/pages/operator-details-page.ts
Comment on lines
+141
to
+144
| const matchingOperators = operators.filter((op: any) => | ||
| op.metadata.name?.includes(operatorPackageName) || | ||
| op.spec?.packageName === operatorPackageName | ||
| ); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The unanchored cluster Operator name predicate repeats at six sites. Every site matches with op.metadata.name?.includes(operatorPackageName) || op.spec?.packageName === operatorPackageName. OLM names these resources <packageName>.<namespace>, so an anchored match is sufficient. includes additionally matches a different package that contains the parameter as a substring, for example datagrid matching datagrid-enterprise. Two of the sites drive cluster-scoped deletion. Extract one shared helper and use it everywhere.
const isOperatorForPackage = (op: any, packageName: string): boolean =>
op.metadata?.name === packageName ||
op.metadata?.name?.startsWith(`${packageName}.`) ||
op.spec?.packageName === packageName;frontend/e2e/test-utils/operator-cleanup.ts#L141-L144: replace the predicate incleanupAllOperatorsByPackageNamebefore the cluster-scoped delete at Line 149.frontend/e2e/test-utils/operator-cleanup.ts#L198-L201: replace the predicate inforceCleanupAllOperatorsByPackageNamebefore the finalizer strip and force delete.frontend/e2e/test-utils/operator-cleanup.ts#L453-L456: replace the predicate that selects operators for deletion inoperatorTestCleanup.frontend/e2e/test-utils/operator-cleanup.ts#L494-L497: replace the predicate in the polling loop so the wait condition matches the deletion set.frontend/e2e/test-utils/operator-cleanup.ts#L516-L519: replace the predicate in the final operator check.frontend/e2e/test-utils/operator-cleanup.ts#L597-L600: replace the predicate in the post-CRD verification check.
📍 Affects 1 file
frontend/e2e/test-utils/operator-cleanup.ts#L141-L144(this comment)frontend/e2e/test-utils/operator-cleanup.ts#L198-L201frontend/e2e/test-utils/operator-cleanup.ts#L453-L456frontend/e2e/test-utils/operator-cleanup.ts#L494-L497frontend/e2e/test-utils/operator-cleanup.ts#L516-L519frontend/e2e/test-utils/operator-cleanup.ts#L597-L600
🤖 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/test-utils/operator-cleanup.ts` around lines 141 - 144, The
operator package matching predicate is duplicated and uses an overly broad
includes check. In frontend/e2e/test-utils/operator-cleanup.ts#L141-L144, define
the shared isOperatorForPackage helper with exact-name, package-prefixed-name,
and spec.packageName matching, then replace the predicates in
cleanupAllOperatorsByPackageName at `#L141-L144`,
forceCleanupAllOperatorsByPackageName at `#L198-L201`, operatorTestCleanup at
`#L453-L456`, its polling loop at `#L494-L497`, final operator check at `#L516-L519`,
and post-CRD verification at `#L597-L600`; all six sites require the shared helper
so deletion and verification use identical matching.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Substring matching selects unrelated operators for cluster-scoped deletion.
Line 142 matches any cluster Operator whose name contains operatorPackageName. OLM names these resources <packageName>.<namespace>, so a prefix or exact-segment match is what you need. includes also matches a different package that contains the parameter as a substring. For example, datagrid matches datagrid-enterprise. Line 149 then deletes that unrelated cluster Operator.
Anchor the match to the package-name boundary.
🐛 Proposed fix
const operators = await k8sClient.listClusterCustomResources('operators.coreos.com', 'v1', 'operators');
+ const isPackageMatch = (name?: string) =>
+ name === operatorPackageName || name?.startsWith(`${operatorPackageName}.`);
const matchingOperators = operators.filter((op: any) =>
- op.metadata.name?.includes(operatorPackageName) ||
+ isPackageMatch(op.metadata.name) ||
op.spec?.packageName === operatorPackageName
);📝 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
| const matchingOperators = operators.filter((op: any) => | |
| op.metadata.name?.includes(operatorPackageName) || | |
| op.spec?.packageName === operatorPackageName | |
| ); | |
| const isPackageMatch = (name?: string) => | |
| name === operatorPackageName || | |
| name?.startsWith(`${operatorPackageName}.`); | |
| const matchingOperators = operators.filter((op: any) => | |
| isPackageMatch(op.metadata.name) || | |
| op.spec?.packageName === operatorPackageName | |
| ); |
🤖 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/test-utils/operator-cleanup.ts` around lines 141 - 144, Update
the matchingOperators filter to match operator names only when
operatorPackageName is the complete package-name segment before the namespace
suffix, rather than using includes. Preserve the spec.packageName exact-match
fallback and ensure unrelated names such as datagrid-enterprise are not selected
for deletion.
Comment on lines
+423
to
+425
| if (!cleanupSuccess) { | ||
| console.log(`❌ Namespace cleanup failed for ${targetNamespace}`); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
operatorTestCleanup reports success after a failed sub-step.
Line 423 detects that cleanupOperatorResources failed and only logs. Line 613 then returns true. Line 605 also logs remaining operators and still reaches Line 613. A caller that checks the boolean cannot distinguish a clean teardown from a partial one.
Carry the sub-step result into the return value.
🐛 Proposed fix
if (!cleanupSuccess) {
console.log(`❌ Namespace cleanup failed for ${targetNamespace}`);
}Then at the end of the try block:
- return true;
+ return cleanupSuccess;🤖 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/test-utils/operator-cleanup.ts` around lines 423 - 425, Update
operatorTestCleanup so a failed cleanupOperatorResources result sets
cleanupSuccess to false rather than only logging the failure. Preserve the
existing remaining-operators logging, and return the accumulated cleanupSuccess
value at the end of the try block so callers can distinguish complete from
partial cleanup.
Contributor
|
@trgeiger: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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
1 participant
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:
Solution description:
Screenshots / screen recording:
Test setup:
Test cases:
Browser conformance:
Additional info:
Reviewers and assignees:
Summary by CodeRabbit
New Features
Bug Fixes