WIP: OCPBUGS-78044: Improve WatchList test robustness#2622
WIP: OCPBUGS-78044: Improve WatchList test robustness#2622jacobsee wants to merge 1 commit intoopenshift:masterfrom
Conversation
The WatchList test “[sig-api-machinery] API Streaming (aka. WatchList) [FeatureGate:WatchList] [Beta] should be requested by metadatainformer when WatchListClient is enabled” works by fetching an expected (initial) state of secrets, starting an informer, and polling until context timeout for the informer to converge to the expected state. If any secret in the namespace changes while the test is running, they never converge, and the test times out. This change limits the secrets we’re listing to just the ones relevant to the test.
openshift-ci-robot
added
the
backports/unvalidated-commits
openshift-ci
bot
added
do-not-merge/work-in-progress
openshift-ci-robot
added
jira/valid-reference
|
@jacobsee: This pull request references Jira Issue OCPBUGS-78044, 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
bot
added
the
kind/flake
|
@jacobsee: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
WalkthroughThis change adds a label selector filter Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jacobsee 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 |
|
@jacobsee: This pull request references Jira Issue OCPBUGS-78044, which is invalid:
Comment 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/apimachinery/watchlist.go (1)
106-108: Extract the watchlist selector into a shared constant.
"watchlist=true"is now duplicated across the informer, the directListcalls, and the request assertion helper. Hoisting the label key/value/selector into one place will make this test less brittle if the filter ever changes.♻️ Suggested cleanup
+const ( + watchlistLabelKey = "watchlist" + watchlistLabelValue = "true" + watchlistLabelSelector = watchlistLabelKey + "=" + watchlistLabelValue +) + ... func(options *metav1.ListOptions) { - options.LabelSelector = "watchlist=true" + options.LabelSelector = watchlistLabelSelector }, ... - expectedSecrets, err := metadataClient.Resource(v1.SchemeGroupVersion.WithResource("secrets")).Namespace(f.Namespace.Name).List(ctx, metav1.ListOptions{LabelSelector: "watchlist=true"}) + expectedSecrets, err := metadataClient.Resource(v1.SchemeGroupVersion.WithResource("secrets")).Namespace(f.Namespace.Name).List(ctx, metav1.ListOptions{LabelSelector: watchlistLabelSelector}) ... - expectedSecrets, err = metadataClient.Resource(v1.SchemeGroupVersion.WithResource("secrets")).Namespace(f.Namespace.Name).List(ctx, metav1.ListOptions{LabelSelector: "watchlist=true"}) + expectedSecrets, err = metadataClient.Resource(v1.SchemeGroupVersion.WithResource("secrets")).Namespace(f.Namespace.Name).List(ctx, metav1.ListOptions{LabelSelector: watchlistLabelSelector}) ... - Labels: map[string]string{"watchlist": "true"}, + Labels: map[string]string{watchlistLabelKey: watchlistLabelValue},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/apimachinery/watchlist.go` around lines 106 - 108, Create a shared constant for the watchlist label selector (e.g., WatchlistLabelSelector) and replace all hard-coded occurrences of "watchlist=true" with that constant: update the metav1.ListOptions lambda where options.LabelSelector is set, the direct List(...) calls, and the request assertion helper that validates the selector; keep the constant close to the test package (same file or a shared test helper) and reference it by name wherever LabelSelector is compared or assigned so the selector value is maintained in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/e2e/apimachinery/watchlist.go`:
- Around line 106-108: Create a shared constant for the watchlist label selector
(e.g., WatchlistLabelSelector) and replace all hard-coded occurrences of
"watchlist=true" with that constant: update the metav1.ListOptions lambda where
options.LabelSelector is set, the direct List(...) calls, and the request
assertion helper that validates the selector; keep the constant close to the
test package (same file or a shared test helper) and reference it by name
wherever LabelSelector is compared or assigned so the selector value is
maintained in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6764aef9-d11e-405c-ab5f-a0f2ba8f7b9c
📒 Files selected for processing (1)
test/e2e/apimachinery/watchlist.go
|
/retest |
|
/payload-aggregate periodic-ci-openshift-release-main-ci-4.22-upgrade-from-stable-4.21-e2e-gcp-ovn-rt-upgrade |
|
/payload-aggregate periodic-ci-openshift-release-main-ci-4.22-upgrade-from-stable-4.21-e2e-gcp-ovn-rt-upgrade 10 |
|
@jacobsee: 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/cfef87d0-1ca1-11f1-91fe-856567d27159-0 |
|
/payload-aggregate periodic-ci-openshift-release-main-ci-4.22-upgrade-from-stable-4.21-e2e-gcp-ovn-rt-upgrade 10 |
|
@jacobsee: 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/74497360-215a-11f1-9801-a8403ecd0929-0 |
|
/test e2e-aws-ovn-hypershift |
|
@jacobsee: The following tests 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. |
3 participants
What type of PR is this?
/kind bug
/kind flake
What this PR does / why we need it:
The WatchList test
[sig-api-machinery] API Streaming (aka. WatchList) [FeatureGate:WatchList] [Beta] should be requested by metadatainformer when WatchListClient is enabledworks by fetching an expected (initial) state of secrets, starting an informer, and polling until context timeout for the informer to converge to the expected state. If any secret in the namespace changes while the test is running, they never converge, and the test times out. This change limits the secrets we’re listing to just the ones relevant to the test.NOTE: This PR is in this state for testing. Once I'm confident this is the issue, this should really be submitted upstream.
Which issue(s) this PR is related to:
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
Summary by CodeRabbit