OPRUN-4392,OPRUN-4393: Add OLMv1 progress deadline QE tests + fixes#755
OPRUN-4392,OPRUN-4393: Add OLMv1 progress deadline QE tests + fixes#755dtfranz wants to merge 1 commit into
Conversation
Automate the ClusterExtension rollout failure coverage for OCP-88331 and OCP-88332 by building in-cluster bundle and catalog images for successful and failing bundle versions. The new QE specs verify ProgressDeadlineExceeded on an initial failed rollout and ProbeFailure while upgrading to a bad revision under the BoxCutter runtime. Signed-off-by: Daniel Franz <dfranz@redhat.com> Co-authored-by: Bruno Andrade <bruno.balint@gmail.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@dtfranz: This pull request references OPRUN-4392 which is a valid jira issue. This pull request references OPRUN-4393 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 story 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. |
WalkthroughAdds a new Ginkgo/Gomega integration test file ChangesClusterExtension progress deadline QE test
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dtfranz 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 |
![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
🧹 Nitpick comments (1)
openshift/tests-extension/test/qe/specs/olmv1_ce_progress_deadline.go (1)
405-416: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting the 12-minute timeout rationale.
The 12-minute timeout here is notably longer than other assertions (3-5 minutes). While this is correct for waiting beyond the 10-minute progress deadline set in test case 88331, a comment explaining the relationship would improve maintainability.
📝 Optional improvement
func expectClusterObjectSetCondition(ctx context.Context, name, conditionType string, status metav1.ConditionStatus, reason string) { + // Timeout is 12 minutes to accommodate the 10-minute progress deadline in test case 88331, + // plus buffer time for controller processing and status updates. eventually(func(g o.Gomega) {🤖 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 `@openshift/tests-extension/test/qe/specs/olmv1_ce_progress_deadline.go` around lines 405 - 416, Add a comment above the eventually function call in expectClusterObjectSetCondition to document why the 12-minute timeout is used. The comment should explain that this timeout is intentionally longer than other assertions (3-5 minutes) to wait beyond the 10-minute progress deadline configured in test case 88331, establishing the relationship between the timeout value and the deadline requirement for maintainability.
🤖 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 `@openshift/tests-extension/test/qe/specs/olmv1_ce_progress_deadline.go`:
- Around line 263-264: The error message in the o.Expect assertion for the oc
start-build command includes the full output variable, which can contain
extensive build logs violating QE guidelines. Remove the output variable from
the o.Expect error message assertion at the line where start-build is run and
Run method is called, and instead log the output separately using g.By() before
the assertion or truncate the output to a reasonable size if the error needs to
include diagnostic information. This ensures large build logs are handled
through proper logging mechanisms rather than being embedded in the error
expectation message.
---
Nitpick comments:
In `@openshift/tests-extension/test/qe/specs/olmv1_ce_progress_deadline.go`:
- Around line 405-416: Add a comment above the eventually function call in
expectClusterObjectSetCondition to document why the 12-minute timeout is used.
The comment should explain that this timeout is intentionally longer than other
assertions (3-5 minutes) to wait beyond the 10-minute progress deadline
configured in test case 88331, establishing the relationship between the timeout
value and the deadline requirement for maintainability.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2af580ca-f800-4db0-8859-d59b8a61f185
📒 Files selected for processing (2)
openshift/tests-extension/.openshift-tests-extension/openshift_payload_olmv1.jsonopenshift/tests-extension/test/qe/specs/olmv1_ce_progress_deadline.go
| output, err := oc.AsAdmin().WithoutNamespace().Run("start-build").Args(name, "-n", namespace, "--from-archive="+archive, "--wait").Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to build image %s: %s", name, output) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Don't include large build output in error messages.
The output variable from oc start-build could contain extensive build logs (hundreds of lines). Including this directly in the o.Expect error message violates the QE guideline: "Don't put large log outputs in error messages (use proper log messages instead of o.Expect with large output)".
Consider logging the output separately with g.By() or truncating it:
♻️ Suggested fix
output, err := oc.AsAdmin().WithoutNamespace().Run("start-build").Args(name, "-n", namespace, "--from-archive="+archive, "--wait").Output()
-o.Expect(err).NotTo(o.HaveOccurred(), "failed to build image %s: %s", name, output)
+if err != nil {
+ g.By(fmt.Sprintf("Build output for %s:\n%s", name, output))
+ o.Expect(err).NotTo(o.HaveOccurred(), "failed to build image %s", name)
+}Or truncate the output:
output, err := oc.AsAdmin().WithoutNamespace().Run("start-build").Args(name, "-n", namespace, "--from-archive="+archive, "--wait").Output()
-o.Expect(err).NotTo(o.HaveOccurred(), "failed to build image %s: %s", name, output)
+truncated := output
+if len(truncated) > 500 {
+ truncated = truncated[:500] + "... (truncated)"
+}
+o.Expect(err).NotTo(o.HaveOccurred(), "failed to build image %s: %s", name, truncated)🤖 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 `@openshift/tests-extension/test/qe/specs/olmv1_ce_progress_deadline.go` around
lines 263 - 264, The error message in the o.Expect assertion for the oc
start-build command includes the full output variable, which can contain
extensive build logs violating QE guidelines. Remove the output variable from
the o.Expect error message assertion at the line where start-build is run and
Run method is called, and instead log the output separately using g.By() before
the assertion or truncate the output to a reasonable size if the error needs to
include diagnostic information. This ensures large build logs are handled
through proper logging mechanisms rather than being embedded in the error
expectation message.
Source: Coding guidelines
|
/retest |
|
@dtfranz: 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
Automate the ClusterExtension rollout failure coverage for OCP-88331 and OCP-88332 by building in-cluster bundle and catalog images for successful and failing bundle versions.
The new QE specs verify ProgressDeadlineExceeded on an initial failed rollout and ProbeFailure while upgrading to a bad revision under the BoxCutter runtime.
Supersedes #745
Additional changes:
httpdscriptTest pass shown here: PR, CI run
Summary by CodeRabbit