🌱 tests: address CodeRabbit review comments from downstream PR#2672
🌱 tests: address CodeRabbit review comments from downstream PR#2672tmshort wants to merge 1 commit intooperator-framework:mainfrom
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Addresses downstream review feedback by making tests more robust, improving logging/diagnostics in option parsing, and bounding concurrency during e2e cleanup.
Changes:
- Make
TestCatalog_FBCGenerationindependent of channel insertion order by looking up channels by name. - Add logging for invalid
LargeCRD(...)counts and cap concurrentkubectl deletecalls in scenario cleanup. - Small docs/shell cleanups (README clarification, fenced code block language, remove extra blank line).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/internal/catalog/catalog_test.go | Makes the channel assertions resilient to ordering changes. |
| test/internal/catalog/catalog.go | Renames local var to avoid shadowing tag parameter. |
| test/extension-developer-e2e/setup.sh | Removes an extra blank line. |
| test/e2e/steps/steps.go | Logs and skips invalid LargeCRD(...) counts. |
| test/e2e/steps/hooks.go | Adds a semaphore to bound concurrent deletions. |
| test/e2e/README.md | Clarifies conditional deletion behavior under feature gate. |
| docs/designs/testing/2026-04-13-e2e-isolation/design.md | Adds text language identifier to code fence. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2672 +/- ##
=======================================
Coverage 67.99% 67.99%
=======================================
Files 144 144
Lines 10573 10573
=======================================
Hits 7189 7189
Misses 2865 2865
Partials 519 519
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR updates the e2e test harness, test catalog builder, and supporting docs/scripts to incorporate review feedback from a downstream PR—primarily improving robustness, observability, and bounding cleanup concurrency.
Changes:
- Make
TestCatalog_FBCGenerationresilient to channel ordering by selecting channels by name. - Improve e2e step parsing by logging and skipping invalid
LargeCRD(...)counts rather than silently ignoring them. - Add a concurrency bound to scenario cleanup deletes and apply small docs/script formatting fixes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/internal/catalog/catalog_test.go | Avoids relying on channel insertion order by finding channels by name. |
| test/internal/catalog/catalog.go | Renames local tag variable to bundleTag to avoid shadowing the Build parameter. |
| test/extension-developer-e2e/setup.sh | Removes an extra blank line. |
| test/e2e/steps/steps.go | Logs and skips invalid LargeCRD(...) parse errors in bundle option parsing. |
| test/e2e/steps/hooks.go | Introduces a semaphore to bound concurrent kubectl delete calls during cleanup. |
| test/e2e/README.md | Clarifies ClusterObjectSet deletion depends on BoxcutterRuntime gate being enabled. |
| docs/designs/testing/2026-04-13-e2e-isolation/design.md | Adds text language identifier to a fenced code block. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tmshort
force-pushed
the
fix-coderabbit-707
branch
from
15c8df6 to
e1b5a6e
Compare
tmshort
force-pushed
the
fix-coderabbit-707
branch
from
e1b5a6e to
f24245d
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates test and documentation utilities to incorporate prior review feedback, improving robustness (ordering independence), clarity (docs), and operational safety (bounded cleanup concurrency) in the operator-controller e2e/test tooling.
Changes:
- Make
TestCatalog_FBCGenerationresilient to channel ordering by looking channels up by name. - Update the e2e bundle “contents” parser to validate
LargeCRD(...)counts and propagate parse errors with bundle context. - Bound concurrent
kubectl deletecalls during scenario cleanup using a limited-concurrencyerrgroup, plus small doc/script formatting fixes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/internal/catalog/catalog_test.go | Finds channels by name to avoid assuming slice insertion order. |
| test/internal/catalog/catalog.go | Renames a local variable to avoid shadowing the tag parameter. |
| test/extension-developer-e2e/setup.sh | Removes an extra blank line. |
| test/e2e/steps/steps.go | Changes parseContents to return errors and validates LargeCRD(...) counts. |
| test/e2e/steps/hooks.go | Uses errgroup.Group with a concurrency limit for cleanup deletions. |
| test/e2e/README.md | Clarifies ClusterObjectSet deletion is gated by BoxcutterRuntime. |
| docs/designs/testing/2026-04-13-e2e-isolation/design.md | Adds a text language specifier to a fenced code block. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tmshort
force-pushed
the
fix-coderabbit-707
branch
from
f24245d to
18b8052
Compare
- Rename local variable `tag` to `bundleTag` in catalog.Build to avoid shadowing the method's `tag` parameter - Fail the step with an error on invalid LargeCRD field counts in the bundle option parser; use strconv.ParseInt with bitSize 0 to also reject non-positive values - Look up channels by name in TestCatalog_FBCGeneration instead of assuming insertion order - Use errgroup.Group with SetLimit(8) in ScenarioCleanup deletion loop to bound concurrent goroutines and kubectl calls - Clarify README that ClusterObjectSet deletion is conditional on the BoxcutterRuntime feature gate - Add missing `text` language specifier to fenced code block in e2e-isolation design doc - Remove extra blank line in setup.sh Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Todd Short <tshort@redhat.com>
tmshort
force-pushed
the
fix-coderabbit-707
branch
from
18b8052 to
19e1d71
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR applies a set of test, e2e, and documentation adjustments in response to downstream review feedback, aiming to make tests more robust, improve failure visibility in bundle parsing, and bound concurrency during e2e cleanup.
Changes:
- Make catalog channel assertions robust to ordering changes and avoid variable shadowing in catalog build logic.
- Propagate parsing errors for bundle options (notably invalid
LargeCRD(...)) so scenarios fail loudly instead of silently dropping options. - Bound concurrent
kubectldeletions during scenario cleanup; clarify docs and minor shell/doc formatting tweaks.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/internal/catalog/catalog_test.go | Looks up channels by name to avoid relying on insertion order in tests. |
| test/internal/catalog/catalog.go | Renames local tag to bundleTag to avoid shadowing. |
| test/extension-developer-e2e/setup.sh | Removes an extra blank line. |
| test/e2e/steps/steps.go | Makes bundle option parsing return errors; validates LargeCRD count. |
| test/e2e/steps/hooks.go | Uses errgroup with a concurrency limit for cleanup deletions. |
| test/e2e/README.md | Clarifies conditional deletion of ClusterObjectSet by feature gate. |
| docs/designs/testing/2026-04-13-e2e-isolation/design.md | Adds text language identifier to fenced code block. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
Reviewer Checklist