[WIP] OCPBUGS-79355: Fix and enable OLM Package E2E tests disabled during the public/ directory useK8sWatchResource refactoring#16220
Conversation
openshift-ci-robot
added
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@cajieh: This pull request references Jira Issue OCPBUGS-79355, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 APPROVED This pull-request has been approved by: cajieh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
component/core
cajieh
changed the title
cajieh
force-pushed
the
fix-olm-packages-e2e-tests-skipped-in-firhose-removal
branch
from
8c9b484 to
280bce5
Compare
openshift-ci
bot
added
the
kind/i18n
cajieh
force-pushed
the
fix-olm-packages-e2e-tests-skipped-in-firhose-removal
branch
2 times, most recently
from
9215b54 to
807b4ba
Compare
cajieh
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/test e2e-gcp-console |
|
/retest |
…irectory useK8sWatchResource refactoring
cajieh
force-pushed
the
fix-olm-packages-e2e-tests-skipped-in-firhose-removal
branch
from
807b4ba to
77d38f4
Compare
cajieh
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
| {_.isFunction(customActionMenu) | ||
| ? customActionMenu(kindObj, data, extraResources) | ||
| : customActionMenu} | ||
| {hasData && |
There was a problem hiding this comment.
TODO: Temporary addition for CI testing. To be removed once (#16219) merged.
📝 WalkthroughWalkthroughThis changeset re-enables previously skipped integration tests for operator install and uninstall scenarios across three test suites. It enhances test reliability by adding explicit timeout options to Cypress selector commands and wait assertions. Component rendering logic is refined to gate the display of custom action menus and navigation pages on resource load states. Test cleanup procedures are extended to delete operand instances before deleting operator-scoped resources. Mock implementations for Kubernetes watch hooks are updated to report ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/packages/operator-lifecycle-manager/integration-tests-cypress/views/operator.view.ts`:
- Around line 95-101: The two hardcoded cy.wait(...) sleeps should be replaced
with deterministic readiness assertions: remove the cy.wait(3000) before
cy.byTestOperatorRow(operatorName).click() and instead assert the operator row
is visible and interactable (e.g., .should('be.visible') and
.should('not.have.attr', 'disabled')) before clicking; remove the trailing
cy.wait(2000) and replace it with an assertion that the horizontal nav finished
rendering by checking a stable element such as
cy.byLegacyTestID('horizontal-link-Details').should('be.visible', { timeout:
60000 }) (or other nav tab container visibility) after the
cy.url(...).should('include', 'ClusterServiceVersion') check so navigation
readiness is deterministic.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 19c05969-1cc0-44a3-b578-2d57649c2758
📒 Files selected for processing (10)
frontend/packages/helm-plugin/src/components/details-page/__tests__/HelmReleaseDetails.spec.tsxfrontend/packages/integration-tests-cypress/support/selectors.tsfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-global.cy.tsfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-single-namespace.cy.tsfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-uninstall.cy.tsfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/views/operator.view.tsfrontend/packages/operator-lifecycle-manager/src/components/clusterserviceversion.tsxfrontend/public/components/factory/__tests__/details.spec.tsxfrontend/public/components/utils/headings.tsxfrontend/public/components/utils/horizontal-nav.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/public/components/factory/__tests__/details.spec.tsxfrontend/public/components/utils/horizontal-nav.tsxfrontend/packages/operator-lifecycle-manager/src/components/clusterserviceversion.tsxfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-global.cy.tsfrontend/public/components/utils/headings.tsxfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-uninstall.cy.tsfrontend/packages/helm-plugin/src/components/details-page/__tests__/HelmReleaseDetails.spec.tsxfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-single-namespace.cy.tsfrontend/packages/integration-tests-cypress/support/selectors.tsfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/views/operator.view.ts
🔇 Additional comments (11)
frontend/public/components/utils/headings.tsx (1)
150-153: LGTM — Defensive gating onhasDataaligns with the component's existing guard pattern.This mirrors the guards already applied to
hasButtonActionsandhasMenuActionsat lines 134 and 138. The change protects against callers (likeoperand/index.tsxper the context snippets) that don't null-checkobjin theircustomActionMenucallbacks, preventing runtime errors during the loading phase.frontend/public/components/utils/horizontal-nav.tsx (1)
331-334: LGTM — PreventspagesForcallback from receiving undefined data.The guard ensures
props.pagesFor(props.obj.data)isn't invoked untilloadedis true. TheStatusBoxwrapper already handles the loading state visually, and plugin-derived pages remain available immediately since they don't depend on resource data.frontend/packages/operator-lifecycle-manager/src/components/clusterserviceversion.tsx (1)
1346-1355: LGTM — Guards action menu context against undefinedcsv.Passing
undefinedtocustomActionMenuwhencsvisn't loaded prevents theLazyActionMenufrom receiving an undefined resource in its context object. This aligns with thehasDatagate added inheadings.tsx.frontend/public/components/factory/__tests__/details.spec.tsx (1)
36-36: LGTM — Mock state aligns with updatedHorizontalNavguard.Since
HorizontalNavnow only derives base pages whenprops.obj?.loadedis true, the mocks must returnloaded: truefor the navigation tabs to render. The test assertions verify the expected UI presence correctly.Also applies to: 71-72
frontend/packages/helm-plugin/src/components/details-page/__tests__/HelmReleaseDetails.spec.tsx (1)
12-17: LGTM — Consistent mock setup withdetails.spec.tsx.The mock pattern mirrors
frontend/public/components/factory/__tests__/details.spec.tsx, ensuringLoadedHelmReleaseDetails(which usesDetailsPageinternally) renders tabs correctly after theHorizontalNavloading guard changes. ThebeforeEachreset ensures proper test isolation.Also applies to: 39-41
frontend/packages/integration-tests-cypress/support/selectors.ts (1)
31-38: Good update: selector helpers now support Cypress options end-to-end.The new optional
optionstyping and pass-through tocy.get(...)are consistent and make timeout-driven assertions in OLM tests much cleaner.Also applies to: 87-99
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/views/operator.view.ts (2)
103-104:horizontalNavTabtimeout parameter is a solid reliability improvement.Centralizing timeout control in this helper keeps tab navigation assertions consistent across tests.
139-140: Nice guard for create-flow completion.Waiting for URL to drop
~newafter submit makes the operand create path less race-prone.frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-uninstall.cy.ts (1)
25-25: Good re-enable plus timeout hardening.Reactivating this suite and adding explicit wait budgets for key details-page assertions should reduce flakes while restoring uninstall coverage.
Also applies to: 53-55
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-global.cy.ts (1)
22-26: Strong reliability improvements in setup/cleanup and assertions.Deleting operand instances first, re-enabling the suite, and adding explicit assertion timeouts all move this test toward more stable, repeatable execution.
Also applies to: 41-41, 59-61
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-single-namespace.cy.ts (1)
24-28: Nice parity with global-install flow.This brings single-namespace tests in line with the same cleanup-first and explicit-timeout strategy, which should help reduce intermittent failures.
Also applies to: 43-43, 68-70
| // Wait for operator row to be fully interactive | ||
| cy.wait(3000); | ||
| cy.byTestOperatorRow(operatorName).click(); | ||
| cy.url({ timeout: 30000 }).should('include', 'ClusterServiceVersion'); | ||
| cy.byLegacyTestID('horizontal-link-Details', { timeout: 60000 }).should('exist'); | ||
| // Extra wait to ensure all horizontal nav tabs render | ||
| cy.wait(2000); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify static waits in OLM Cypress flows and their locations.
rg -n --type=ts 'cy\.wait\(\d+\)' \
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/views/operator.view.ts \
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-*.cy.tsRepository: openshift/console
Length of output: 293
🏁 Script executed:
#!/bin/bash
# Read the full context around the disputed lines
sed -n '85,110p' frontend/packages/operator-lifecycle-manager/integration-tests-cypress/views/operator.view.tsRepository: openshift/console
Length of output: 1400
🏁 Script executed:
#!/bin/bash
# Check for custom command definitions
rg -n "byTestOperatorRow|byLegacyTestID" \
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/ \
--type=ts -A 2 | head -50Repository: openshift/console
Length of output: 6294
🏁 Script executed:
#!/bin/bash
# Find custom command definitions
find frontend/packages/operator-lifecycle-manager/integration-tests-cypress -name "*.ts" -o -name "*.js" | \
xargs grep -l "byTestOperatorRow\|byLegacyTestID" | grep -i commandRepository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Look for Cypress custom command definitions in support files
find frontend/packages/operator-lifecycle-manager/integration-tests-cypress -path "*/support/*" -type fRepository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Look for Cypress command definitions more broadly
rg -n "byTestOperatorRow|byLegacyTestID" --type=ts \
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/ \
-B 3 | grep -E "(export|Cypress.Commands)" -A 2 | head -30Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Search for command definitions in the broader Cypress support structure
find frontend -path "*/cypress/support*" -type f -name "*.ts" -o -name "*.js" | head -20Repository: openshift/console
Length of output: 845
🏁 Script executed:
#!/bin/bash
# Search more broadly for custom command definitions
rg -n "byTestOperatorRow|byLegacyTestID" --type=ts --type=js \
frontend/packages/operator-lifecycle-manager/ | grep -E "export|command|Command" -i | head -20Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Search the entire codebase for these command definitions
rg -n "byTestOperatorRow|byLegacyTestID" --type=ts --type=js -B 2 | \
grep -E "Cypress.Commands.add|export.*=" | head -20Repository: openshift/console
Length of output: 1986
🏁 Script executed:
#!/bin/bash
# Read the definition of byTestOperatorRow and byLegacyTestID
sed -n '83,100p' frontend/packages/integration-tests-cypress/support/selectors.tsRepository: openshift/console
Length of output: 631
🏁 Script executed:
#!/bin/bash
# Find byLegacyTestID definition
rg -n "byLegacyTestID" frontend/packages/integration-tests-cypress/support/selectors.ts -A 3 | head -15Repository: openshift/console
Length of output: 418
🏁 Script executed:
#!/bin/bash
# Get full definition of byTestOperatorRow to see if it returns
sed -n '83,88p' frontend/packages/integration-tests-cypress/support/selectors.tsRepository: openshift/console
Length of output: 258
🏁 Script executed:
#!/bin/bash
# Search for all cy.wait() calls in OLM integration tests (broader search)
rg -n "cy\.wait\(" \
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/ \
--type=tsRepository: openshift/console
Length of output: 572
Replace fixed cy.wait(...) sleeps with deterministic readiness checks.
Lines 96 and 101 inject 5 seconds of arbitrary latency into every details-page navigation. These hardcoded sleeps still risk flakiness on slower environments and violate the performance-first principle. Prefer visibility checks with explicit timeouts instead.
Suggested change
- cy.byTestOperatorRow(operatorName, { timeout: 30000 }).should('exist');
- // Wait for operator row to be fully interactive
- cy.wait(3000);
- cy.byTestOperatorRow(operatorName).click();
+ cy.byTestOperatorRow(operatorName, { timeout: 30000 }).should('be.visible').click();
cy.url({ timeout: 30000 }).should('include', 'ClusterServiceVersion');
- cy.byLegacyTestID('horizontal-link-Details', { timeout: 60000 }).should('exist');
- // Extra wait to ensure all horizontal nav tabs render
- cy.wait(2000);
+ cy.byLegacyTestID('horizontal-link-Details', { timeout: 60000 }).should('be.visible');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/operator-lifecycle-manager/integration-tests-cypress/views/operator.view.ts`
around lines 95 - 101, The two hardcoded cy.wait(...) sleeps should be replaced
with deterministic readiness assertions: remove the cy.wait(3000) before
cy.byTestOperatorRow(operatorName).click() and instead assert the operator row
is visible and interactable (e.g., .should('be.visible') and
.should('not.have.attr', 'disabled')) before clicking; remove the trailing
cy.wait(2000) and replace it with an assertion that the horizontal nav finished
rendering by checking a stable element such as
cy.byLegacyTestID('horizontal-link-Details').should('be.visible', { timeout:
60000 }) (or other nav tab container visibility) after the
cy.url(...).should('include', 'ClusterServiceVersion') check so navigation
readiness is deterministic.
|
@cajieh: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
…irectory useK8sWatchResource refactoring
Summary by CodeRabbit
Bug Fixes
Tests