CONSOLE-5292: Migrate OLM enabled tests to playwright#16450
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@Cragsmann: This pull request references CONSOLE-5292 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Cragsmann The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThis PR transitions the OpenShift Console OLM test suite from Cypress integration tests to Playwright E2E tests. It introduces cluster-scoped Kubernetes custom resource operations (CRUD, patching, polling) to the test client, extends cleanup fixtures to handle cluster resources, and establishes a library of reusable Playwright page objects for generic UI patterns (details, lists, modals, navigation) and OLM-specific workflows (operator hub, installed operators, operator details). Mock generators produce consistent test resources (CRDs, CSVs, CatalogSources). Five comprehensive E2E specs cover catalog source details, OLM descriptors, operator installation with namespace creation, deprecated operator warnings, and PackageManifest tabs. Cypress test files and their fixtures are removed, ESLint is configured to exclude E2E tests, and Suggested reviewers
🚥 Pre-merge checks | ✅ 9 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
frontend/e2e/clients/kubernetes-client.ts (1)
588-588: ⚡ Quick winAdd generic type parameter to replace
anyin condition callbacks.The
conditionFnparameters useanyand lose type information. Since the underlyinggetCustomResourceandgetClusterCustomResourcemethods already returnPromise<unknown>, adding a generic type parameter enables callers to specify the expected resource type and improves type safety.Proposed refactor
- async waitForCustomResourceCondition( + async waitForCustomResourceCondition<T = unknown>( group: string, version: string, namespace: string, plural: string, name: string, - conditionFn: (resource: T) => boolean, + conditionFn: (resource: T) => boolean, timeoutMs: number, ): Promise<boolean> { @@ - const resource = await this.getCustomResource(group, version, namespace, plural, name); + const resource = (await this.getCustomResource( + group, + version, + namespace, + plural, + name, + )) as T; return conditionFn(resource); @@ - async waitForClusterCustomResourceCondition( + async waitForClusterCustomResourceCondition<T = unknown>( group: string, version: string, plural: string, name: string, - conditionFn: (resource: T) => boolean, + conditionFn: (resource: T) => boolean, timeoutMs: number, ): Promise<boolean> { @@ - const resource = await this.getClusterCustomResource(group, version, plural, name); + const resource = (await this.getClusterCustomResource(group, version, plural, name)) as T; return conditionFn(resource);Also applies to: 610-610
🤖 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` at line 588, Update the callback and enclosing method signatures to add a generic type parameter (e.g., <T>) so conditionFn is typed as (resource: T) => boolean instead of any, and propagate T to the methods that call getCustomResource/getClusterCustomResource so callers can specify the expected resource type; specifically modify the conditionFn parameter and the surrounding function(s) to be generic and ensure returns from getCustomResource/getClusterCustomResource are treated as Promise<T> (or cast to T) when passed into conditionFn. This change should be applied wherever conditionFn is declared (the occurrences referencing conditionFn and the calls that use getCustomResource/getClusterCustomResource).frontend/e2e/pages/olm/operator-details-page.ts (2)
33-38: ⚡ Quick winClarify or remove
.last()in tab selection.Using
.last()on a tab locator suggests there might be multiple elements matchinghorizontal-link-${tabName}. If tabs have unique test IDs,.last()is unnecessary and masks potential issues. If there are genuinely multiple tab sets, consider scoping the locator to a specific region or documenting why.last()is needed.♻️ Proposed clarification
If tabs are unique, remove
.last():async selectTab(tabName: string): Promise<void> { - const tab = this.page.locator(`[data-test-id="horizontal-link-${tabName}"]`).last(); + const tab = this.page.locator(`[data-test-id="horizontal-link-${tabName}"]`); await expect(tab).toBeAttached({ timeout: 60_000 });If multiple tab sets exist, add a comment explaining which set
.last()targets, or scope the locator to a specific container.🤖 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/olm/operator-details-page.ts` around lines 33 - 38, In selectTab, the locator uses .last() which hides whether multiple elements match `[data-test-id="horizontal-link-${tabName}"]`; either remove .last() if test IDs are unique so the first matching element is used, or instead scope the locator to the correct container (e.g., a specific panel or region) and keep a short comment explaining why .last() is required when multiple tab sets exist; update the locator expression in selectTab to reflect the chosen approach and add the explanatory comment if retaining .last().
53-88: ⚡ Quick winExtract repeated tab name logic.
The ternary
operand.name === 'All instances' ? 'All instances' : operand.nameis repeated on lines 54, 72, 81, and 86. Extract it to a helper method to reduce duplication.♻️ Proposed refactor
+ private getTabName(operand: TestOperandProps): string { + return operand.name === 'All instances' ? 'All instances' : operand.name; + } + async createOperand(operand: TestOperandProps): Promise<void> { - await this.selectTab(operand.name === 'All instances' ? 'All instances' : operand.name); + await this.selectTab(this.getTabName(operand)); // ... } async deleteOperand(operand: TestOperandProps): Promise<void> { - await this.selectTab(operand.name === 'All instances' ? 'All instances' : operand.name); + await this.selectTab(this.getTabName(operand)); // ... } async operandShouldExist(operand: TestOperandProps): Promise<void> { - await this.selectTab(operand.name === 'All instances' ? 'All instances' : operand.name); + await this.selectTab(this.getTabName(operand)); // ... } async operandShouldNotExist(operand: TestOperandProps): Promise<void> { - await this.selectTab(operand.name === 'All instances' ? 'All instances' : operand.name); + await this.selectTab(this.getTabName(operand)); // ... }🤖 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/olm/operator-details-page.ts` around lines 53 - 88, The repeated ternary operand.name === 'All instances' ? 'All instances' : operand.name should be extracted into a helper to avoid duplication; add a private helper (e.g., getTabName(operand: TestOperandProps): string) in the class that returns that value and replace the inline ternary in createOperand, deleteOperand, operandShouldExist, and operandShouldNotExist with a call to selectTab(this.getTabName(operand)) (or call getTabName and pass the result to selectTab) so all four methods use the helper instead of repeating the expression.frontend/e2e/pages/olm/installed-operators-page.ts (1)
29-31: ⚡ Quick winScope
statusText()to avoid ambiguity.The
statusText()locator uses a generic test ID that could match multiple status elements on the page. If there are multiple operator rows visible, this will select the first match rather than the intended operator's status.Consider accepting a
nameparameter and scoping the locator withinoperatorRow(name):♻️ Proposed refactor
- statusText(): Locator { - return this.page.getByTestId('status-text'); + statusText(operatorName: string): Locator { + return this.operatorRow(operatorName).getByTestId('status-text'); }Then update the caller on line 35:
async waitForOperatorSucceeded(name: string, timeoutMs = 720_000): Promise<void> { await expect(this.operatorRow(name)).toBeAttached({ timeout: 300_000 }); - await expect(this.statusText()).toContainText('Succeeded', { timeout: timeoutMs }); + await expect(this.statusText(name)).toContainText('Succeeded', { timeout: timeoutMs }); }🤖 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/olm/installed-operators-page.ts` around lines 29 - 31, The statusText() locator is too generic and should be scoped to a specific operator: change statusText() to accept a name parameter (e.g., statusText(name: string): Locator) and return this.operatorRow(name).getByTestId('status-text') so it targets the status within the specific operatorRow(name) locator; then update all callers that currently call statusText() (the caller referenced near the existing call site) to pass the operator name (statusText(name)) so the locator is unambiguous.frontend/e2e/tests/olm/packageserver-tabs.spec.ts (1)
80-82: 💤 Low valueSimplify negative assertions with a single positive URL check.
Instead of three negative assertions to verify we're back at the Details tab, consider a single positive assertion:
- await expect(page).not.toHaveURL(/\/yaml/); - await expect(page).not.toHaveURL(/\/resources/); - await expect(page).not.toHaveURL(/\/events/); + await expect(page).toHaveURL(baseUrl);This makes the intent clearer and is more maintainable.
🤖 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 80 - 82, Replace the three negative URL assertions (the three expect(page).not.toHaveURL calls) with a single positive assertion that verifies the page is on the Details tab, e.g. use expect(page).toHaveURL(/\/details/) so the test explicitly checks the intended URL; update the assertion in packageserver-tabs.spec.ts where the current not.toHaveURL(/\/yaml/), not.toHaveURL(/\/resources/), and not.toHaveURL(/\/events/) calls appear and remove those three negatives in favor of the single positive check.frontend/e2e/tests/olm/mocks/index.ts (1)
217-217: 💤 Low valueType assertions bypass TypeScript safety in test fixtures.
The
as unknown as Record<string, string>double-cast defeats the type safety ofas constobjects. While this works for test code, consider defining explicit type mappings or helper functions that preserve type information. This would catch errors if capability URNs are refactored.Also applies to: 220-220, 273-278
🤖 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/mocks/index.ts` at line 217, The test fixture bypasses TypeScript safety by double-casting SpecCapability to Record<string,string> when calling buildSpecEntries; update the fixtures to preserve types by creating an explicit typed mapping or helper that converts SpecCapability into the shape buildSpecEntries expects (e.g., define a SpecCapabilityMap or a helper function convertSpecCapabilityToRecord) and use that instead of "as unknown as Record<string,string>" in the calls to buildSpecEntries (also replace the similar casts at the other occurrences around lines 220 and 273-278).frontend/e2e/tests/olm/create-namespace.spec.ts (1)
26-26: 💤 Low valueType assertion
as anyloses type safety.The code uses
(csv as any)?.metadata?.nameand(ip as any)?.metadata?.name, bypassing TypeScript's type checking. While acceptable in test code, consider defining minimal interfaces for the expected shape:interface K8sResource { metadata?: { name?: string }; } interface InstallPlan extends K8sResource { spec?: { clusterServiceVersionNames?: string[] }; }Then use
csv as K8sResourcefor clearer intent and partial type safety.Also applies to: 109-109
🤖 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` at line 26, The tests use `(csv as any)?.metadata?.name` and `(ip as any)?.metadata?.name`, which removes type safety; define minimal interfaces (e.g., K8sResource with metadata?: { name?: string } and InstallPlan extending it with spec?: { clusterServiceVersionNames?: string[] }) in the test file and cast `csv` and `ip` to those interfaces (e.g., `csv as K8sResource`, `ip as InstallPlan`) before reading `metadata.name` or `spec.clusterServiceVersionNames` so you keep partial typing while still accessing `name` (variable `name`) and the install plan fields safely.frontend/e2e/tests/olm/descriptors.spec.ts (1)
136-138: 💤 Low valueConsider extracting getNestedValue to a shared test utility.
This helper function resolves dot-separated paths to nested object values—a common pattern in form/descriptor tests. If other test files need similar logic, extract it to a shared utility module (e.g.,
frontend/e2e/utils/object-utils.ts) to avoid duplication.🤖 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/descriptors.spec.ts` around lines 136 - 138, The local helper getNestedValue is duplicated logic that should be extracted to a shared test utility: create a shared module (e.g., object-utils) that exports a function getNestedValue(pathed lookup) implemented the same way, replace the inline function in descriptors.spec.ts with an import from that module, and update any other test files that need nested lookup to import and reuse the exported getNestedValue to avoid duplication.
🤖 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/clients/kubernetes-client.ts`:
- Around line 545-547: The success branch unconditionally calls
resolve(JSON.parse(body)) which will throw on an empty 2xx PATCH response;
update the conditional in the handler around resolve(JSON.parse(body)) to check
for an empty or whitespace-only body (e.g., body == null || body.trim() === "")
and if so resolve(undefined or null) instead of parsing, otherwise parse and
resolve the JSON; change the code that currently calls resolve(JSON.parse(body))
so it safely handles empty successful responses.
In `@frontend/e2e/tests/olm/catalog-source-details.spec.ts`:
- Around line 11-37: The tests currently create shared resources in
test.beforeAll/test.afterAll (catalogSourceName, createTestCatalogSource) which
violates the self-contained guideline; refactor each test() to create its own
CatalogSource inside the test body (call createTestCatalogSource there),
register its name with the existing cleanup fixture instead of using
test.afterAll, and remove usage of test.beforeAll/test.afterAll for
CatalogSource management so each test independently creates, asserts, and cleans
up its CatalogSource resource.
In `@frontend/e2e/tests/olm/create-namespace.spec.ts`:
- Around line 45-47: The suite-level cleanup using test.beforeAll/test.afterAll
calling cleanupOperator on the 'openshift-operators' namespace breaks the
self-contained test guideline; either remove these beforeAll/afterAll hooks and
perform cleanup inside the individual test (use the existing cleanup fixture and
call cleanupOperator(k8sClient, 'openshift-operators') within the test body,
e.g., before creating resources, and ensure resources are registered with the
cleanup fixture), or if 'openshift-operators' is a shared global that cannot be
isolated, delete the hooks and add a clear comment near the test explaining that
this namespace is globally managed and requires special handling; update
references to cleanupOperator and the cleanup fixture accordingly so all
resource management is local to the test or explicitly documented as global.
In `@frontend/e2e/tests/olm/deprecated-operator-warnings.spec.ts`:
- Around line 26-134: The suite currently creates a shared CatalogSource in
test.beforeAll using k8sClient.createCustomResource and waits with
k8sClient.waitForCustomResourceCondition for catalogSourceName to be READY;
change this so each test creates its own CatalogSource (use test.beforeEach to
call k8sClient.createCustomResource with testDeprecatedCatalogSource or a
per-test variant and waitForCustomResourceCondition), and remove the shared
creation from test.beforeAll and related deletion from test.afterAll; ensure
each created CatalogSource is cleaned up via the existing cleanup fixture (or
call k8sClient.deleteCustomResource for catalogSourceName in test.afterEach) so
tests are self-contained and do not share state.
- Around line 233-307: The "Installed Operator deprecation warnings" test suite
violates self-contained test guidelines by using serial mode configuration and
performing shared operator installation in the beforeAll hook that is depended
on by all three tests. Refactor this by either combining the three tests into a
single test function using test.step() blocks for each test scenario (if shared
state is necessary), or make each of the three tests independently install and
verify its own operator instance, removing the beforeAll hook and test.configure
serial mode configuration. Choose the approach based on whether the tests
genuinely must share the operator installation state or can be made
independently verifiable.
In `@frontend/e2e/tests/olm/descriptors.spec.ts`:
- Around line 14-55: The test suite currently creates shared cluster-scoped
resources in test.beforeAll/test.afterAll (see beforeAll, afterAll,
createClusterCustomResource, createCustomResource, createNamespace) which breaks
test isolation; change each test to create its own CRD/CSV/namespace inside the
individual test body (or in test.beforeEach) and register those created
resources with the existing cleanup fixture rather than using beforeAll/afterAll
so each test is self-contained and resources are deleted per-test via cleanup;
ensure you remove the shared createClusterCustomResource/createCustomResource
calls from beforeAll/afterAll and instead call
k8sClient.createClusterCustomResource/createCustomResource and
k8sClient.createNamespace inside each test and call cleanup for their names.
In `@frontend/e2e/tests/olm/edit-default-sources.spec.ts`:
- Around line 6-35: This test mutates global OperatorHub state by toggling
defaultSourceToBeToggled ('redhat-operators') from the test "disables and
re-enables default catalog sources..." using DetailsPage and ModalPage; fix it
by either (preferred) creating and using a test-specific catalog source instead
of touching the global default, or (if you must toggle the global) ensure
isolation and robust cleanup: mark the test to run serially (e.g.,
test.describe.serial or an isolation tag) and wrap the disable/enable steps in a
try/finally that captures the original status via
page.getByTestId(`status_${defaultSourceToBeToggled}`) before changes and always
restores it using the same DetailsPage/ModalPage flow and modalPage.submit in
the finally block so the original state is re-applied even on
failures/interruption.
In `@frontend/e2e/tests/olm/packageserver-tabs.spec.ts`:
- Around line 6-9: The test assumes the PackageManifest named by
packageManifestName ('3scale-operator') and CSV csvName ('packageserver') exist,
which is brittle; modify the olm/packageserver-tabs.spec.ts tests to either (A)
assert the test environment guarantees this PackageManifest before using baseUrl
by checking for its existence via the k8s client and skipping/failing early if
missing, or (B) create a dedicated CatalogSource in the test setup (using the
same approach as deprecated-operator-warnings) and wait until the generated
PackageManifest and CSV appear before constructing baseUrl and running
assertions; update teardown to remove the CatalogSource and related resources to
keep tests hermetic.
---
Nitpick comments:
In `@frontend/e2e/clients/kubernetes-client.ts`:
- Line 588: Update the callback and enclosing method signatures to add a generic
type parameter (e.g., <T>) so conditionFn is typed as (resource: T) => boolean
instead of any, and propagate T to the methods that call
getCustomResource/getClusterCustomResource so callers can specify the expected
resource type; specifically modify the conditionFn parameter and the surrounding
function(s) to be generic and ensure returns from
getCustomResource/getClusterCustomResource are treated as Promise<T> (or cast to
T) when passed into conditionFn. This change should be applied wherever
conditionFn is declared (the occurrences referencing conditionFn and the calls
that use getCustomResource/getClusterCustomResource).
In `@frontend/e2e/pages/olm/installed-operators-page.ts`:
- Around line 29-31: The statusText() locator is too generic and should be
scoped to a specific operator: change statusText() to accept a name parameter
(e.g., statusText(name: string): Locator) and return
this.operatorRow(name).getByTestId('status-text') so it targets the status
within the specific operatorRow(name) locator; then update all callers that
currently call statusText() (the caller referenced near the existing call site)
to pass the operator name (statusText(name)) so the locator is unambiguous.
In `@frontend/e2e/pages/olm/operator-details-page.ts`:
- Around line 33-38: In selectTab, the locator uses .last() which hides whether
multiple elements match `[data-test-id="horizontal-link-${tabName}"]`; either
remove .last() if test IDs are unique so the first matching element is used, or
instead scope the locator to the correct container (e.g., a specific panel or
region) and keep a short comment explaining why .last() is required when
multiple tab sets exist; update the locator expression in selectTab to reflect
the chosen approach and add the explanatory comment if retaining .last().
- Around line 53-88: The repeated ternary operand.name === 'All instances' ?
'All instances' : operand.name should be extracted into a helper to avoid
duplication; add a private helper (e.g., getTabName(operand: TestOperandProps):
string) in the class that returns that value and replace the inline ternary in
createOperand, deleteOperand, operandShouldExist, and operandShouldNotExist with
a call to selectTab(this.getTabName(operand)) (or call getTabName and pass the
result to selectTab) so all four methods use the helper instead of repeating the
expression.
In `@frontend/e2e/tests/olm/create-namespace.spec.ts`:
- Line 26: The tests use `(csv as any)?.metadata?.name` and `(ip as
any)?.metadata?.name`, which removes type safety; define minimal interfaces
(e.g., K8sResource with metadata?: { name?: string } and InstallPlan extending
it with spec?: { clusterServiceVersionNames?: string[] }) in the test file and
cast `csv` and `ip` to those interfaces (e.g., `csv as K8sResource`, `ip as
InstallPlan`) before reading `metadata.name` or
`spec.clusterServiceVersionNames` so you keep partial typing while still
accessing `name` (variable `name`) and the install plan fields safely.
In `@frontend/e2e/tests/olm/descriptors.spec.ts`:
- Around line 136-138: The local helper getNestedValue is duplicated logic that
should be extracted to a shared test utility: create a shared module (e.g.,
object-utils) that exports a function getNestedValue(pathed lookup) implemented
the same way, replace the inline function in descriptors.spec.ts with an import
from that module, and update any other test files that need nested lookup to
import and reuse the exported getNestedValue to avoid duplication.
In `@frontend/e2e/tests/olm/mocks/index.ts`:
- Line 217: The test fixture bypasses TypeScript safety by double-casting
SpecCapability to Record<string,string> when calling buildSpecEntries; update
the fixtures to preserve types by creating an explicit typed mapping or helper
that converts SpecCapability into the shape buildSpecEntries expects (e.g.,
define a SpecCapabilityMap or a helper function convertSpecCapabilityToRecord)
and use that instead of "as unknown as Record<string,string>" in the calls to
buildSpecEntries (also replace the similar casts at the other occurrences around
lines 220 and 273-278).
In `@frontend/e2e/tests/olm/packageserver-tabs.spec.ts`:
- Around line 80-82: Replace the three negative URL assertions (the three
expect(page).not.toHaveURL calls) with a single positive assertion that verifies
the page is on the Details tab, e.g. use expect(page).toHaveURL(/\/details/) so
the test explicitly checks the intended URL; update the assertion in
packageserver-tabs.spec.ts where the current not.toHaveURL(/\/yaml/),
not.toHaveURL(/\/resources/), and not.toHaveURL(/\/events/) calls appear and
remove those three negatives in favor of the single positive check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a2420c61-ad44-4a1e-b865-ec771563d364
📒 Files selected for processing (26)
.gitignorefrontend/e2e/clients/kubernetes-client.tsfrontend/e2e/fixtures/cleanup-fixture.tsfrontend/e2e/pages/details-page.tsfrontend/e2e/pages/list-page.tsfrontend/e2e/pages/modal-page.tsfrontend/e2e/pages/nav-page.tsfrontend/e2e/pages/olm/installed-operators-page.tsfrontend/e2e/pages/olm/operator-details-page.tsfrontend/e2e/pages/olm/operator-hub-page.tsfrontend/e2e/pages/yaml-editor-page.tsfrontend/e2e/tests/olm/catalog-source-details.spec.tsfrontend/e2e/tests/olm/create-namespace.spec.tsfrontend/e2e/tests/olm/deprecated-operator-warnings.spec.tsfrontend/e2e/tests/olm/descriptors.spec.tsfrontend/e2e/tests/olm/edit-default-sources.spec.tsfrontend/e2e/tests/olm/mocks/index.tsfrontend/e2e/tests/olm/packageserver-tabs.spec.tsfrontend/packages/eslint-plugin-console/lib/config/testing-library-tests.jsfrontend/packages/operator-lifecycle-manager/integration-tests/mocks/index.tsxfrontend/packages/operator-lifecycle-manager/integration-tests/tests/catalog-source-details.cy.tsfrontend/packages/operator-lifecycle-manager/integration-tests/tests/create-namespace.cy.tsfrontend/packages/operator-lifecycle-manager/integration-tests/tests/deprecated-operator-warnings.cy.tsfrontend/packages/operator-lifecycle-manager/integration-tests/tests/descriptors.cy.tsfrontend/packages/operator-lifecycle-manager/integration-tests/tests/edit-default-sources.cy.tsfrontend/packages/operator-lifecycle-manager/integration-tests/tests/packageserver-tabs.cy.ts
💤 Files with no reviewable changes (7)
- frontend/packages/operator-lifecycle-manager/integration-tests/tests/catalog-source-details.cy.ts
- frontend/packages/operator-lifecycle-manager/integration-tests/mocks/index.tsx
- frontend/packages/operator-lifecycle-manager/integration-tests/tests/deprecated-operator-warnings.cy.ts
- frontend/packages/operator-lifecycle-manager/integration-tests/tests/edit-default-sources.cy.ts
- frontend/packages/operator-lifecycle-manager/integration-tests/tests/create-namespace.cy.ts
- frontend/packages/operator-lifecycle-manager/integration-tests/tests/packageserver-tabs.cy.ts
- frontend/packages/operator-lifecycle-manager/integration-tests/tests/descriptors.cy.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
frontend/**/*.{ts,tsx}: Never import from package index files (barrel imports) like@console/sharedin new code, as they create circular dependencies and slow builds. Import from specific file paths instead.
Never use backticks in i18n t() function calls as the i18n parser cannot extract keys from template literals. Use single or double quotes instead.
Never import from deprecated packages or use code marked with@deprecatedTSdoc tag in new code.
Files:
frontend/e2e/pages/nav-page.tsfrontend/e2e/tests/olm/edit-default-sources.spec.tsfrontend/e2e/pages/modal-page.tsfrontend/e2e/tests/olm/create-namespace.spec.tsfrontend/e2e/tests/olm/catalog-source-details.spec.tsfrontend/e2e/pages/olm/installed-operators-page.tsfrontend/e2e/pages/olm/operator-hub-page.tsfrontend/e2e/fixtures/cleanup-fixture.tsfrontend/e2e/tests/olm/packageserver-tabs.spec.tsfrontend/e2e/tests/olm/deprecated-operator-warnings.spec.tsfrontend/e2e/tests/olm/descriptors.spec.tsfrontend/e2e/pages/olm/operator-details-page.tsfrontend/e2e/pages/list-page.tsfrontend/e2e/pages/yaml-editor-page.tsfrontend/e2e/pages/details-page.tsfrontend/e2e/tests/olm/mocks/index.tsfrontend/e2e/clients/kubernetes-client.ts
frontend/**/*.{ts,tsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Never use absolute URLs or paths. The console runs behind a proxy under an arbitrary path.
Files:
frontend/e2e/pages/nav-page.tsfrontend/e2e/tests/olm/edit-default-sources.spec.tsfrontend/e2e/pages/modal-page.tsfrontend/e2e/tests/olm/create-namespace.spec.tsfrontend/e2e/tests/olm/catalog-source-details.spec.tsfrontend/e2e/pages/olm/installed-operators-page.tsfrontend/e2e/pages/olm/operator-hub-page.tsfrontend/e2e/fixtures/cleanup-fixture.tsfrontend/e2e/tests/olm/packageserver-tabs.spec.tsfrontend/e2e/tests/olm/deprecated-operator-warnings.spec.tsfrontend/e2e/tests/olm/descriptors.spec.tsfrontend/e2e/pages/olm/operator-details-page.tsfrontend/e2e/pages/list-page.tsfrontend/e2e/pages/yaml-editor-page.tsfrontend/e2e/pages/details-page.tsfrontend/e2e/tests/olm/mocks/index.tsfrontend/e2e/clients/kubernetes-client.ts
**/*
📄 CodeRabbit inference engine (STYLEGUIDE.md)
Use lowercase dash-separated names for all files (to avoid git issues with case-insensitive file systems)
Files:
frontend/e2e/pages/nav-page.tsfrontend/e2e/tests/olm/edit-default-sources.spec.tsfrontend/e2e/pages/modal-page.tsfrontend/e2e/tests/olm/create-namespace.spec.tsfrontend/e2e/tests/olm/catalog-source-details.spec.tsfrontend/e2e/pages/olm/installed-operators-page.tsfrontend/e2e/pages/olm/operator-hub-page.tsfrontend/e2e/fixtures/cleanup-fixture.tsfrontend/e2e/tests/olm/packageserver-tabs.spec.tsfrontend/e2e/tests/olm/deprecated-operator-warnings.spec.tsfrontend/packages/eslint-plugin-console/lib/config/testing-library-tests.jsfrontend/e2e/tests/olm/descriptors.spec.tsfrontend/e2e/pages/olm/operator-details-page.tsfrontend/e2e/pages/list-page.tsfrontend/e2e/pages/yaml-editor-page.tsfrontend/e2e/pages/details-page.tsfrontend/e2e/tests/olm/mocks/index.tsfrontend/e2e/clients/kubernetes-client.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (STYLEGUIDE.md)
**/*.{ts,tsx,js,jsx}: New code MUST be written in TypeScript, not JavaScript
Run the linter and follow all rules defined in .eslintrc
Never use absolute paths in code; the app should be able to run behind a proxy under an arbitrary path
Use PascalCase for component file names, kebab-case for utility files, and*.spec.ts(x)for test filesUse camelCase for variable names in TypeScript and JavaScript files
**/*.{ts,tsx,js,jsx}: Any usage of i18next'sTFunction(rather than react-i18next'sTFunction) must be performed inside a function or component.
Don't use backticks inside of aTFunction. Our code parser will not automatically pick up the keys that contain backticks. Use single or double quotes instead.
Specify possible static values in comments for dynamic i18next keys that can't be interpolated by i18next-parser, such ast(key),t('key' + id), ort(key${id}).
Files:
frontend/e2e/pages/nav-page.tsfrontend/e2e/tests/olm/edit-default-sources.spec.tsfrontend/e2e/pages/modal-page.tsfrontend/e2e/tests/olm/create-namespace.spec.tsfrontend/e2e/tests/olm/catalog-source-details.spec.tsfrontend/e2e/pages/olm/installed-operators-page.tsfrontend/e2e/pages/olm/operator-hub-page.tsfrontend/e2e/fixtures/cleanup-fixture.tsfrontend/e2e/tests/olm/packageserver-tabs.spec.tsfrontend/e2e/tests/olm/deprecated-operator-warnings.spec.tsfrontend/packages/eslint-plugin-console/lib/config/testing-library-tests.jsfrontend/e2e/tests/olm/descriptors.spec.tsfrontend/e2e/pages/olm/operator-details-page.tsfrontend/e2e/pages/list-page.tsfrontend/e2e/pages/yaml-editor-page.tsfrontend/e2e/pages/details-page.tsfrontend/e2e/tests/olm/mocks/index.tsfrontend/e2e/clients/kubernetes-client.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (STYLEGUIDE.md)
**/*.{ts,tsx}: Prefer functional programming patterns and immutable data structures in TypeScript/JavaScript
Use React functional components with hooks instead of class components in TypeScript
Use React hooks and Context API for state management (migrating away from legacy Redux/Immutable.js)
Use existing hooks fromconsole-sharedwhen possible (useK8sWatchResource,useUserSettings, etc.)
Use k8s resource hooks for data fetching andconsoleFetchJSONfor HTTP requests in TypeScript
Place plugin routes in plugin-specific route files
Check existing types inconsole-sharedbefore creating new types
Use SCSS modules co-located with components, PatternFly design system components, and avoid any SCSS/CSS if possible
Follow WCAG 2.1 AA standards for accessibility; use semantic HTML, ARIA labels where needed, ensure keyboard navigation, and test with screen readers
UseuseTranslation('namespace')hook withkeyformat for translation keys in TypeScript
Use ErrorBoundary components and graceful degradation patterns for error handling in TypeScript
UseuseCallbackto memoize callbacks and prevent unnecessary re-renders in React
UseuseMemofor expensive filtering and computations to prevent re-computation on every render
UseReact.lazy()to lazy load heavy components
Avoid using theanytype in TypeScript; suggest proper type definitions instead
Check that null/undefined are properly handled in TypeScript (e.g.,string | undefined)
Verify exported types for reusable components in TypeScript
Reuse types from existing components rather than duplicating type definitions in component props
UseusePluginInfohook for plugin data in TypeScript
Avoid deprecated components; check for JSDoc@deprecatedtags, import paths containing/deprecated, andDEPRECATED_file name prefixes
Use direct imports to specific files instead of barrel exports (index.ts) to avoid circular dependency cycles and improve build performance
Useimport typefor importing type...
Files:
frontend/e2e/pages/nav-page.tsfrontend/e2e/tests/olm/edit-default-sources.spec.tsfrontend/e2e/pages/modal-page.tsfrontend/e2e/tests/olm/create-namespace.spec.tsfrontend/e2e/tests/olm/catalog-source-details.spec.tsfrontend/e2e/pages/olm/installed-operators-page.tsfrontend/e2e/pages/olm/operator-hub-page.tsfrontend/e2e/fixtures/cleanup-fixture.tsfrontend/e2e/tests/olm/packageserver-tabs.spec.tsfrontend/e2e/tests/olm/deprecated-operator-warnings.spec.tsfrontend/e2e/tests/olm/descriptors.spec.tsfrontend/e2e/pages/olm/operator-details-page.tsfrontend/e2e/pages/list-page.tsfrontend/e2e/pages/yaml-editor-page.tsfrontend/e2e/pages/details-page.tsfrontend/e2e/tests/olm/mocks/index.tsfrontend/e2e/clients/kubernetes-client.ts
frontend/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (README.md)
Follow internationalization guidelines as documented in INTERNATIONALIZATION.md for all frontend code
Files:
frontend/e2e/pages/nav-page.tsfrontend/e2e/tests/olm/edit-default-sources.spec.tsfrontend/e2e/pages/modal-page.tsfrontend/e2e/tests/olm/create-namespace.spec.tsfrontend/e2e/tests/olm/catalog-source-details.spec.tsfrontend/e2e/pages/olm/installed-operators-page.tsfrontend/e2e/pages/olm/operator-hub-page.tsfrontend/e2e/fixtures/cleanup-fixture.tsfrontend/e2e/tests/olm/packageserver-tabs.spec.tsfrontend/e2e/tests/olm/deprecated-operator-warnings.spec.tsfrontend/packages/eslint-plugin-console/lib/config/testing-library-tests.jsfrontend/e2e/tests/olm/descriptors.spec.tsfrontend/e2e/pages/olm/operator-details-page.tsfrontend/e2e/pages/list-page.tsfrontend/e2e/pages/yaml-editor-page.tsfrontend/e2e/pages/details-page.tsfrontend/e2e/tests/olm/mocks/index.tsfrontend/e2e/clients/kubernetes-client.ts
**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (INTERNATIONALIZATION.md)
**/*.{ts,tsx,jsx}: Thearia-label,aria-placeholder,aria-roledescription, andaria-valuetextattributes should be internationalized.
The optionali18nKeyproperty on the react-i18next Trans component should only be used as a last resort when the parser incorrectly generates keys containing HTML tags.
Files:
frontend/e2e/pages/nav-page.tsfrontend/e2e/tests/olm/edit-default-sources.spec.tsfrontend/e2e/pages/modal-page.tsfrontend/e2e/tests/olm/create-namespace.spec.tsfrontend/e2e/tests/olm/catalog-source-details.spec.tsfrontend/e2e/pages/olm/installed-operators-page.tsfrontend/e2e/pages/olm/operator-hub-page.tsfrontend/e2e/fixtures/cleanup-fixture.tsfrontend/e2e/tests/olm/packageserver-tabs.spec.tsfrontend/e2e/tests/olm/deprecated-operator-warnings.spec.tsfrontend/e2e/tests/olm/descriptors.spec.tsfrontend/e2e/pages/olm/operator-details-page.tsfrontend/e2e/pages/list-page.tsfrontend/e2e/pages/yaml-editor-page.tsfrontend/e2e/pages/details-page.tsfrontend/e2e/tests/olm/mocks/index.tsfrontend/e2e/clients/kubernetes-client.ts
**/e2e/pages/**/*.ts
📄 CodeRabbit inference engine (TESTING.md)
Extend
BasePagefrome2e/pages/base-page.tsfor Playwright page objects withrobustClick(),waitForLoadingComplete(), andgoTo()methods. Useprivate readonlyproperties for locators andasyncmethods for actions
Files:
frontend/e2e/pages/nav-page.tsfrontend/e2e/pages/modal-page.tsfrontend/e2e/pages/olm/installed-operators-page.tsfrontend/e2e/pages/olm/operator-hub-page.tsfrontend/e2e/pages/olm/operator-details-page.tsfrontend/e2e/pages/list-page.tsfrontend/e2e/pages/yaml-editor-page.tsfrontend/e2e/pages/details-page.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (STYLEGUIDE.md)
TypeScript tests should follow a similar 'test tables' convention as used in Go where applicable
Files:
frontend/e2e/tests/olm/edit-default-sources.spec.tsfrontend/e2e/tests/olm/create-namespace.spec.tsfrontend/e2e/tests/olm/catalog-source-details.spec.tsfrontend/e2e/tests/olm/packageserver-tabs.spec.tsfrontend/e2e/tests/olm/deprecated-operator-warnings.spec.tsfrontend/e2e/tests/olm/descriptors.spec.ts
**/e2e/tests/**/*.spec.ts
📄 CodeRabbit inference engine (TESTING.md)
**/e2e/tests/**/*.spec.ts: E2E tests with Playwright must be self-contained: eachtest()block creates its own resources, asserts independently, and cleans up via thecleanupfixture
In Playwright E2E tests, always usepage.getByTestId('x')to query[data-test="x"]attributes. If elements only have legacy test attributes, adddata-testand never remove legacy attributes
UseKubernetesClientfrome2e/clients/kubernetes-client.tsfor cluster interactions in Playwright E2E tests, never use shell commands in tests
Importtestandexpectfrome2e/fixturesin Playwright tests, not from@playwright/test. Custom fixtures providecleanup,testConfig, andk8sClient
Files:
frontend/e2e/tests/olm/edit-default-sources.spec.tsfrontend/e2e/tests/olm/create-namespace.spec.tsfrontend/e2e/tests/olm/catalog-source-details.spec.tsfrontend/e2e/tests/olm/packageserver-tabs.spec.tsfrontend/e2e/tests/olm/deprecated-operator-warnings.spec.tsfrontend/e2e/tests/olm/descriptors.spec.ts
🔇 Additional comments (16)
.gitignore (1)
43-43: LGTM!frontend/packages/eslint-plugin-console/lib/config/testing-library-tests.js (1)
21-21: ⚡ Quick winThe
excludedFilespattern at line 21 is correctly scoped. ESLint executes from thefrontend/directory (whereroot: trueis set in.eslintrc.js), soe2e/**properly matches and excludes E2E test files atfrontend/e2e/tests/**/*.spec.ts. No change is needed.> Likely an incorrect or invalid review comment.frontend/e2e/clients/kubernetes-client.ts (1)
470-521: LGTM!Also applies to: 559-580
frontend/e2e/fixtures/cleanup-fixture.ts (1)
26-32: LGTM!Also applies to: 105-119, 168-174
frontend/e2e/pages/details-page.ts (1)
1-68: LGTM!frontend/e2e/pages/list-page.ts (1)
1-34: LGTM!frontend/e2e/pages/modal-page.ts (1)
1-44: LGTM!frontend/e2e/pages/nav-page.ts (1)
1-31: LGTM!frontend/e2e/pages/olm/operator-hub-page.ts (1)
1-139: LGTM!frontend/e2e/pages/yaml-editor-page.ts (1)
1-33: LGTM!frontend/e2e/tests/olm/packageserver-tabs.spec.ts (2)
1-4: LGTM!
12-46: LGTM!frontend/e2e/tests/olm/descriptors.spec.ts (1)
121-212: LGTM!frontend/e2e/tests/olm/create-namespace.spec.ts (1)
56-98: LGTM!frontend/e2e/tests/olm/deprecated-operator-warnings.spec.ts (1)
228-231: LGTM!frontend/e2e/tests/olm/mocks/index.ts (1)
297-297: Empty image in test CatalogSource fixture is intentional—no action needed.The empty
image: ''field is a valid pattern for this test fixture. The CatalogSource is used purely for UI testing (details page rendering and registry poll interval modification) and doesn't require a functional image. When a real image is needed elsewhere (seetestDeprecatedCatalogSource), it's explicitly provided.> Likely an incorrect or invalid review comment.
| if (res.statusCode && res.statusCode >= 200 && res.statusCode < 300) { | ||
| resolve(JSON.parse(body)); | ||
| } else { |
There was a problem hiding this comment.
Handle empty successful PATCH responses before JSON parsing.
Line 546 unconditionally parses body; a valid 2xx response with an empty body will throw and incorrectly fail the patch flow.
Proposed fix
res.on('end', () => {
if (res.statusCode && res.statusCode >= 200 && res.statusCode < 300) {
- resolve(JSON.parse(body));
+ if (!body.trim()) {
+ resolve({});
+ return;
+ }
+ resolve(JSON.parse(body));
} else {
const msg = `Merge patch failed: HTTP ${res.statusCode} ${body.substring(0, 500)}`;
reject(new Error(msg));
}
});📝 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.
| if (res.statusCode && res.statusCode >= 200 && res.statusCode < 300) { | |
| resolve(JSON.parse(body)); | |
| } else { | |
| if (res.statusCode && res.statusCode >= 200 && res.statusCode < 300) { | |
| if (!body.trim()) { | |
| resolve({}); | |
| return; | |
| } | |
| resolve(JSON.parse(body)); | |
| } else { |
🤖 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 545 - 547, The
success branch unconditionally calls resolve(JSON.parse(body)) which will throw
on an empty 2xx PATCH response; update the conditional in the handler around
resolve(JSON.parse(body)) to check for an empty or whitespace-only body (e.g.,
body == null || body.trim() === "") and if so resolve(undefined or null) instead
of parsing, otherwise parse and resolve the JSON; change the code that currently
calls resolve(JSON.parse(body)) so it safely handles empty successful responses.
| test.describe('CatalogSource details page', { tag: ['@admin'] }, () => { | ||
| const testNs = `olm-catsrc-${Date.now()}`; | ||
| let catalogSourceName: string; | ||
|
|
||
| test.beforeAll(async ({ k8sClient }) => { | ||
| await k8sClient.createNamespace(testNs); | ||
| const catalogSource = createTestCatalogSource(testNs); | ||
| catalogSourceName = catalogSource.metadata.name; | ||
| await k8sClient.createCustomResource( | ||
| 'operators.coreos.com', | ||
| 'v1alpha1', | ||
| testNs, | ||
| 'catalogsources', | ||
| catalogSource, | ||
| ); | ||
| }); | ||
|
|
||
| test.afterAll(async ({ k8sClient }) => { | ||
| await k8sClient.deleteCustomResource( | ||
| 'operators.coreos.com', | ||
| 'v1alpha1', | ||
| testNs, | ||
| 'catalogsources', | ||
| catalogSourceName, | ||
| ); | ||
| await k8sClient.deleteNamespace(testNs); | ||
| }); |
There was a problem hiding this comment.
Tests share resources via beforeAll/afterAll instead of being self-contained.
The test suite creates a shared catalogSource in beforeAll that's used by all three tests. This violates the coding guideline: "E2E tests with Playwright must be self-contained: each test() block creates its own resources, asserts independently, and cleans up via the cleanup fixture."
Self-contained tests prevent:
- Cascading failures when one test corrupts shared state
- Inability to run tests in isolation or parallel
- Unclear test dependencies
Refactor each test to create its own catalog source and use the cleanup fixture for tracking. As per coding guidelines, E2E tests must create resources per-test and use the cleanup fixture.
♻️ Refactoring approach
Example for the first test:
-test.describe('CatalogSource details page', { tag: ['@admin'] }, () => {
- const testNs = `olm-catsrc-${Date.now()}`;
- let catalogSourceName: string;
-
- test.beforeAll(async ({ k8sClient }) => {
- await k8sClient.createNamespace(testNs);
- const catalogSource = createTestCatalogSource(testNs);
- catalogSourceName = catalogSource.metadata.name;
- await k8sClient.createCustomResource(
- 'operators.coreos.com',
- 'v1alpha1',
- testNs,
- 'catalogsources',
- catalogSource,
- );
- });
-
- test.afterAll(async ({ k8sClient }) => {
- await k8sClient.deleteCustomResource(
- 'operators.coreos.com',
- 'v1alpha1',
- testNs,
- 'catalogsources',
- catalogSourceName,
- );
- await k8sClient.deleteNamespace(testNs);
- });
+test.describe('CatalogSource details page', { tag: ['@admin'] }, () => {
async function navigateToCatalogSourcesList(page: import('@playwright/test').Page) {
// ... unchanged ...
}
- test(`renders details about the ${managedCatalogSource.name} catalog source`, async ({
- page,
- }) => {
+ test(`renders details about the ${managedCatalogSource.name} catalog source`, async ({
+ page,
+ k8sClient,
+ cleanup,
+ }) => {
+ const testNs = `olm-catsrc-${Date.now()}`;
+ await k8sClient.createNamespace(testNs);
+ cleanup.trackNamespace(testNs);
+
+ const catalogSource = createTestCatalogSource(testNs);
+ await k8sClient.createCustomResource(
+ 'operators.coreos.com',
+ 'v1alpha1',
+ testNs,
+ 'catalogsources',
+ catalogSource,
+ );
+ cleanup.trackCustomResource('operators.coreos.com', 'v1alpha1', testNs, 'catalogsources', catalogSource.metadata.name);
+
const detailsPage = new DetailsPage(page);
// ... rest of test ...
});Apply the same pattern to all three tests.
🤖 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 11 - 37,
The tests currently create shared resources in test.beforeAll/test.afterAll
(catalogSourceName, createTestCatalogSource) which violates the self-contained
guideline; refactor each test() to create its own CatalogSource inside the test
body (call createTestCatalogSource there), register its name with the existing
cleanup fixture instead of using test.afterAll, and remove usage of
test.beforeAll/test.afterAll for CatalogSource management so each test
independently creates, asserts, and cleans up its CatalogSource resource.
| test.beforeAll(async ({ k8sClient }) => { | ||
| await cleanupOperator(k8sClient, 'openshift-operators'); | ||
| }); |
There was a problem hiding this comment.
beforeAll/afterAll cleanup pattern is not self-contained.
Lines 45-47 and 49-54 use beforeAll/afterAll to clean up the openshift-operators namespace. While the main test at line 56 correctly creates resources and uses the cleanup fixture (lines 57-58, 88), the suite-level hooks violate the self-contained guideline.
Refactor to perform all cleanup within the test itself or accept that openshift-operators is a shared global namespace that tests can't fully isolate. If the latter, add a comment explaining why this namespace requires special handling. As per coding guidelines, E2E tests should avoid suite-level resource management.
Also applies to: 49-54
🤖 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 45 - 47, The
suite-level cleanup using test.beforeAll/test.afterAll calling cleanupOperator
on the 'openshift-operators' namespace breaks the self-contained test guideline;
either remove these beforeAll/afterAll hooks and perform cleanup inside the
individual test (use the existing cleanup fixture and call
cleanupOperator(k8sClient, 'openshift-operators') within the test body, e.g.,
before creating resources, and ensure resources are registered with the cleanup
fixture), or if 'openshift-operators' is a shared global that cannot be
isolated, delete the hooks and add a clear comment near the test explaining that
this namespace is globally managed and requires special handling; update
references to cleanupOperator and the cleanup fixture accordingly so all
resource management is local to the test or explicitly documented as global.
| test.beforeAll(async ({ k8sClient }) => { | ||
| await k8sClient.createNamespace(testNs); | ||
| // Clean up any existing resources from previous failed runs | ||
| await k8sClient | ||
| .deleteCustomResource( | ||
| 'operators.coreos.com', | ||
| 'v1alpha1', | ||
| subscriptionNamespace, | ||
| 'subscriptions', | ||
| subscriptionName, | ||
| ) | ||
| .catch(() => {}); | ||
| await k8sClient | ||
| .deleteCustomResource( | ||
| 'operators.coreos.com', | ||
| 'v1alpha1', | ||
| subscriptionNamespace, | ||
| 'clusterserviceversions', | ||
| csvName, | ||
| ) | ||
| .catch(() => {}); | ||
| await k8sClient | ||
| .deleteCustomResource( | ||
| 'operators.coreos.com', | ||
| 'v1alpha1', | ||
| catalogSourceNamespace, | ||
| 'catalogsources', | ||
| catalogSourceName, | ||
| ) | ||
| .catch(() => {}); | ||
|
|
||
| await k8sClient.createCustomResource( | ||
| 'operators.coreos.com', | ||
| 'v1alpha1', | ||
| catalogSourceNamespace, | ||
| 'catalogsources', | ||
| testDeprecatedCatalogSource, | ||
| ); | ||
|
|
||
| // Wait for CatalogSource to become READY | ||
| const ready = await k8sClient.waitForCustomResourceCondition( | ||
| 'operators.coreos.com', | ||
| 'v1alpha1', | ||
| catalogSourceNamespace, | ||
| 'catalogsources', | ||
| catalogSourceName, | ||
| (cs: any) => cs?.status?.connectionState?.lastObservedState === 'READY', | ||
| TIMEOUT, | ||
| ); | ||
| if (!ready) { | ||
| throw new Error(`CatalogSource ${catalogSourceName} did not become READY within timeout`); | ||
| } | ||
| }); | ||
|
|
||
| test.afterAll(async ({ k8sClient }) => { | ||
| await k8sClient | ||
| .deleteCustomResource( | ||
| 'operators.coreos.com', | ||
| 'v1alpha1', | ||
| subscriptionNamespace, | ||
| 'subscriptions', | ||
| subscriptionName, | ||
| ) | ||
| .catch(() => {}); | ||
| await k8sClient | ||
| .deleteCustomResource( | ||
| 'operators.coreos.com', | ||
| 'v1alpha1', | ||
| subscriptionNamespace, | ||
| 'clusterserviceversions', | ||
| csvName, | ||
| ) | ||
| .catch(() => {}); | ||
|
|
||
| // Delete InstallPlans related to the operator | ||
| const installPlans = await k8sClient.listCustomResources( | ||
| 'operators.coreos.com', | ||
| 'v1alpha1', | ||
| subscriptionNamespace, | ||
| 'installplans', | ||
| ); | ||
| for (const ip of installPlans) { | ||
| const ipName = (ip as any)?.metadata?.name; | ||
| const csvNames: string[] = (ip as any)?.spec?.clusterServiceVersionNames ?? []; | ||
| if (ipName && csvNames.includes(csvName)) { | ||
| await k8sClient | ||
| .deleteCustomResource( | ||
| 'operators.coreos.com', | ||
| 'v1alpha1', | ||
| subscriptionNamespace, | ||
| 'installplans', | ||
| ipName, | ||
| ) | ||
| .catch(() => {}); | ||
| } | ||
| } | ||
|
|
||
| await k8sClient | ||
| .deleteCustomResource( | ||
| 'operators.coreos.com', | ||
| 'v1alpha1', | ||
| catalogSourceNamespace, | ||
| 'catalogsources', | ||
| catalogSourceName, | ||
| ) | ||
| .catch(() => {}); | ||
|
|
||
| await k8sClient.deleteNamespace(testNs); | ||
| }); |
There was a problem hiding this comment.
Tests share a catalog source created in beforeAll.
All tests in this suite depend on a single CatalogSource created in beforeAll (lines 57-77). This violates the self-contained test guideline and creates cascading failures if the catalog source state is corrupted by any test.
Each test should create its own catalog source, wait for READY, and clean up via the cleanup fixture. As per coding guidelines, E2E tests must create resources per-test.
🤖 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/deprecated-operator-warnings.spec.ts` around lines 26
- 134, The suite currently creates a shared CatalogSource in test.beforeAll
using k8sClient.createCustomResource and waits with
k8sClient.waitForCustomResourceCondition for catalogSourceName to be READY;
change this so each test creates its own CatalogSource (use test.beforeEach to
call k8sClient.createCustomResource with testDeprecatedCatalogSource or a
per-test variant and waitForCustomResourceCondition), and remove the shared
creation from test.beforeAll and related deletion from test.afterAll; ensure
each created CatalogSource is cleaned up via the existing cleanup fixture (or
call k8sClient.deleteCustomResource for catalogSourceName in test.afterEach) so
tests are self-contained and do not share state.
| test.describe('Installed Operator deprecation warnings', () => { | ||
| test.describe.configure({ mode: 'serial' }); | ||
|
|
||
| test.beforeAll(async ({ k8sClient }) => { | ||
| // Install operator via API | ||
| await k8sClient.createCustomResource( | ||
| 'operators.coreos.com', | ||
| 'v1alpha1', | ||
| subscriptionNamespace, | ||
| 'subscriptions', | ||
| testDeprecatedSubscription, | ||
| ); | ||
|
|
||
| // Wait for InstallPlan to be created | ||
| const hasInstallPlan = await k8sClient.waitForCustomResourceCondition( | ||
| 'operators.coreos.com', | ||
| 'v1alpha1', | ||
| subscriptionNamespace, | ||
| 'subscriptions', | ||
| subscriptionName, | ||
| (sub: any) => !!sub?.status?.installPlanRef?.name, | ||
| 120_000, | ||
| ); | ||
| if (!hasInstallPlan) { | ||
| throw new Error('InstallPlan was not created within timeout'); | ||
| } | ||
|
|
||
| // Find and approve the InstallPlan | ||
| const installPlans = await k8sClient.listCustomResources( | ||
| 'operators.coreos.com', | ||
| 'v1alpha1', | ||
| subscriptionNamespace, | ||
| 'installplans', | ||
| ); | ||
| for (const ip of installPlans) { | ||
| const csvNames: string[] = (ip as any)?.spec?.clusterServiceVersionNames ?? []; | ||
| if (csvNames.includes(csvName)) { | ||
| const ipName = (ip as any)?.metadata?.name; | ||
| await k8sClient.patchCustomResource( | ||
| 'operators.coreos.com', | ||
| 'v1alpha1', | ||
| subscriptionNamespace, | ||
| 'installplans', | ||
| ipName, | ||
| { spec: { approved: true } }, | ||
| ); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Wait for CSV to succeed | ||
| const csvReady = await k8sClient.waitForCustomResourceCondition( | ||
| 'operators.coreos.com', | ||
| 'v1alpha1', | ||
| subscriptionNamespace, | ||
| 'clusterserviceversions', | ||
| csvName, | ||
| (csv: any) => csv?.status?.phase === 'Succeeded', | ||
| TIMEOUT, | ||
| ); | ||
| if (!csvReady) { | ||
| throw new Error(`CSV ${csvName} did not reach Succeeded phase within timeout`); | ||
| } | ||
|
|
||
| // Wait for PackageDeprecated condition on subscription | ||
| await k8sClient.waitForCustomResourceCondition( | ||
| 'operators.coreos.com', | ||
| 'v1alpha1', | ||
| subscriptionNamespace, | ||
| 'subscriptions', | ||
| subscriptionName, | ||
| hasPackageDeprecated, | ||
| 180_000, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Nested test suite uses serial mode and shared operator installation.
The Installed Operator deprecation warnings nested suite (line 233):
- Configures serial execution mode (line 234)
- Installs an operator in
beforeAll(lines 236-307) - Runs three tests that all depend on this shared operator state
This is a severe violation of the self-contained test guideline. Serial execution and shared state make tests:
- Unable to run in parallel
- Unable to run in isolation for debugging
- Fragile—if the
beforeAllfails, all three tests are skipped
Refactor to either:
- Combine the three tests into a single
test()with multipletest.step()blocks (if they must share state) - Make each test install and verify its own operator instance
As per coding guidelines, avoid suite-level shared state in E2E tests.
🤖 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/deprecated-operator-warnings.spec.ts` around lines 233
- 307, The "Installed Operator deprecation warnings" test suite violates
self-contained test guidelines by using serial mode configuration and performing
shared operator installation in the beforeAll hook that is depended on by all
three tests. Refactor this by either combining the three tests into a single
test function using test.step() blocks for each test scenario (if shared state
is necessary), or make each of the three tests independently install and verify
its own operator instance, removing the beforeAll hook and test.configure serial
mode configuration. Choose the approach based on whether the tests genuinely
must share the operator installation state or can be made independently
verifiable.
| test.beforeAll(async ({ k8sClient }) => { | ||
| await k8sClient.createNamespace(testNs); | ||
| await k8sClient.createClusterCustomResource( | ||
| 'apiextensions.k8s.io', | ||
| 'v1', | ||
| 'customresourcedefinitions', | ||
| crd, | ||
| ); | ||
| await k8sClient.createCustomResource( | ||
| 'operators.coreos.com', | ||
| 'v1alpha1', | ||
| testNs, | ||
| 'clusterserviceversions', | ||
| csv, | ||
| ); | ||
| }); | ||
|
|
||
| test.afterAll(async ({ k8sClient }) => { | ||
| await k8sClient | ||
| .deleteCustomResource( | ||
| crd.spec.group, | ||
| version, | ||
| testNs, | ||
| crd.spec.names.plural, | ||
| cr.metadata.name, | ||
| ) | ||
| .catch(() => {}); | ||
| await k8sClient.deleteClusterCustomResource( | ||
| 'apiextensions.k8s.io', | ||
| 'v1', | ||
| 'customresourcedefinitions', | ||
| crd.metadata.name, | ||
| ); | ||
| await k8sClient.deleteCustomResource( | ||
| 'operators.coreos.com', | ||
| 'v1alpha1', | ||
| testNs, | ||
| 'clusterserviceversions', | ||
| csv.metadata.name, | ||
| ); | ||
| await k8sClient.deleteNamespace(testNs); | ||
| }); |
There was a problem hiding this comment.
Tests share CRD and CSV resources via beforeAll/afterAll.
This test suite creates a shared CRD and CSV in beforeAll that both tests depend on, violating the self-contained test guideline. Each test should create its own CRD, CSV, and other resources, then clean up via the cleanup fixture.
The impact is amplified here because CRDs are cluster-scoped—if the CRD is left in an inconsistent state by one test, the second test will fail in non-obvious ways. As per coding guidelines, E2E tests must create resources per-test and use the cleanup fixture.
🤖 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/descriptors.spec.ts` around lines 14 - 55, The test
suite currently creates shared cluster-scoped resources in
test.beforeAll/test.afterAll (see beforeAll, afterAll,
createClusterCustomResource, createCustomResource, createNamespace) which breaks
test isolation; change each test to create its own CRD/CSV/namespace inside the
individual test body (or in test.beforeEach) and register those created
resources with the existing cleanup fixture rather than using beforeAll/afterAll
so each test is self-contained and resources are deleted per-test via cleanup;
ensure you remove the shared createClusterCustomResource/createCustomResource
calls from beforeAll/afterAll and instead call
k8sClient.createClusterCustomResource/createCustomResource and
k8sClient.createNamespace inside each test and call cleanup for their names.
| test('disables and re-enables default catalog sources from OperatorHub details page', async ({ | ||
| page, | ||
| }) => { | ||
| const detailsPage = new DetailsPage(page); | ||
| const modalPage = new ModalPage(page); | ||
| const defaultSourceToBeToggled = 'redhat-operators'; | ||
|
|
||
| await test.step('Navigate to OperatorHub configuration page', async () => { | ||
| await page.goto('/settings/cluster'); | ||
| await page.locator('[data-test-id="horizontal-link-Configuration"]').click(); | ||
| await page.locator('[data-test-id="OperatorHub"]').click(); | ||
| await detailsPage.sectionHeaderShouldExist('OperatorHub details'); | ||
| }); | ||
|
|
||
| await test.step('Disable the default source via modal', async () => { | ||
| await page.getByTestId('Default sources-details-item__edit-button').click(); | ||
| await modalPage.modalTitleShouldContain('Edit default sources'); | ||
| await page.getByTestId(`${defaultSourceToBeToggled}__checkbox`).click(); | ||
| await modalPage.submit(); | ||
| await expect(page.getByTestId(`status_${defaultSourceToBeToggled}`)).toHaveText('Disabled'); | ||
| }); | ||
|
|
||
| await test.step('Re-enable the default source via modal', async () => { | ||
| await page.getByTestId('Default sources-details-item__edit-button').click(); | ||
| await modalPage.modalTitleShouldContain('Edit default sources'); | ||
| await page.getByTestId(`${defaultSourceToBeToggled}__checkbox`).click(); | ||
| await modalPage.submit(); | ||
| await expect(page.getByTestId(`status_${defaultSourceToBeToggled}`)).toHaveText('Enabled'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test modifies global OperatorHub configuration without isolation.
This test toggles the redhat-operators default source, which is global cluster configuration shared across all namespaces. If other tests run concurrently and depend on redhat-operators being enabled, they will fail. Additionally, if this test is interrupted, it may leave redhat-operators disabled.
Consider:
- Using a test-specific catalog source instead of modifying defaults
- Documenting that this test must run in isolation (e.g., with a special tag)
- Ensuring the test restores the original state even if it fails (wrap in try/finally or use cleanup hooks)
🛡️ Proposed safeguard
Capture and restore the original state:
test('disables and re-enables default catalog sources from OperatorHub details page', async ({
page,
+ k8sClient,
}) => {
const detailsPage = new DetailsPage(page);
const modalPage = new ModalPage(page);
const defaultSourceToBeToggled = 'redhat-operators';
+
+ // Capture original state
+ const originalConfig = await k8sClient.getClusterCustomResource(
+ 'config.openshift.io',
+ 'v1',
+ 'operatorhubs',
+ 'cluster',
+ );
+ const wasEnabled = !originalConfig?.spec?.sources?.some(
+ (s: any) => s.name === defaultSourceToBeToggled && s.disabled,
+ );
await test.step('Navigate to OperatorHub configuration page', async () => {
// ... unchanged ...
});
await test.step('Disable the default source via modal', async () => {
// ... unchanged ...
});
await test.step('Re-enable the default source via modal', async () => {
// ... unchanged ...
});
+
+ // Restore original state if it was already disabled
+ if (!wasEnabled) {
+ await page.getByTestId('Default sources-details-item__edit-button').click();
+ await page.getByTestId(`${defaultSourceToBeToggled}__checkbox`).click();
+ await modalPage.submit();
+ }
});🤖 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 6 - 35,
This test mutates global OperatorHub state by toggling defaultSourceToBeToggled
('redhat-operators') from the test "disables and re-enables default catalog
sources..." using DetailsPage and ModalPage; fix it by either (preferred)
creating and using a test-specific catalog source instead of touching the global
default, or (if you must toggle the global) ensure isolation and robust cleanup:
mark the test to run serially (e.g., test.describe.serial or an isolation tag)
and wrap the disable/enable steps in a try/finally that captures the original
status via page.getByTestId(`status_${defaultSourceToBeToggled}`) before changes
and always restores it using the same DetailsPage/ModalPage flow and
modalPage.submit in the finally block so the original state is re-applied even
on failures/interruption.
| const csvNamespace = 'openshift-operator-lifecycle-manager'; | ||
| const csvName = 'packageserver'; | ||
| const packageManifestName = '3scale-operator'; | ||
| const baseUrl = `/k8s/ns/${csvNamespace}/operators.coreos.com~v1alpha1~ClusterServiceVersion/${csvName}/packages.operators.coreos.com~v1~PackageManifest/${packageManifestName}`; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for available PackageManifests in the openshift-operator-lifecycle-manager namespace
# List PackageManifests to verify 3scale-operator availability
kubectl get packagemanifests -n openshift-operator-lifecycle-manager -o json | jq -r '.items[].metadata.name' | sortRepository: openshift/console
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Read the full test file to understand what it's testing
head -100 frontend/e2e/tests/olm/packageserver-tabs.spec.tsRepository: openshift/console
Length of output: 3330
🏁 Script executed:
#!/bin/bash
# Find other OLM test files to compare patterns
fd -e spec.ts frontend/e2e/tests/olm/Repository: openshift/console
Length of output: 459
🏁 Script executed:
#!/bin/bash
# List all test files in the OLM test directory
ls -la frontend/e2e/tests/olm/Repository: openshift/console
Length of output: 700
🏁 Script executed:
#!/bin/bash
# Read catalog-source-details.spec.ts to compare resource creation patterns
cat frontend/e2e/tests/olm/catalog-source-details.spec.tsRepository: openshift/console
Length of output: 5189
🏁 Script executed:
#!/bin/bash
# Read descriptors.spec.ts to compare resource creation patterns
cat frontend/e2e/tests/olm/descriptors.spec.tsRepository: openshift/console
Length of output: 8181
🏁 Script executed:
#!/bin/bash
# Check what's in the OLM test mocks
cat frontend/e2e/tests/olm/mocks/index.ts | head -150Repository: openshift/console
Length of output: 5736
🏁 Script executed:
#!/bin/bash
# Continue reading the mocks file to see if PackageManifest is created
tail -200 frontend/e2e/tests/olm/mocks/index.tsRepository: openshift/console
Length of output: 6097
🏁 Script executed:
#!/bin/bash
# Check if there's CI configuration for test environment setup
find . -name "*.yaml" -o -name "*.yml" | grep -E "(ci|test|e2e)" | head -20Repository: openshift/console
Length of output: 1924
🏁 Script executed:
#!/bin/bash
# Check CI operator configuration for test environment setup
cat .ci-operator.yaml | head -100Repository: openshift/console
Length of output: 139
🏁 Script executed:
#!/bin/bash
# Check for E2E test configuration or documentation
find . -path ./node_modules -prune -o -name "playwright.config.*" -type f -print
find . -path ./node_modules -prune -o -name "README*" -o -name "*.md" | grep -i e2e | head -10Repository: openshift/console
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Check playwright configuration for test setup details
cat frontend/playwright.config.ts | head -200Repository: openshift/console
Length of output: 3851
🏁 Script executed:
#!/bin/bash
# Check deprecated-operator-warnings test to see how other tests handle PackageManifest setup
grep -A 30 "beforeAll\|CatalogSource" frontend/e2e/tests/olm/deprecated-operator-warnings.spec.ts | head -60Repository: openshift/console
Length of output: 2234
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation about which operators are guaranteed in test environment
grep -r "3scale-operator\|packageserver" frontend/e2e --include="*.ts" --include="*.md"Repository: openshift/console
Length of output: 437
🏁 Script executed:
#!/bin/bash
# Check create-namespace.spec.ts to see 3scale-operator usage context
cat frontend/e2e/tests/olm/create-namespace.spec.tsRepository: openshift/console
Length of output: 3633
Verify test environment guarantees the 3scale-operator PackageManifest exists, or use a different approach to ensure test reliability.
These tests assume the packageserver CSV and 3scale-operator PackageManifest are pre-installed in the cluster. While PackageManifests are derived resources (generated automatically by OLM from CatalogSources) and cannot be directly created, relying on 3scale-operator from default catalogs may be brittle—if the test cluster's catalog sources change, these tests will fail.
Unlike other OLM tests in this suite (catalog-source-details, descriptors, deprecated-operator-warnings), which either create their own test resources via k8sClient or explicitly install resources as part of test flow, this test assumes pre-existing cluster state with no setup or teardown.
Either confirm that CI test clusters guarantee 3scale-operator is always available from default catalogs, or consider creating a test CatalogSource that generates the PackageManifest (as demonstrated in the deprecated-operator-warnings tests).
🤖 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 6 - 9, The
test assumes the PackageManifest named by packageManifestName
('3scale-operator') and CSV csvName ('packageserver') exist, which is brittle;
modify the olm/packageserver-tabs.spec.ts tests to either (A) assert the test
environment guarantees this PackageManifest before using baseUrl by checking for
its existence via the k8s client and skipping/failing early if missing, or (B)
create a dedicated CatalogSource in the test setup (using the same approach as
deprecated-operator-warnings) and wait until the generated PackageManifest and
CSV appear before constructing baseUrl and running assertions; update teardown
to remove the CatalogSource and related resources to keep tests hermetic.
|
@Cragsmann: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
2 participants
Analysis / Root cause:
Solution description:
Screenshots / screen recording:
Test setup:
Test cases:
Browser conformance:
Additional info:
Reviewers and assignees:
Summary by CodeRabbit
Release Notes
Tests
Chores