OCPBUGS-78534: Make E2E test create helper idempotent and improve test cleanup#16148
OCPBUGS-78534: Make E2E test create helper idempotent and improve test cleanup#16148cajieh wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci
bot
added
component/olm
📝 WalkthroughWalkthroughThis pull request refactors Kubernetes CLI operations and test infrastructure cleanup procedures. The integration-tests-cypress support file transitions from ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![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/tests/deprecated-operator-warnings.cy.ts`:
- Around line 64-65: The afterEach catalogsource deletion is missing the
non-blocking flag; update the teardown command that uses catalogSourceName and
catalogSourceNamespace in the afterEach hook to include --wait=false (to match
the beforeEach invocation) so the oc delete call becomes non-blocking and avoids
blocking teardown under load; locate the afterEach block where the command
string is built and append --wait=false to the oc delete invocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: bd32f783-3a3e-4087-bcca-e6cf64a4c19c
📒 Files selected for processing (2)
frontend/packages/integration-tests-cypress/support/index.tsfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/deprecated-operator-warnings.cy.ts
📜 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/packages/integration-tests-cypress/support/index.tsfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/deprecated-operator-warnings.cy.ts
🧬 Code graph analysis (1)
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/deprecated-operator-warnings.cy.ts (1)
frontend/packages/integration-tests-cypress/support/index.ts (1)
create(126-134)
🔇 Additional comments (2)
frontend/packages/integration-tests-cypress/support/index.ts (1)
132-132: Idempotentoc applychange looks correct.Good change for rerun safety and CI flake reduction in resource setup.
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/deprecated-operator-warnings.cy.ts (1)
29-56: Cleanup centralization is a solid reliability improvement.Extracting
cleanupOperatorResources()and reusing it in setup reduces duplication and makes teardown behavior consistent.
| `oc delete catalogsource ${catalogSourceName} -n ${catalogSourceNamespace} --ignore-not-found`, | ||
| { failOnNonZeroExit: false, timeout: 60000 }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify catalogsource teardown commands are consistently non-blocking.
# Expected: both before/after delete commands include --wait=false.
rg -n --type=ts -C2 'oc delete catalogsource .*--ignore-not-found'Repository: openshift/console
Length of output: 1637
Add --wait=false to afterEach catalogsource deletion for consistent non-blocking teardown.
Line 64's afterEach hook omits --wait=false while line 52's beforeEach hook includes it. This asymmetry can cause test flakes under load, as teardown may block on async catalogsource deletion. Align both teardown operations.
Fix
- `oc delete catalogsource ${catalogSourceName} -n ${catalogSourceNamespace} --ignore-not-found`,
+ `oc delete catalogsource ${catalogSourceName} -n ${catalogSourceNamespace} --ignore-not-found --wait=false`,📝 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.
| `oc delete catalogsource ${catalogSourceName} -n ${catalogSourceNamespace} --ignore-not-found`, | |
| { failOnNonZeroExit: false, timeout: 60000 }, | |
| `oc delete catalogsource ${catalogSourceName} -n ${catalogSourceNamespace} --ignore-not-found --wait=false`, | |
| { failOnNonZeroExit: false, timeout: 60000 }, |
🤖 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 64 - 65, The afterEach catalogsource deletion is missing the
non-blocking flag; update the teardown command that uses catalogSourceName and
catalogSourceNamespace in the afterEach hook to include --wait=false (to match
the beforeEach invocation) so the oc delete call becomes non-blocking and avoids
blocking teardown under load; locate the afterEach block where the command
string is built and append --wait=false to the oc delete invocation.
The create() helper function used `oc create` which fails with "AlreadyExists" if the resource exists from a previous failed test run. This causes flaky test failures in CI, particularly affecting the "Deprecated operator warnings" test suite. Changed to `oc apply` which is idempotent - it creates the resource if it doesn't exist, or updates it if it does. This improves test reliability when runs don't fully clean up or when resources persist between test runs. Made-with: Cursor
cajieh
force-pushed
the
fix-flaky-deprecated-operator-test
branch
from
079758e to
e93f6f0
Compare
|
/retest |
cajieh
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@cajieh: This pull request references Jira Issue OCPBUGS-78534, 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@cajieh: This pull request references Jira Issue OCPBUGS-78534, 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. |
|
/retest |
|
@cajieh: This pull request references Jira Issue OCPBUGS-78534, which is valid. 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. |
Add cleanup helpers to both global and single-namespace Data Grid operator installation tests. This addresses flaky test failures caused by leftover resources from previous test runs. Changes: - Add cleanupOperatorResources() helper to delete Subscriptions, CSVs, and InstallPlans with --ignore-not-found --wait=false flags - Call cleanup before operator installation to handle dirty state - Add after() hook to global install test for consistent teardown - Add cleanup to single-namespace test's after() hook This follows the same pattern used in the deprecated-operator-warnings test fix (PR openshift#16148) and improves test reliability in CI environments. Made-with: Cursor
|
[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 |
|
/verified later @yapei |
openshift-ci-robot
added
verified-later
verified
|
/retest |
|
@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. |
|
|
|
@cajieh: once the present PR merges, I will cherry-pick it on top of 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. |
|
@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. |
4 participants
Summary:
oc createtooc applyfor idempotent resource creation.Using
oc applyinstead ofoc createfollows Kubernetes best practices for declarative resource managementand is more reliable for test infrastructure.
The
create()helper function usedoc createwhich fails with "AlreadyExists" error if the resource exists from a previous run. This causes flaky test failures in CI due to:Changed to
oc applywhich is idempotent - it creates the resource if it doesn't exist, or updates it if it does. This improves test reliability regardless of prior cluster state.Issues fixed:
Specific: Resolves the flaky "Deprecated operator warnings" test suite that was failing 51% of the time (9% overall CI impact) with consistent
oc createfailures for thekiali.subscription.jsonresource.Broader Impact: The
create()helper is used by other test suites, so they will all benefit from this change.Summary by CodeRabbit
Release Notes