CNTRLPLANE-3863: improve v2 test isolation - #9229
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ironcladlou: This pull request references CNTRLPLANE-3863 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 epic 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. |
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe E2E framework removes cached HostedCluster state and legacy validation methods. Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ 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 |
|
Should be rebased once #9228 lands |
openshift-ci
Bot
added
the
area/platform/aws
openshift-ci
Bot
added
area/platform/azure
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9229 +/- ##
=======================================
Coverage 45.67% 45.67%
=======================================
Files 781 781
Lines 97726 97726
=======================================
Hits 44641 44641
Misses 50019 50019
Partials 3066 3066
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
test/e2e/v2/internal/test_context.go (1)
120-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the exported TestContext helper behavior.
Add doc comments for the exported helpers. Document that they fetch current HostedCluster state. Document that
Must*and version helpers fail the active Ginkgo spec on retrieval failure. Document the skip conditions and return values forSkipIfVersionBelowandSkipIfNotPlatform.As per coding guidelines, “Comments on exported functions must describe actual behavior, including panic conditions, not just intended behavior.”
🤖 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/internal/test_context.go` around lines 120 - 180, Add Go doc comments to the exported TestContext helpers MustGetHostedCluster, VersionAtLeast, MustGetHostedClusterClient, MustGetHostedClusterRESTConfig, SkipIfVersionBelow, SkipIfNotPlatform, and SkipIfPlatform, describing their current HostedCluster retrieval behavior, Ginkgo-spec failure on retrieval errors for Must* and version helpers, skip conditions, and return values for the version and platform helpers.Sources: Coding guidelines, Path instructions
🤖 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 `@test/e2e/v2/internal/test_context.go`:
- Around line 61-77: Add focused unit tests in the internal test package
covering getHostedClusterVersion for missing or empty version history, invalid
version strings returning an error, and valid versions normalized by clearing
Patch, Pre, and Build while preserving the major and minor components. Verify
the returned HostedCluster and semver values, and cover the behavior relied on
by versionAtLeast and skipIfVersionBelow.
In `@test/e2e/v2/tests/control_plane_upgrade_test.go`:
- Around line 56-61: Move both SkipIfVersionBelow checks, including the
Version420 and Version422 gates, before the e2eutil.UpdateObject mutation in the
upgrade test. Ensure every skip condition is evaluated before changing
HostedCluster.Spec.Release.Image or hyperv1.ForceUpgradeToAnnotation, and remove
the post-mutation skip paths so ineligible clusters are never partially
upgraded.
In `@test/e2e/v2/tests/hosted_cluster_aws_test.go`:
- Around line 113-123: Update the Infrastructure polling inside the Eventually
callback to use a cleanup-safe context instead of tc.Context when calling
hcClient.Get. Use context.Background() or a bounded context derived from it so
verification remains functional after test-context cancellation while preserving
the existing polling assertions.
In `@test/e2e/v2/tests/hosted_cluster_image_registry_test.go`:
- Line 59: Refresh the HostedCluster used by the GCP context by assigning the
result of the current TestContext helper call to the local hc variable before
the WorkloadIdentity checks. Update the relevant hook around
MustGetHostedCluster so subsequent assertions use the refreshed object rather
than the outer BeforeAll-captured state.
In `@test/e2e/v2/tests/nodepool_lifecycle_test.go`:
- Around line 841-842: Replace the fail-fast MustGetHostedCluster call in the
trust-bundle cleanup with a direct MgmtClient.Get into currentHC; return when
the error is apierrors.IsNotFound, and log other fetch errors before continuing
cleanup. Pass currentHC to UpdateObject so the outer hc is not shadowed,
preserving cleanup’s log-and-continue behavior.
- Around line 484-485: Reorder the setup in the nodepool lifecycle test so
SkipIfNotPlatform obtains the hosted cluster before calling
MustGetHostedClusterClient. Remove or avoid the later manual AWS/Azure platform
check, and create hcClient only after the centralized skip helper allows the
test to proceed.
---
Nitpick comments:
In `@test/e2e/v2/internal/test_context.go`:
- Around line 120-180: Add Go doc comments to the exported TestContext helpers
MustGetHostedCluster, VersionAtLeast, MustGetHostedClusterClient,
MustGetHostedClusterRESTConfig, SkipIfVersionBelow, SkipIfNotPlatform, and
SkipIfPlatform, describing their current HostedCluster retrieval behavior,
Ginkgo-spec failure on retrieval errors for Must* and version helpers, skip
conditions, and return values for the version and platform helpers.
🪄 Autofix
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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 734cc4dc-8406-48d3-a04c-300a68693aa5
📒 Files selected for processing (28)
test/e2e/util/version.gotest/e2e/v2/backuprestore/cleanup.gotest/e2e/v2/internal/test_context.gotest/e2e/v2/internal/workload_registry.gotest/e2e/v2/tests/backup_restore_test.gotest/e2e/v2/tests/control_plane_pki_operator_test.gotest/e2e/v2/tests/control_plane_upgrade_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_azure_test.gotest/e2e/v2/tests/hosted_cluster_ccm_test.gotest/e2e/v2/tests/hosted_cluster_compliance_test.gotest/e2e/v2/tests/hosted_cluster_cpo_test.gotest/e2e/v2/tests/hosted_cluster_dns_test.gotest/e2e/v2/tests/hosted_cluster_external_oidc_test.gotest/e2e/v2/tests/hosted_cluster_health_test.gotest/e2e/v2/tests/hosted_cluster_image_registry_test.gotest/e2e/v2/tests/hosted_cluster_ingress_test.gotest/e2e/v2/tests/hosted_cluster_metrics_test.gotest/e2e/v2/tests/hosted_cluster_node_communication_test.gotest/e2e/v2/tests/hosted_cluster_psc_test.gotest/e2e/v2/tests/hosted_cluster_pull_secret_test.gotest/e2e/v2/tests/hosted_cluster_secret_encryption_test.gotest/e2e/v2/tests/hosted_cluster_security_test.gotest/e2e/v2/tests/nodepool_autoscaling_test.gotest/e2e/v2/tests/nodepool_lifecycle_test.gotest/e2e/v2/tests/nodepool_osimagestream_test.go
💤 Files with no reviewable changes (1)
- test/e2e/util/version.go
8a95ddf to
f6a826e
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/pipeline required |
f6a826e to
21f79cc
Compare
|
/pipeline required |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Scheduling tests matching the |
There was a problem hiding this comment.
Overall: Strong direction — removing cached shared mutable state is the right call for test isolation, and the new helpers make the right thing easy. A few items need to be addressed before merge, including AGENTS.md violations and two behavioral regressions. See inline comments for details on each.
Blocking
-
MustGetHostedCluster()insideEventuallywill abort retries on transient failures —verifyAPIReachableinhosted_cluster_azure_test.gocallsMustGetHostedCluster()inside anEventuallyclosure. It uses top-levelExpect(notg.Expect), so a transient API error immediately fails the test instead of retrying. This is especially risky here since this function is called during endpoint access transitions where transient errors are expected. Fix: fetch the HC before theEventually, or use the privategetHostedCluster()withg.Expectinside the closure. -
EnsureAdmissionPoliciesTestdrops theIsPublicHCskip guard — inhosted_cluster_security_test.go, the old code skipped on private clusters via!netutil.IsPublicHC(hostedCluster). The new code removes this guard and thenetutilimport entirely. The test description itself says "checking admission policies on a public hosted cluster" but the guard enforcing that is gone. This will cause admission policy tests to run (and likely fail) on private clusters. TheIsPublicHCcheck needs to be restored.
Must fix (AGENTS.md violations)
-
Missing docstrings on all 7 new exported methods (AGENTS.md Standard 12) —
MustGetHostedCluster,MustGetHostedClusterClient,MustGetHostedClusterRESTConfig,VersionAtLeast,SkipIfVersionBelow,SkipIfNotPlatform,SkipIfPlatformall lack doc comments. Standard 12 requires: "Comments on exported functions must describe actual behavior including panic conditions." The old methods they replace had them. These need doc comments describing that they fail the test via GomegaExpecton error (not panic), and forSkipIfVersionBelow/SkipIfNotPlatform, that they callginkgo.Skip. -
AGENTS.md Standard 3 must be updated — Standard 3 still says "
GetHostedCluster()usessync.Onceto fetch lazily and panics on failure." That function no longer exists. The standard must be updated to document the newMustGetHostedCluster+ GomegaExpectcontract. A PR that changes framework behavior without updating the framework's own documented standards is incomplete. -
Lost
HostedClusterConfiguredskip guard violates Fail-Loud philosophy (AGENTS.md Standard 3) — The oldValidateHostedCluster()cleanly skipped with "no hosted cluster configured for this test run" when env vars were unset. NowMustGetHostedCluster()will attempt a Get with an empty key and fail with an opaque API error. Standard 3 requires diagnostic messages, not opaque failures. Consider adding a guard inMustGetHostedClusterthat skips whenClusterNameorClusterNamespaceare empty. -
Dead nil-checks after
MustGetHostedCluster—MustGetHostedClusterfails viaExpectand never returns nil, making nil-checks unreachable in 4 locations. These were introduced by the mechanical refactor and need to be cleaned up:control_plane_workloads_test.goDeploymentGenerationTest:if hostedCluster == nil || ...— remove the nil check, keep theCreationTimestamp.IsZero()checkhosted_cluster_image_registry_test.goImageRegistryCapabilityDisabledTest:if hc == nil { Skip(...) }— remove entirelyhosted_cluster_secret_encryption_test.goBeforeEach:if hc == nil || hc.Spec.SecretEncryption...— removehc == nil ||, keep the resthosted_cluster_azure_test.goverifyAPIReachable:g.Expect(hc).NotTo(BeNil(), ...)— redundant assertion, remove
7385afe to
a99b744
Compare
|
/pipeline required |
|
Scheduling tests matching the |
|
Rebased |
|
/retest |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, ironcladlou 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 |
|
/test e2e-kubevirt-aws-ovn-reduced |
|
/test e2e-kubevirt-aws-ovn-reduced |
1 similar comment
|
/test e2e-kubevirt-aws-ovn-reduced |
|
/test e2e-v2-azure-self-managed |
|
/verified by e2e regression analysis |
openshift-ci-robot
added
the
verified
|
@ironcladlou: 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. |
|
/test e2e-kubevirt-aws-ovn-reduced |
|
/test e2e-kubevirt-aws-ovn-reduced |
|
/override ci/prow/e2e-kubevirt-aws-ovn-reduced |
|
@csrwng: Overrode contexts on behalf of csrwng: ci/prow/e2e-kubevirt-aws-ovn-reduced 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 kubernetes-sigs/prow repository. |
|
/test e2e-v2-azure-self-managed |
|
@ironcladlou: 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. |
7 participants
Before this commit, v2 tests relied on cached, shared mutable hostedcluster state via TestContext (and indirectly through version.releaseVersion which was derived from the shared hostedcluster). This contradicts our goals to move tests towards full process isolation at the test case granularity and is a vector for tricky bugs as tests can easily mutate the shared hostedcluster pointer or get a stale view.
This commit removes all caching of the hostedcluster and releaseVersion from the v2 path, and refactors dependent code to use a variety of new test helpers for uniform access and consistent patterns.
A significant amount of boilerplate is reduced, and the helpers make it easy to do the right thing and hard to do the wrong thing (i.e. access shared mutable state).
The primary implication of these changes is that tests which access the hostedcluster will fetch through the client every time, but in the e2e context this overhead is negligable (and if we wanted to it could be addressed in other, safer ways than sharing a cached pointer to a snapshot).
Summary by CodeRabbit