OCPQE-31636: enhance QE cases#666
Conversation
Xia-Zhao-rh
force-pushed
the
enhance_qe_cases
branch
from
aa73ea2 to
ff818b6
Compare
|
/payload-job periodic-ci-openshift-operator-framework-operator-controller-release-4.22-periodics-e2e-aws-ovn-techpreview-extended-f1 |
|
@Xia-Zhao-rh: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/4ecf9ec0-2402-11f1-9ee4-95fcbb0a9364-0 |
|
/payload-job periodic-ci-openshift-operator-framework-operator-controller-release-4.22-periodics-e2e-aws-ovn-techpreview-extended-f1 |
|
@Xia-Zhao-rh: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/6f364d30-272c-11f1-887d-eaf7e04e7e0b-0 |
|
/verified by @Xia-Zhao-rh OCP-75218 is failed due to OCPBUGS-77829 |
openshift-ci-robot
added
the
verified
|
@Xia-Zhao-rh: This PR has been marked as verified 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. |
WalkthroughTest specification files for OLMv1 functionality are updated to use new fixture templates with labels and selectors, propagate LabelValue into object descriptions, and refactor polling/condition assertion logic to accept broader success criteria and handle errors more gracefully. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ 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 |
Xia-Zhao-rh
changed the title
|
@Xia-Zhao-rh: This pull request references OCPQE-31636 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 "4.22.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. |
openshift-ci-robot
added
the
jira/valid-reference
Xia-Zhao-rh
force-pushed
the
enhance_qe_cases
branch
from
ff818b6 to
1cf20c7
Compare
openshift-ci-robot
removed
the
verified
|
/payload-job periodic-ci-openshift-operator-framework-operator-controller-release-4.22-periodics-e2e-aws-ovn-techpreview-extended-f1 |
|
@Xia-Zhao-rh: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/e752fe20-2732-11f1-9bd9-7e39fde4c7a0-0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openshift/tests-extension/test/qe/specs/olmv1_ce.go`:
- Around line 2242-2247: The current checks use generic substrings like "already
exists" or just "Installed" which can match unrelated failures; update the
predicate(s) that inspect the variable message (the strings.Contains(...)
branches) to require a conflict-specific marker such as the word "Conflicting"
combined with the specific conflicting object or CRD name (e.g., "Conflicting:
<CRD/ResourceName>") instead of accepting any "already exists" or only
"Installed"==Failed/False; change all occurrences that use
strings.Contains(message, "already exists") or only check for "Installed"
(including the blocks referencing message and the other occurrences you noted
around the second block and lines 2957-2962) to assert strings.Contains(message,
"Conflicting") && strings.Contains(message, "<expected-CRD-or-resource>") or
equivalent exact-match logic so the test only passes on true ownership
conflicts.
- Around line 2833-2838: The predicate is too permissive because it returns
success on any message that mentions "ns-80117-watch"; update the check around
the variable message (from the call to olmv1util.GetNoEmpty) to require the
specific "namespace not found" failure (or the exact phrase the controller
emits, e.g. `namespace "ns-80117-watch" not found`) rather than any mention of
the namespace; replace the current strings.Contains(message, "ns-80117-watch")
condition with a stricter test (for example require both
strings.Contains(message, "ns-80117-watch") && strings.Contains(message, "not
found") or a regex match for the exact error phrase) so that return true only
when the expected namespace-not-found path is observed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a2f8f9f-cdab-4384-b0e5-dc4e75f8b16c
📒 Files selected for processing (2)
openshift/tests-extension/test/qe/specs/olmv1_cc.goopenshift/tests-extension/test/qe/specs/olmv1_ce.go
| if strings.Contains(message, "already exists") || strings.Contains(message, "Conflicting") { | ||
| e2e.Logf("status is %s", message) | ||
| return false, nil | ||
| return true, nil | ||
| } | ||
| return true, nil | ||
| e2e.Logf("status is %s", message) | ||
| return false, nil |
There was a problem hiding this comment.
Keep these conflict checks tied to the actual conflict.
Line 2242 and Line 2957 now accept any generic already exists, and Lines 2280-2281 only require Installed to be Failed/False. That can make the tests pass on unrelated failures (stale leftovers, RBAC issues, image pull failures) even if the ownership-conflict path regresses. Please keep a conflict-specific marker in the assertion as well, such as Conflicting plus the conflicting object/CRD name.
Also applies to: 2280-2281, 2957-2962
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openshift/tests-extension/test/qe/specs/olmv1_ce.go` around lines 2242 -
2247, The current checks use generic substrings like "already exists" or just
"Installed" which can match unrelated failures; update the predicate(s) that
inspect the variable message (the strings.Contains(...) branches) to require a
conflict-specific marker such as the word "Conflicting" combined with the
specific conflicting object or CRD name (e.g., "Conflicting:
<CRD/ResourceName>") instead of accepting any "already exists" or only
"Installed"==Failed/False; change all occurrences that use
strings.Contains(message, "already exists") or only check for "Installed"
(including the blocks referencing message and the other occurrences you noted
around the second block and lines 2957-2962) to assert strings.Contains(message,
"Conflicting") && strings.Contains(message, "<expected-CRD-or-resource>") or
equivalent exact-match logic so the test only passes on true ownership
conflicts.
| message, _ := olmv1util.GetNoEmpty(oc, "clusterextension", clusterextension.Name, "-o", "jsonpath={.status.conditions[*].message}") | ||
| if !strings.Contains(message, "failed to create resource") { | ||
| if strings.Contains(message, "failed to create resource") || strings.Contains(message, "ns-80117-watch") { | ||
| e2e.Logf("status is %s", message) | ||
| return false, nil | ||
| return true, nil | ||
| } | ||
| return true, nil | ||
| return false, nil |
There was a problem hiding this comment.
This missing-watch-namespace predicate is too broad.
Line 2834 will pass on any error that merely mentions ns-80117-watch, so the test can go green without proving the expected namespace not found path.
Suggested tightening
errWait := wait.PollUntilContextTimeout(context.TODO(), 3*time.Second, 150*time.Second, false, func(ctx context.Context) (bool, error) {
message, _ := olmv1util.GetNoEmpty(oc, "clusterextension", clusterextension.Name, "-o", "jsonpath={.status.conditions[*].message}")
- if strings.Contains(message, "failed to create resource") || strings.Contains(message, "ns-80117-watch") {
+ lowerMessage := strings.ToLower(message)
+ if strings.Contains(message, "ns-80117-watch") &&
+ (strings.Contains(message, "failed to create resource") || strings.Contains(lowerMessage, "not found")) {
e2e.Logf("status is %s", message)
return true, nil
}
return false, nil
})📝 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.
| message, _ := olmv1util.GetNoEmpty(oc, "clusterextension", clusterextension.Name, "-o", "jsonpath={.status.conditions[*].message}") | |
| if !strings.Contains(message, "failed to create resource") { | |
| if strings.Contains(message, "failed to create resource") || strings.Contains(message, "ns-80117-watch") { | |
| e2e.Logf("status is %s", message) | |
| return false, nil | |
| return true, nil | |
| } | |
| return true, nil | |
| return false, nil | |
| message, _ := olmv1util.GetNoEmpty(oc, "clusterextension", clusterextension.Name, "-o", "jsonpath={.status.conditions[*].message}") | |
| lowerMessage := strings.ToLower(message) | |
| if strings.Contains(message, "ns-80117-watch") && | |
| (strings.Contains(message, "failed to create resource") || strings.Contains(lowerMessage, "not found")) { | |
| e2e.Logf("status is %s", message) | |
| return true, nil | |
| } | |
| return false, nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openshift/tests-extension/test/qe/specs/olmv1_ce.go` around lines 2833 -
2838, The predicate is too permissive because it returns success on any message
that mentions "ns-80117-watch"; update the check around the variable message
(from the call to olmv1util.GetNoEmpty) to require the specific "namespace not
found" failure (or the exact phrase the controller emits, e.g. `namespace
"ns-80117-watch" not found`) rather than any mention of the namespace; replace
the current strings.Contains(message, "ns-80117-watch") condition with a
stricter test (for example require both strings.Contains(message,
"ns-80117-watch") && strings.Contains(message, "not found") or a regex match for
the exact error phrase) so that return true only when the expected
namespace-not-found path is observed.
|
/verified by @Xia-Zhao-rh OCP-75218 is failed due to OCPBUGS-77829 |
openshift-ci-robot
added
the
verified
|
@Xia-Zhao-rh: This PR has been marked as verified 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 |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jianzhangbjz, Xia-Zhao-rh 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
the
approved
|
@Xia-Zhao-rh: 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. |
4 participants
This PR enhances QE test cases for OLM v1 ClusterCatalog and ClusterExtension by improving test isolation, reliability, and assertion logic.
Key improvements:
- Fixed upgrade verification logic in test 75122 using polling mechanism instead of direct condition checks
- Updated conflict detection assertions in tests 74923 and 80117 to properly validate error messages
- Enhanced wait conditions to be more flexible and accurate (checking for "already exists" OR "Conflicting" instead of exact strings)
- Added better error logging and status output for debugging failures