🌱 Fix integration tests and add comprehensive placement API tests #396
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughAdds six new Ginkgo/Gomega integration test suites under test/integration/api for v1/v1beta1 APIs: AppliedManifestWork, ClusterManager, Klusterlet, ManagedCluster, ManifestWork, and Placement. Each suite creates unique resources, exercises creation/validation/update/status paths, uses Kubernetes core types where needed, and cleans up via deletion. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
- Fix ManifestWork propagation policy validation tests to match integration environment behavior - Add required lastTransitionTime fields to condition objects in status updates - Add comprehensive placement API integration tests with creation, validation, and update scenarios - Fix AppliedManifestWork tests to use proper required fields - Update ManagedCluster tests to handle validation environment limitations All 115 integration tests now pass successfully. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Jian Qiu <[email protected]>
qiujian16
force-pushed
the
fix/integration-tests-and-add-placement-api
branch
from
13d2ac9 to
46bb202
Compare
![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 (24)
test/integration/api/appliedmanifestwork_test.go (3)
23-28: Tighten cleanup: ignore NotFound only, fail on other errors.Currently all errors are silently ignored. Gate only NotFound and surface unexpected failures.
Apply this diff:
ginkgo.AfterEach(func() { - err := hubWorkClient.WorkV1().AppliedManifestWorks().Delete(context.TODO(), appliedManifestWorkName, metav1.DeleteOptions{}) - if err != nil { - // Ignore not found errors in cleanup - } + err := hubWorkClient.WorkV1().AppliedManifestWorks().Delete(context.TODO(), appliedManifestWorkName, metav1.DeleteOptions{}) + if err != nil && !apierrors.IsNotFound(err) { + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + } })Add missing import:
import ( "context" "fmt" "github.com/onsi/ginkgo" "github.com/onsi/gomega" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/rand" workv1 "open-cluster-management.io/api/work/v1" )
47-61: Test name vs. body mismatch.Test “should handle … applied resources” never exercises status/appliedResources. Either rename or add a minimal status update to validate the field.
80-98: Avoid setting UID in status; it’s controller-owned.UID in AppliedManifestResourceMeta is “not directly settable by a client.” Drop it to align with API semantics and reduce future breakage if server-side validation tightens.
Suggested tweak:
- Version: "v1", - UID: "test-uid-123", + Version: "v1",test/integration/api/klusterlet_test.go (3)
35-41: Use HaveOccurred for error assertions.Prefer
Expect(err).To(HaveOccurred())overNotTo(BeNil())for clarity and consistency.- _, err := operatorClient.OperatorV1().Klusterlets().Create(context.TODO(), klusterlet, metav1.CreateOptions{}) - Expect(err).NotTo(BeNil()) + _, err := operatorClient.OperatorV1().Klusterlets().Create(context.TODO(), klusterlet, metav1.CreateOptions{}) + Expect(err).To(HaveOccurred())
15-33: Add cleanup to avoid leaked Klusterlets in this suite.This Describe creates CRs without deleting them. Add AfterEach with NotFound‑tolerant delete to keep the test env clean.
Example:
AfterEach(func() { err := operatorClient.OperatorV1().Klusterlets().Delete(context.TODO(), klusterlet.Name, metav1.DeleteOptions{}) if err != nil && !apierrors.IsNotFound(err) { Expect(err).NotTo(HaveOccurred()) } })And import:
apierrors "k8s.io/apimachinery/pkg/api/errors"
76-125: Add cleanup for HostAlias suite as well.Same leak risk here; mirror the AfterEach pattern to delete the created Klusterlet and ignore NotFound.
test/integration/api/manifestwork_test.go (3)
409-416: Make the “empty propagation policy” test robust against CRD defaulting.Defaulting may set Foreground when DeleteOption is present but policy is omitted, making this test flaky. Force an explicit invalid value to assert validation.
- Spec: workv1.ManifestWorkSpec{ - DeleteOption: &workv1.DeleteOption{}, - }, + Spec: workv1.ManifestWorkSpec{ + DeleteOption: &workv1.DeleteOption{ + PropagationPolicy: workv1.DeletePropagationPolicyType(""), + }, + },
561-613: Minor: avoid []string trick for string pointer.Use a local var for readability.
- StatusFeedbacks: workv1.StatusFeedbackResult{ - Values: []workv1.FeedbackValue{ - { - Name: "status", - Value: workv1.FieldValue{ - Type: workv1.String, - String: &[]string{"Applied"}[0], - }, - }, - }, - }, + StatusFeedbacks: workv1.StatusFeedbackResult{ + Values: []workv1.FeedbackValue{ + { + Name: "status", + Value: func() workv1.FieldValue { + s := "Applied" + return workv1.FieldValue{Type: workv1.String, String: &s} + }(), + }, + }, + },
18-37: Add cleanup for this Describe to prevent resource accumulation.This suite creates ManifestWorks but doesn’t delete them. Add an AfterEach like the enhanced suite below.
Example:
ginkgo.AfterEach(func() { err := hubWorkClient.WorkV1().ManifestWorks(testNamespace).Delete(context.TODO(), manifestWorkName, metav1.DeleteOptions{}) if err != nil && !apierrors.IsNotFound(err) { gomega.Expect(err).NotTo(gomega.HaveOccurred()) } })Import if missing:
apierrors "k8s.io/apimachinery/pkg/api/errors"test/integration/api/placement_test.go (1)
159-175: Optional: add a negative NumberOfClusters test (<=0) to lock validation.Covers a common misconfig and guards against regressions in CRD validation.
test/integration/api/clustermanager_test.go (5)
16-35: Add cleanup for this suite to avoid leaked ClusterManagers.Resources created here aren’t deleted. Add AfterEach with NotFound‑tolerant delete.
Example:
AfterEach(func() { err := operatorClient.OperatorV1().ClusterManagers().Delete(context.TODO(), clusterManagerName, metav1.DeleteOptions{}) if err != nil && !apierrors.IsNotFound(err) { Expect(err).NotTo(HaveOccurred()) } })Import:
apierrors "k8s.io/apimachinery/pkg/api/errors"
53-123: Also add cleanup for the Hosted mode suite.Same reasoning; add an AfterEach to delete the created CR.
157-177: Typo in test description.“euqually” → “equally”.
- It("should have euqually value after create", func() { + It("should have equally valued fields after create", func() {
180-199: Add cleanup for RegistrationConfiguration suite.Mirror the NotFound‑tolerant deletion pattern to avoid accumulating CRs.
312-331: Add cleanup for WorkConfiguration suite.Same leak risk; add an AfterEach to delete CRs created in this block.
test/integration/api/managedcluster_test.go (9)
4-16: Don't swallow cleanup errors; ignore only NotFound.Cleanup should fail the test on unexpected errors.
Apply:
import ( "context" "fmt" "time" "github.com/onsi/ginkgo" "github.com/onsi/gomega" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/rand" clusterv1 "open-cluster-management.io/api/cluster/v1" )ginkgo.AfterEach(func() { - err := hubClusterClient.ClusterV1().ManagedClusters().Delete(context.TODO(), clusterName, metav1.DeleteOptions{}) - if err != nil { - // Ignore not found errors in cleanup - } + err := hubClusterClient.ClusterV1().ManagedClusters().Delete(context.TODO(), clusterName, metav1.DeleteOptions{}) + if err != nil && !apierrors.IsNotFound(err) { + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + } })Also applies to: 122-127
348-352: Fix test name vs. patch type mismatch.The test uses JSON Merge Patch, not Strategic Merge Patch (not supported for CRDs).
-ginkgo.It("should support strategic merge patch for taints", func() { +ginkgo.It("should support merge patch for taints", func() {
101-111: Assert the patched taint value to avoid false positives.Verify value changed to "testnew".
_, err = hubClusterClient.ClusterV1().ManagedClusters().Patch( context.TODO(), cluster.Name, types.MergePatchType, []byte(`{"spec":{"taints":[{"key":"test.io/test","value":"testnew","effect":"NoSelect"}]}}`), metav1.PatchOptions{}, ) gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + // Verify patch effect + cluster, err = hubClusterClient.ClusterV1().ManagedClusters().Get(context.TODO(), cluster.Name, metav1.GetOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(cluster.Spec.Taints).To(gomega.HaveLen(1)) + gomega.Expect(cluster.Spec.Taints[0].Value).To(gomega.Equal("testnew"))
192-193: Use HaveLen matcher for clarity.- gomega.Expect(len(cluster.Spec.ManagedClusterClientConfigs)).Should(gomega.Equal(2)) + gomega.Expect(cluster.Spec.ManagedClusterClientConfigs).Should(gomega.HaveLen(2))- gomega.Expect(len(cluster.Spec.Taints)).Should(gomega.Equal(2)) + gomega.Expect(cluster.Spec.Taints).Should(gomega.HaveLen(2))Also applies to: 382-383
54-54: Correct the comment: value is optional.- // key, value, effect is required + // key and effect are required; value is optional
319-319: Prefer metav1.Now() over NewTime(time.Now()).-now := metav1.NewTime(time.Now()) +now := metav1.Now()
324-331: Simplify quantities with MustParse and explicit units.- Allocatable: clusterv1.ResourceList{ - "cpu": *resource.NewQuantity(4, resource.DecimalSI), - "memory": *resource.NewQuantity(8*1024*1024*1024, resource.BinarySI), - }, - Capacity: clusterv1.ResourceList{ - "cpu": *resource.NewQuantity(4, resource.DecimalSI), - "memory": *resource.NewQuantity(8*1024*1024*1024, resource.BinarySI), - }, + Allocatable: clusterv1.ResourceList{ + "cpu": resource.MustParse("4"), + "memory": resource.MustParse("8Gi"), + }, + Capacity: clusterv1.ResourceList{ + "cpu": resource.MustParse("4"), + "memory": resource.MustParse("8Gi"), + },
130-130: Rename for accuracy.-ginkgo.It("should accept client config without validation", func() { +ginkgo.It("should store client config URL unchanged", func() {
166-168: Also assert CABundle round-trips.-_, err := hubClusterClient.ClusterV1().ManagedClusters().Create(context.TODO(), managedCluster, metav1.CreateOptions{}) -gomega.Expect(err).ToNot(gomega.HaveOccurred()) +createdCluster, err := hubClusterClient.ClusterV1().ManagedClusters().Create(context.TODO(), managedCluster, metav1.CreateOptions{}) +gomega.Expect(err).ToNot(gomega.HaveOccurred()) +gomega.Expect(createdCluster.Spec.ManagedClusterClientConfigs[0].CABundle).Should(gomega.Equal([]byte("dummy-ca-bundle")))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
test/integration/api/appliedmanifestwork_test.go(1 hunks)test/integration/api/clustermanager_test.go(2 hunks)test/integration/api/klusterlet_test.go(2 hunks)test/integration/api/managedcluster_test.go(2 hunks)test/integration/api/manifestwork_test.go(2 hunks)test/integration/api/placement_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
test/integration/api/appliedmanifestwork_test.go (1)
work/v1/types.go (5)
AppliedManifestWork(668-678)AppliedManifestWorkSpec(681-693)AppliedManifestWorkStatus(696-712)AppliedManifestResourceMeta(449-462)ResourceIdentifier(391-411)
test/integration/api/placement_test.go (2)
cluster/v1beta1/types_placement.go (6)
Placement(41-53)PlacementSpec(58-103)ClusterPredicate(169-178)ClusterSelector(182-194)Toleration(369-400)TolerationOpExists(407-407)cluster/v1/types.go (1)
TaintEffectNoSelect(124-124)
test/integration/api/klusterlet_test.go (4)
operator/v1/types_klusterlet.go (7)
Klusterlet(19-28)KlusterletSpec(31-103)ServerURL(106-115)KlusterletDeployOption(335-346)RegistrationConfiguration(133-189)WorkAgentConfiguration(277-327)KlusterletStatus(349-370)cluster/v1beta1/types_placement.go (2)
Toleration(369-400)TolerationOpExists(407-407)operator/v1/types_clustermanager.go (7)
InstallModeDefault(441-441)FeatureGate(311-324)FeatureGateModeTypeEnable(330-330)WorkConfiguration(260-285)InstallModeHosted(445-445)GenerationStatus(503-527)RelatedResourceMeta(479-499)operator/v1/type_resourcerequirement.go (2)
ResourceRequirement(11-18)ResourceQosClassResourceRequirement(28-28)
test/integration/api/manifestwork_test.go (1)
work/v1/types.go (26)
String(591-591)ManifestWork(18-28)Manifest(60-64)DeleteOption(73-95)DeletePropagationPolicyTypeForeground(373-373)DeletePropagationPolicyTypeOrphan(376-376)ManifestConfigOption(98-120)ResourceIdentifier(391-411)UpdateStrategy(223-242)ServerSideApplyConfig(274-291)FeedbackRule(313-329)JSONPathsType(341-341)JsonPath(344-365)ManifestWorkExecutor(173-177)ManifestWorkExecutorSubject(182-193)ExecutorSubjectTypeServiceAccount(201-201)ManifestWorkSubjectServiceAccount(205-220)ManifestWorkStatus(465-479)WorkApplied(500-500)ManifestResourceStatus(483-492)ManifestCondition(524-536)ManifestResourceMeta(417-445)StatusFeedbackResult(539-545)FeedbackValue(547-559)FieldValue(563-584)ManifestApplied(601-601)
test/integration/api/managedcluster_test.go (1)
cluster/v1/types.go (10)
ManagedCluster(34-44)ManagedClusterSpec(48-77)ClientConfig(81-90)Taint(94-116)TaintEffectNoSelect(124-124)TaintEffectNoSelectIfNew(131-131)ManagedClusterStatus(146-175)ManagedClusterVersion(179-183)ResourceList(249-249)ManagedClusterConditionAvailable(232-232)
test/integration/api/clustermanager_test.go (3)
operator/v1/types_clustermanager.go (15)
ClusterManager(18-29)ClusterManagerSpec(32-84)NodePlacement(87-97)ClusterManagerDeployOption(413-433)InstallModeDefault(441-441)RegistrationHubConfiguration(99-123)FeatureGate(311-324)FeatureGateModeTypeEnable(330-330)WorkConfiguration(260-285)WorkDriverTypeKube(292-292)AddOnManagerConfiguration(299-309)ServerConfiguration(166-176)ClusterManagerStatus(455-476)GenerationStatus(503-527)RelatedResourceMeta(479-499)cluster/v1beta1/types_placement.go (2)
Toleration(369-400)TolerationOpExists(407-407)operator/v1/type_resourcerequirement.go (2)
ResourceRequirement(11-18)ResourceQosClassResourceRequirement(28-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: verify
| ginkgo.AfterEach(func() { | ||
| err := hubClusterClient.ClusterV1beta1().Placements(testNamespace).Delete(context.TODO(), placementName, metav1.DeleteOptions{}) | ||
| if err != nil { | ||
| // Ignore not found errors during cleanup | ||
| gomega.Expect(err).ToNot(gomega.HaveOccurred()) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Cleanup block contradicts comment; it will fail on any error.
You intend to ignore NotFound, but Expect(err).ToNot(HaveOccurred()) runs only when err != nil, causing failure. Gate on NotFound.
Apply this diff:
ginkgo.AfterEach(func() {
err := hubClusterClient.ClusterV1beta1().Placements(testNamespace).Delete(context.TODO(), placementName, metav1.DeleteOptions{})
- if err != nil {
- // Ignore not found errors during cleanup
- gomega.Expect(err).ToNot(gomega.HaveOccurred())
- }
+ if err != nil && !apierrors.IsNotFound(err) {
+ gomega.Expect(err).ToNot(gomega.HaveOccurred())
+ }
})Add missing import:
import (
"context"
"fmt"
"github.com/onsi/ginkgo"
"github.com/onsi/gomega"
+ apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/rand"
clusterv1 "open-cluster-management.io/api/cluster/v1"
clusterv1beta1 "open-cluster-management.io/api/cluster/v1beta1"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ginkgo.AfterEach(func() { | |
| err := hubClusterClient.ClusterV1beta1().Placements(testNamespace).Delete(context.TODO(), placementName, metav1.DeleteOptions{}) | |
| if err != nil { | |
| // Ignore not found errors during cleanup | |
| gomega.Expect(err).ToNot(gomega.HaveOccurred()) | |
| } | |
| }) | |
| import ( | |
| "context" | |
| "fmt" | |
| "github.com/onsi/ginkgo" | |
| "github.com/onsi/gomega" | |
| apierrors "k8s.io/apimachinery/pkg/api/errors" | |
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | |
| "k8s.io/apimachinery/pkg/util/rand" | |
| clusterv1 "open-cluster-management.io/api/cluster/v1" | |
| clusterv1beta1 "open-cluster-management.io/api/cluster/v1beta1" | |
| ) |
| ginkgo.AfterEach(func() { | |
| err := hubClusterClient.ClusterV1beta1().Placements(testNamespace).Delete(context.TODO(), placementName, metav1.DeleteOptions{}) | |
| if err != nil { | |
| // Ignore not found errors during cleanup | |
| gomega.Expect(err).ToNot(gomega.HaveOccurred()) | |
| } | |
| }) | |
| ginkgo.AfterEach(func() { | |
| err := hubClusterClient.ClusterV1beta1().Placements(testNamespace).Delete(context.TODO(), placementName, metav1.DeleteOptions{}) | |
| if err != nil && !apierrors.IsNotFound(err) { | |
| gomega.Expect(err).ToNot(gomega.HaveOccurred()) | |
| } | |
| }) |
🤖 Prompt for AI Agents
In test/integration/api/placement_test.go around lines 24 to 30, the cleanup
currently expects no error whenever err != nil which contradicts the comment
about ignoring NotFound; change the logic to treat NotFound as acceptable and
only fail for other errors (i.e., if err != nil and !apierrors.IsNotFound(err)
then Expect(err).ToNot(HaveOccurred())); also add the missing import for k8s API
errors (k8s.io/apimachinery/pkg/api/errors) and reference it as apierrors in the
check.
All 115 integration tests now pass successfully.
🤖 Generated with Claude Code
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit