From 9eea115df26194296abc7c2b17c5ecaaf63f3396 Mon Sep 17 00:00:00 2001 From: Eric Fried Date: Tue, 8 Jun 2021 11:22:49 -0500 Subject: [PATCH] Fix clusterdeployment tests for DeepEquals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TL;DR: When comparing objects coming back from kube, things like ResourceVersion should be ignored (unless asserting that the object has not changed on the server, in which case comparing *only* ResourceVersion should suffice). We had a couple of tests that were using DeepEquals to compare an object from the (fake) kube server to one we constructed locally. The ResourceVersion returned by the fake server [changed in controller-runtime 0.8.0](https://github.com/kubernetes-sigs/controller-runtime/pull/1306) which broke tests. We [worked around](https://github.com/openshift/hive/pull/1407/commits/c7d60527612c141f58d748b8bf2e25d72f9499d5) that; but the Right Thing To Do™ is to make the comparison ignore ResourceVersion entirely. It is thusly done via this commit. --- .../clusterdeployment_controller_test.go | 25 +++++++----------- pkg/test/assert/assertions.go | 26 +++++++++++++++++++ 2 files changed, 35 insertions(+), 16 deletions(-) diff --git a/pkg/controller/clusterdeployment/clusterdeployment_controller_test.go b/pkg/controller/clusterdeployment/clusterdeployment_controller_test.go index 14560286cf5..05532664223 100644 --- a/pkg/controller/clusterdeployment/clusterdeployment_controller_test.go +++ b/pkg/controller/clusterdeployment/clusterdeployment_controller_test.go @@ -15,14 +15,12 @@ import ( batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" - apiequality "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/diff" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/kubernetes/scheme" "k8s.io/utils/pointer" @@ -223,17 +221,14 @@ func TestClusterDeploymentReconcile(t *testing.T) { validate: func(c client.Client, t *testing.T) { cd := getCD(c) if assert.NotNil(t, cd, "no clusterdeployment found") { - if e, a := testClusterDeploymentWithDefaultConditions(testClusterDeploymentWithInitializedConditions( - testClusterDeploymentWithProvision())), cd; !assert.True(t, apiequality.Semantic.DeepEqual(e, a), - "unexpected change in clusterdeployment") { - t.Logf("diff = %s", diff.ObjectReflectDiff(e, a)) - } + e := testClusterDeploymentWithDefaultConditions(testClusterDeploymentWithInitializedConditions( + testClusterDeploymentWithProvision())) + testassert.AssertEqualWhereItCounts(t, e, cd, "unexpected change in clusterdeployment") } provisions := getProvisions(c) if assert.Len(t, provisions, 1, "expected provision to exist") { - if e, a := testProvision(), provisions[0]; !assert.True(t, apiequality.Semantic.DeepEqual(e, a), "unexpected change in provision") { - t.Logf("diff = %s", diff.ObjectReflectDiff(e, a)) - } + e := testProvision() + testassert.AssertEqualWhereItCounts(t, e, provisions[0], "unexpected change in provision") } }, }, @@ -2491,11 +2486,10 @@ func testEmptyClusterDeployment() *hivev1.ClusterDeployment { Kind: "ClusterDeployment", }, ObjectMeta: metav1.ObjectMeta{ - Name: testName, - Namespace: testNamespace, - Finalizers: []string{hivev1.FinalizerDeprovision}, - UID: types.UID("1234"), - ResourceVersion: "999", + Name: testName, + Namespace: testNamespace, + Finalizers: []string{hivev1.FinalizerDeprovision}, + UID: types.UID("1234"), }, } return cd @@ -2709,7 +2703,6 @@ func testProvision() *hivev1.ClusterProvision { Labels: map[string]string{ constants.ClusterDeploymentNameLabel: testName, }, - ResourceVersion: "999", }, Spec: hivev1.ClusterProvisionSpec{ ClusterDeploymentRef: corev1.LocalObjectReference{ diff --git a/pkg/test/assert/assertions.go b/pkg/test/assert/assertions.go index b956c95ae6d..4f6ff4868fc 100644 --- a/pkg/test/assert/assertions.go +++ b/pkg/test/assert/assertions.go @@ -5,9 +5,13 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" testifyassert "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" hivev1 "github.com/openshift/hive/apis/hive/v1" ) @@ -71,3 +75,25 @@ func AssertConditions(t *testing.T, cd *hivev1.ClusterDeployment, expectedCondit } } } + +// AssertEqualWhereItCounts compares two runtime.Objects, ignoring their ResourceVersion and TypeMeta, asserting that they +// are otherwise equal. +// This and cleanRVAndTypeMeta were borrowed/adapted from: +// https://github.com/openshift/ci-tools/blob/179a0edb6c003aa95ae1332692c5b4c79e58f674/pkg/testhelper/testhelper.go#L157-L175 +func AssertEqualWhereItCounts(t *testing.T, x, y runtime.Object, msg string) { + xCopy := x.DeepCopyObject() + yCopy := y.DeepCopyObject() + cleanRVAndTypeMeta(xCopy) + cleanRVAndTypeMeta(yCopy) + diff := cmp.Diff(xCopy, yCopy) + testifyassert.Empty(t, diff, msg) +} + +func cleanRVAndTypeMeta(r runtime.Object) { + if metaObject, ok := r.(metav1.Object); ok { + metaObject.SetResourceVersion("") + } + if typeObject, ok := r.(interface{ SetGroupVersionKind(schema.GroupVersionKind) }); ok { + typeObject.SetGroupVersionKind(schema.GroupVersionKind{}) + } +}