OCPBUGS-79581: Fix Deprecated Operator Warnings E2E tests Skipped in the Firehose Public Folder PR#16197
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@cajieh: This pull request references Jira Issue OCPBUGS-79581, 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. |
openshift-ci-robot
added
the
jira/invalid-bug
openshift-ci
bot
added
component/core
cajieh
force-pushed
the
fix-e2e-tests-skipped-in-firehose-public-folder-refactoring-pr
branch
2 times, most recently
from
999b66b to
293ccf1
Compare
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@cajieh: This pull request references Jira Issue OCPBUGS-79581, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
cajieh
force-pushed
the
fix-e2e-tests-skipped-in-firehose-public-folder-refactoring-pr
branch
from
293ccf1 to
2320c8f
Compare
cajieh
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@cajieh: This pull request references Jira Issue OCPBUGS-79581, which is valid. 3 validation(s) were run on this bug
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. |
cajieh
force-pushed
the
fix-e2e-tests-skipped-in-firehose-public-folder-refactoring-pr
branch
from
2320c8f to
963e78c
Compare
📝 WalkthroughWalkthroughThis pull request updates Operator Lifecycle Manager integration tests and related utilities. The deprecated-operator-warnings test suite activates previously skipped UI test cases for badge rendering and warning displays across the Installed Operators, CSV details, and subscription pages. Test setup migrates to a ✨ 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.
🧹 Nitpick comments (2)
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/deprecated-operator-warnings.cy.ts (2)
191-205: Silent failure if InstallPlan lookup returns empty could lead to confusing downstream test failures.When
installPlanNameis empty, this returnscy.wrap(null)and proceeds, leaving the InstallPlan unapproved. Subsequent assertions will fail with unclear errors rather than an explicit "InstallPlan not found" message.Consider adding an explicit assertion or throwing an error if no InstallPlan is found, so failures are diagnosed quickly in CI logs.
💡 Suggested improvement
cy.exec( `oc get installplan -n ${subscriptionNamespace} -o jsonpath=` + `'{.items[?(@.spec.clusterServiceVersionNames[*]=="${csvName}")].metadata.name}'`, { timeout: 60000 }, ).then((result) => { const installPlanName = result.stdout.trim(); if (installPlanName) { return cy.exec( `oc patch installplan ${installPlanName} -n ${subscriptionNamespace} ` + `--type merge -p '{"spec":{"approved":true}}'`, { timeout: 60000 }, ); } - return cy.wrap(null); + throw new Error(`InstallPlan not found for CSV ${csvName} in ${subscriptionNamespace}`); });🤖 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/tests/deprecated-operator-warnings.cy.ts` around lines 191 - 205, The test currently silently continues when installPlanName is empty (the cy.exec.then block returns cy.wrap(null)), which causes confusing downstream failures; modify the then callback that reads installPlanName so that if installPlanName is falsy you explicitly fail the test (e.g., throw an Error or use cy.log + Cypress.fail/assert) with a clear message like "InstallPlan not found for CSV {csvName} in namespace {subscriptionNamespace}", otherwise proceed to the existing cy.exec patch call; update references in this block (installPlanName, subscriptionNamespace, csvName, and the cy.exec.then handler) to implement the explicit error path.
208-214:failOnNonZeroExit: falsemasks setup failures.If the CSV never succeeds or the
PackageDeprecatedcondition never appears, this step passes silently and the real failure surfaces later with less context. Consider either removingfailOnNonZeroExit: falseor loggingresult.stdout/result.stderrafterward to aid debugging flaky runs.🤖 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/tests/deprecated-operator-warnings.cy.ts` around lines 208 - 214, The cy.exec call that waits for CSV and subscription conditions uses failOnNonZeroExit: false which hides setup failures; update the cy.exec invocation (the call using cy.exec with the `oc wait csv/${csvName}` and `oc wait subscription/${subscriptionName}` commands) to either remove the failOnNonZeroExit: false option so failures fail the test immediately, or keep it but capture and log the returned result.stdout/result.stderr after the command completes so failing cases surface useful diagnostics (log the output with cy.log or processLogger).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/deprecated-operator-warnings.cy.ts`:
- Around line 191-205: The test currently silently continues when
installPlanName is empty (the cy.exec.then block returns cy.wrap(null)), which
causes confusing downstream failures; modify the then callback that reads
installPlanName so that if installPlanName is falsy you explicitly fail the test
(e.g., throw an Error or use cy.log + Cypress.fail/assert) with a clear message
like "InstallPlan not found for CSV {csvName} in namespace
{subscriptionNamespace}", otherwise proceed to the existing cy.exec patch call;
update references in this block (installPlanName, subscriptionNamespace,
csvName, and the cy.exec.then handler) to implement the explicit error path.
- Around line 208-214: The cy.exec call that waits for CSV and subscription
conditions uses failOnNonZeroExit: false which hides setup failures; update the
cy.exec invocation (the call using cy.exec with the `oc wait csv/${csvName}` and
`oc wait subscription/${subscriptionName}` commands) to either remove the
failOnNonZeroExit: false option so failures fail the test immediately, or keep
it but capture and log the returned result.stdout/result.stderr after the
command completes so failing cases surface useful diagnostics (log the output
with cy.log or processLogger).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 18d11ddf-c177-4286-9a9b-ba8182e160fa
📒 Files selected for processing (3)
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/deprecated-operator-warnings.cy.tsfrontend/packages/operator-lifecycle-manager/src/status/csv-status.tsfrontend/public/components/factory/list-page.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/list-page.tsxfrontend/packages/operator-lifecycle-manager/src/status/csv-status.tsfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/deprecated-operator-warnings.cy.ts
🔇 Additional comments (4)
frontend/public/components/factory/list-page.tsx (1)
133-133: Good fix for resource prop propagation to list components.Spreading
watchedResourcesintoListComponentrestores resource-keyed props (for example, subscription/catalog sources) while still passing normalizeddata, which aligns with the intended compatibility behavior.frontend/packages/operator-lifecycle-manager/src/status/csv-status.ts (1)
24-25: LGTM — defensive fallback for namespace resolution.The fallback to
csvNamespacewhenoperatorNamespaceForreturns falsy is correct for scenarios where theolm.operatorNamespaceannotation is absent (e.g., single-namespace installs). The guard at line 30 properly short-circuits if both values are undefined, preventing spurious matches. This aligns with consumers inclusterserviceversion.tsxanddashboard/utils.tsthat rely on accurate subscription lookup for deprecation and status display.frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/deprecated-operator-warnings.cy.ts (2)
11-13: Good approach using common substrings.Using
.contains()with these simplified strings provides resilience against minor text variations between pre- and post-installation deprecation messages while still validating the essential warning content.
217-275: Test cases are well-structured with appropriate async handling.Good use of
TIMEOUTfor subscription-dependent elements andbyLegacyTestIDfor nav links that usedata-test-id. The assertion flow covers badge, warning messages, and subscription-update modal interaction comprehensively.
|
/test e2e-gcp-console |
|
/retest |
|
/test all |
|
/retest |
|
@cajieh: 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. |
|
@cajieh: you cannot LGTM your own PR. 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 kubernetes-sigs/prow repository. |
openshift-ci-robot
added
the
verified
|
@cajieh: This PR has been marked to be verified later by 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cajieh, sg00dwin 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 |
3 participants
Root Cause:
This is a regression caused by the migration from the legacy Firehose component to React hooks. The refactor altered the data flow, causing the Deprecated badge to disappear from the 'Installed Operators' list page because ClusterServiceVersionList no longer receives the Subscription data required to determine deprecation status.
Summary Changes:
ListPageWrapperto spread{...watchedResources}toListComponent, ensuring subscriptions and catalogSources data reaches ClusterServiceVersionList.byTestIDtobyLegacyTestIDfor horizontal nav links (horizontal-link-Details, horizontal-link-Subscription) since they usedata-test-id.TIMEOUTto async element checks in tests 7-8 for subscription data loadingBefore:
After:
depreciated.operator.warninh.-.new.mov