NO-JIRA: test(e2e/v2): sweep AGENTS.md conformance violations#8659
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@cblecker: This pull request explicitly references no jira issue. 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughWalkthroughThis PR modernizes e2e tests: replace ad-hoc contexts with testCtx.Context, prefer hosted-cluster client (hcClient) for hosted-scoped operations, add precondition assertions (non-nil fields and non-empty lists), skip workloads with no matching pods, and consolidate cleanup to DeferCleanup with stricter error assertions. Sequence Diagram(s)No diagram is needed. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
do-not-merge/needs-area
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
area/platform/aws
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cblecker 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
area/testing
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8659 +/- ##
==========================================
+ Coverage 41.27% 41.39% +0.12%
==========================================
Files 755 756 +1
Lines 93446 93613 +167
==========================================
+ Hits 38566 38750 +184
+ Misses 52148 52134 -14
+ Partials 2732 2729 -3 see 2 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/test e2e-v2-aws e2e-v2-gke |
![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/v2/tests/nodepool_lifecycle_test.go (1)
856-860: ⚡ Quick winConsider using retry logic for HostedCluster update in cleanup.
The cleanup now uses a plain
Updatewhich will fail if the HostedCluster has been modified concurrently (resourceVersion conflict). While this is more strict than the previous warning-based approach, it could cause flakiness.♻️ Consider using e2eutil.UpdateObject for retry on conflict
DeferCleanup(func() { - hc := testCtx.GetHostedCluster() - hc.Spec.AdditionalTrustBundle = nil - Expect(testCtx.MgmtClient.Update(ctx, hc)).To(Succeed(), "cleanup: failed to remove additional trust bundle") + Expect(e2eutil.UpdateObject(GinkgoTB(), ctx, testCtx.MgmtClient, testCtx.GetHostedCluster(), func(obj *hyperv1.HostedCluster) { + obj.Spec.AdditionalTrustBundle = nil + })).To(Succeed(), "cleanup: failed to remove additional trust bundle") })🤖 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 `@test/e2e/v2/tests/nodepool_lifecycle_test.go` around lines 856 - 860, The cleanup currently calls testCtx.MgmtClient.Update(ctx, hc) directly which can fail on resourceVersion conflicts; change it to use an update-with-retry helper (e.g., e2eutil.UpdateObject or a RetryOnConflict loop) so the HostedCluster modification is retried on conflict: retrieve the HostedCluster (testCtx.GetHostedCluster()/or pass the current object), call e2eutil.UpdateObject(ctx, testCtx.MgmtClient, hc, func(obj) { obj.Spec.AdditionalTrustBundle = nil }) or wrap the Update in client.RetryOnConflict to set hc.Spec.AdditionalTrustBundle = nil and retry until success, then assert Succeed() as before.
🤖 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.
Nitpick comments:
In `@test/e2e/v2/tests/nodepool_lifecycle_test.go`:
- Around line 856-860: The cleanup currently calls
testCtx.MgmtClient.Update(ctx, hc) directly which can fail on resourceVersion
conflicts; change it to use an update-with-retry helper (e.g.,
e2eutil.UpdateObject or a RetryOnConflict loop) so the HostedCluster
modification is retried on conflict: retrieve the HostedCluster
(testCtx.GetHostedCluster()/or pass the current object), call
e2eutil.UpdateObject(ctx, testCtx.MgmtClient, hc, func(obj) {
obj.Spec.AdditionalTrustBundle = nil }) or wrap the Update in
client.RetryOnConflict to set hc.Spec.AdditionalTrustBundle = nil and retry
until success, then assert Succeed() as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 01fbd05c-1b77-45ea-a846-ebe42292fdaa
📒 Files selected for processing (11)
test/e2e/v2/tests/control_plane_infrastructure_test.gotest/e2e/v2/tests/control_plane_workloads_test.gotest/e2e/v2/tests/etcd_chaos_test.gotest/e2e/v2/tests/hosted_cluster_aws_test.gotest/e2e/v2/tests/hosted_cluster_ccm_test.gotest/e2e/v2/tests/hosted_cluster_health_test.gotest/e2e/v2/tests/hosted_cluster_image_registry_test.gotest/e2e/v2/tests/hosted_cluster_metrics_test.gotest/e2e/v2/tests/hosted_cluster_security_test.gotest/e2e/v2/tests/nodepool_autoscaling_test.gotest/e2e/v2/tests/nodepool_lifecycle_test.go
Audit all test files in test/e2e/v2/tests/ against the 18 established standards in AGENTS.md and fix every confirmed violation: - Rule 1 (Terminology): rename guestClient→hcClient throughout etcd, nodepool lifecycle, and autoscaling tests; rename EnsureGuestWebhooksValidatedTest; fix "guest Prometheus" string - Rule 2 (File Org): add ValidateHostedCluster() to CCM test BeforeEach - Rule 6 (Context): replace context.Background() with tc.Context in workloads and CCM tests; drop now-unused "context" import in CCM - Rule 9 (DeferCleanup): convert 13 defer cleanupNodePool and 5 defer cleanupWorkload calls to DeferCleanup in lifecycle/autoscaling tests; add missing DeferCleanup for 4 ConfigMap creates and 1 Service create; convert trust bundle defer func() closure; fix Warning Printf→Expect in all DeferCleanup blocks - Rule 11 (Pointer Safety): nil-check Status.Platform.AWS and Spec.Platform.GCP.WorkloadIdentity before dereference - Rule 16 (Vacuous Pass): add non-empty assertions before pod/component loops in workloads, infrastructure, and health tests; add namespacesChecked counter; add Skip when matchingPods is empty Assisted-by: Claude:claude-sonnet-4-6[1m]
cblecker
force-pushed
the
test/e2ev2-agents-conformance-sweep
branch
from
e61ca19 to
5340f3a
Compare
|
/test e2e-v2-aws e2e-v2-gke |
|
/verified by e2e |
openshift-ci-robot
added
the
verified
|
@cblecker: 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. |
|
/pipeline required |
|
Scheduling tests matching the |
|
I have all the evidence needed. The failure is clear — it's a CI infrastructure transient error, not related to the PR code changes. Let me compile the final report: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe job failed instantly during ci-operator namespace initialization on the Root CauseThe root cause is a transient Kubernetes API conflict (optimistic concurrency failure) in the CI infrastructure, specifically on the What happened step by step:
This is a known class of transient CI infrastructure failure. The ci-operator lacks retry logic for namespace initialization conflicts. Recommendations
Evidence
|
Test Resultse2e-aws
e2e-aks
|
|
/retest-required |
|
/lgtm |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
@cblecker: 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
Summary
guestClient→hcClientin etcd, nodepool lifecycle, and autoscaling tests; renameEnsureGuestWebhooksValidatedTest; fix"guest Prometheus"string in metrics testValidateHostedCluster()to CCM test top-levelBeforeEachcontext.Background()withtc.Contextin workloads and CCM tests; drop now-unused"context"import in CCM testdefer cleanupNodePooland 5defer cleanupWorkloadcalls toDeferCleanupin lifecycle/autoscaling tests; add missingDeferCleanupfor 4 ConfigMap creates and 1 Service create; convert trust bundledefer func()closure; fix allGinkgoWriter.Printf("Warning: ...")cleanup patterns to useExpectStatus.Platform.AWSandSpec.Platform.GCP.WorkloadIdentitybefore dereference in AWS and image registry testsnamespacesCheckedcounter in infrastructure test; addSkipwhenmatchingPodsis emptyTest plan
make e2ev2builds cleanlymake backuprestore-e2ebuilds cleanlymake testpasses (unit tests)grep -rn 'guestClient' test/e2e/v2/tests/→ 0 matchesgrep -rn 'EnsureGuest' test/e2e/v2/tests/→ 0 matchesgrep -rn 'context\.Background()' test/e2e/v2/tests/→ onlysuite_test.go:53grep -rn 'defer cleanup\|defer func' test/e2e/v2/tests/→ 0 matchesgrep -rn 'GinkgoWriter.Printf.*Warning.*cleanup' test/e2e/v2/tests/→ 0 matchesSummary by CodeRabbit