From bbee2ce8a8c046a66525e28f9eefc293920ffecf Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Tue, 20 Feb 2024 22:08:28 +0100 Subject: [PATCH 1/6] Enable nice reporting of test failures caused by the not matching predicates. --- pkg/test/assertions/assertions.go | 83 +++++++++++++- pkg/test/assertions/assertions_test.go | 52 +++++++++ pkg/test/assertions/predicates.go | 113 ++++++++++++++++++- pkg/test/assertions/predicates_test.go | 149 +++++++++++++++++++++++++ 4 files changed, 387 insertions(+), 10 deletions(-) create mode 100644 pkg/test/assertions/assertions_test.go create mode 100644 pkg/test/assertions/predicates_test.go diff --git a/pkg/test/assertions/assertions.go b/pkg/test/assertions/assertions.go index 3cefa85d..4c0b5d8d 100644 --- a/pkg/test/assertions/assertions.go +++ b/pkg/test/assertions/assertions.go @@ -1,8 +1,12 @@ package assertions import ( + "fmt" + "reflect" + "strings" "testing" + "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/assert" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -20,10 +24,79 @@ import ( // // AssertThat(t, object, Is(Named("asdf"))) // -// Note that it intentionally doesn't accept a slice of predicates so that it is easy -// to spot the failed predicate in the actual tests and the caller doesn't have to guess -// which of the many supplied predicates might have failed. -func AssertThat(t *testing.T, object client.Object, predicate Predicate[client.Object], msgAndArgs ...any) { +// Note that this method accepts multiple predicates and reports any failures in them using +// the Explain function. +func AssertThat(t *testing.T, object client.Object, predicates ...Predicate[client.Object]) { t.Helper() - assert.True(t, predicate.Matches(object), msgAndArgs...) + results := make([]bool, len(predicates)) + failure := false + for i, p := range predicates { + res := p.Matches(object) + failure = failure || !res + results[i] = res + } + if failure { + // compose the message + sb := strings.Builder{} + sb.WriteString("failed predicates report:") + for i, p := range predicates { + if !results[i] { + sb.WriteRune('\n') + sb.WriteString(Explain(p, object)) + } + } + assert.Fail(t, "some predicates failed to match", sb.String()) + } +} + +// Explain produces a textual explanation for why the provided predicate didn't match. The explanation +// contains the type name of the predicate, the type of the object and, if the predicate implements +// PredicateMatchFixer interface, a diff between what the object looks like and should have looked like +// to match the predicate. This is best used for logging the explanation of test failures in the end to +// end tests. +// +// The lines beginning with "-" are what was expected, "+" marks the actual values. +// +// Note that this function doesn't actually check if the predicate matches the object so it can produce +// slightly misleading output if called with a predicate that matches given object. +func Explain[T client.Object](predicate Predicate[client.Object], actual T) string { + // this is used for reporting the type of the predicate + var reportedPredicateType reflect.Type + + // we want the Is() and Has() to be "transparent" and actually report the type of the + // inner predicate. Because "cast" (the type that underlies Is() and Has()) is generic, + // we need to employ a little bit of reflection trickery to get at its inner predicate. + // + // If it weren't generic, we could simply use a checked cast. But in case of generic + // types, the checked cast requires us to specify the generic type. But we don't know + // that here, hence the pain. + predVal := reflect.ValueOf(predicate) + if predVal.Kind() == reflect.Pointer { + predVal = predVal.Elem() + } + typName := predVal.Type().Name() + if strings.HasPrefix(typName, "cast[") { + // Interestingly, predVal.FieldByName("Inner").Type() returns the type of the field + // not the type of the value. So we need to get the actual value using .Interface() + // and get the type of that. Also notice, that in order to be able to call .Interface() + // on a field, it needs to be public. In code, we could access cast.inner because + // we're in the same package, but not with reflection. Go go... + reportedPredicateType = reflect.TypeOf(predVal.FieldByName("Inner").Interface()) + } else { + reportedPredicateType = reflect.TypeOf(predicate) + } + if reportedPredicateType.Kind() == reflect.Pointer { + reportedPredicateType = reportedPredicateType.Elem() + } + + prefix := fmt.Sprintf("predicate '%s' didn't match the object", reportedPredicateType.String()) + fix, ok := predicate.(PredicateMatchFixer[client.Object]) + if !ok { + return prefix + } + + expected := fix.FixToMatch(actual.DeepCopyObject().(client.Object)) + diff := cmp.Diff(expected, actual) + + return fmt.Sprintf("%s because of the following differences:\n%s", prefix, diff) } diff --git a/pkg/test/assertions/assertions_test.go b/pkg/test/assertions/assertions_test.go new file mode 100644 index 00000000..b4c3357d --- /dev/null +++ b/pkg/test/assertions/assertions_test.go @@ -0,0 +1,52 @@ +package assertions + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +func TestExplain(t *testing.T) { + t.Run("with diff", func(t *testing.T) { + // given + actual := &corev1.Secret{} + actual.SetName("actual") + + pred := Has(Name("expected")) + + // when + expl := Explain(pred, actual) + + // then + assert.True(t, strings.HasPrefix(expl, "predicate 'assertions.named' didn't match the object because of the following differences:")) + assert.Contains(t, expl, "-") + assert.Contains(t, expl, "\"expected\"") + assert.Contains(t, expl, "+") + assert.Contains(t, expl, "\"actual\"") + }) + + t.Run("without diff", func(t *testing.T) { + // given + actual := &corev1.Secret{} + actual.SetName("actual") + + pred := &predicateWithoutFixing{} + + // when + expl := Explain(pred, actual) + + // then + assert.Equal(t, expl, "predicate 'assertions.predicateWithoutFixing' didn't match the object") + }) +} + +type predicateWithoutFixing struct{} + +var _ Predicate[client.Object] = (*predicateWithoutFixing)(nil) + +func (*predicateWithoutFixing) Matches(obj client.Object) bool { + return false +} diff --git a/pkg/test/assertions/predicates.go b/pkg/test/assertions/predicates.go index 757e33e1..32a74c74 100644 --- a/pkg/test/assertions/predicates.go +++ b/pkg/test/assertions/predicates.go @@ -33,41 +33,79 @@ import ( // but waits for an object that satisfies the predicates to appear in the cluster). // // assertions.AssertThat(t, toolchainCluster, assertions.Is(Ready())) +// +// If you're implementing your own predicate, consider implementing the PredicateMatchFixer, +// too, so that you can benefit from improved failure diagnostics offered by Explain function. type Predicate[T client.Object] interface { Matches(obj T) bool } +// PredicateMatchFixer is an optional interface that the predicate implementations can also +// implement. If so, the FixToMatch method is used to obtain an object that WOULD +// match the predicate. This would-be-matching object is then used to produce a diff +// between it and the non-matching object of the predicate in case of a test failure +// for logging purposes. +// +// There is no need to copy the provided object. +type PredicateMatchFixer[T client.Object] interface { + FixToMatch(obj T) T +} + // Is merely casts the generic predicate on type T to a predicate on client.Object. This is // always valid because T is required to implement client.Object. Using this function helps // readability of the code by being able to construct expressions like: // // predicates.Is(predicates.Named("whatevs")) func Is[T client.Object](p Predicate[T]) Predicate[client.Object] { - return cast[T]{inner: p} + return &cast[T]{Inner: p} } // Has is just an alias of Is. It is provided for better readability with certain predicate // names. func Has[T client.Object](p Predicate[T]) Predicate[client.Object] { - return cast[T]{inner: p} + return &cast[T]{Inner: p} } type cast[T client.Object] struct { - inner Predicate[T] + // Inner is public so that Explain (in assertions.go) can access it... + Inner Predicate[T] } -func (c cast[T]) Matches(obj client.Object) bool { - return c.inner.Matches(obj.(T)) +var ( + _ Predicate[client.Object] = (*cast[client.Object])(nil) + _ PredicateMatchFixer[client.Object] = (*cast[client.Object])(nil) +) + +func (c *cast[T]) Matches(obj client.Object) bool { + return c.Inner.Matches(obj.(T)) +} + +func (c *cast[T]) FixToMatch(obj client.Object) client.Object { + pf, ok := c.Inner.(PredicateMatchFixer[T]) + if ok { + return pf.FixToMatch(obj.(T)) + } + return obj } type named struct { name string } +var ( + _ Predicate[client.Object] = (*named)(nil) + _ PredicateMatchFixer[client.Object] = (*named)(nil) +) + func (n *named) Matches(obj client.Object) bool { return obj.GetName() == n.name } +func (n *named) FixToMatch(obj client.Object) client.Object { + obj.SetName(n.name) + return obj +} + // Name returns a predicate checking that an Object has given name. func Name(name string) Predicate[client.Object] { return &named{name: name} @@ -77,10 +115,20 @@ type inNamespace struct { namespace string } +var ( + _ Predicate[client.Object] = (*inNamespace)(nil) + _ PredicateMatchFixer[client.Object] = (*inNamespace)(nil) +) + func (i *inNamespace) Matches(obj client.Object) bool { return obj.GetNamespace() == i.namespace } +func (i *inNamespace) FixToMatch(obj client.Object) client.Object { + obj.SetNamespace(i.namespace) + return obj +} + // InNamespace returns a predicate checking that an Object is in the given namespace. func InNamespace(name string) Predicate[client.Object] { return &inNamespace{namespace: name} @@ -90,10 +138,21 @@ type withKey struct { types.NamespacedName } +var ( + _ Predicate[client.Object] = (*withKey)(nil) + _ PredicateMatchFixer[client.Object] = (*withKey)(nil) +) + func (w *withKey) Matches(obj client.Object) bool { return obj.GetName() == w.Name && obj.GetNamespace() == w.Namespace } +func (w *withKey) FixToMatch(obj client.Object) client.Object { + obj.SetName(w.Name) + obj.SetNamespace(w.Namespace) + return obj +} + // ObjectKey returns a predicate checking that an Object has given NamespacedName (aka client.ObjectKey). func ObjectKey(key types.NamespacedName) Predicate[client.Object] { return &withKey{NamespacedName: key} @@ -103,6 +162,11 @@ type hasLabels struct { requiredLabels map[string]string } +var ( + _ Predicate[client.Object] = (*hasLabels)(nil) + _ PredicateMatchFixer[client.Object] = (*hasLabels)(nil) +) + func (h *hasLabels) Matches(obj client.Object) bool { objLabels := obj.GetLabels() for k, v := range h.requiredLabels { @@ -114,6 +178,23 @@ func (h *hasLabels) Matches(obj client.Object) bool { return true } +func (h *hasLabels) FixToMatch(obj client.Object) client.Object { + if len(h.requiredLabels) == 0 { + return obj + } + objLabels := obj.GetLabels() + if objLabels == nil { + objLabels = map[string]string{} + } + + for k, v := range h.requiredLabels { + objLabels[k] = v + } + + obj.SetLabels(objLabels) + return obj +} + // Labels returns a predicate checking that an Object has provided labels and their values. func Labels(requiredLabels map[string]string) Predicate[client.Object] { return &hasLabels{requiredLabels: requiredLabels} @@ -123,6 +204,11 @@ type hasAnnotations struct { requiredAnnotations map[string]string } +var ( + _ Predicate[client.Object] = (*hasAnnotations)(nil) + _ PredicateMatchFixer[client.Object] = (*hasAnnotations)(nil) +) + func (h *hasAnnotations) Matches(obj client.Object) bool { objAnnos := obj.GetAnnotations() for k, v := range h.requiredAnnotations { @@ -134,6 +220,23 @@ func (h *hasAnnotations) Matches(obj client.Object) bool { return true } +func (h *hasAnnotations) FixToMatch(obj client.Object) client.Object { + if len(h.requiredAnnotations) == 0 { + return obj + } + objAnnos := obj.GetAnnotations() + if objAnnos == nil { + objAnnos = map[string]string{} + } + + for k, v := range h.requiredAnnotations { + objAnnos[k] = v + } + + obj.SetAnnotations(objAnnos) + return obj +} + // Annotations returns a predicate checking that an Object has provided annotations and their values. func Annotations(requiredAnnotations map[string]string) Predicate[client.Object] { return &hasAnnotations{requiredAnnotations: requiredAnnotations} diff --git a/pkg/test/assertions/predicates_test.go b/pkg/test/assertions/predicates_test.go new file mode 100644 index 00000000..4c3b303d --- /dev/null +++ b/pkg/test/assertions/predicates_test.go @@ -0,0 +1,149 @@ +package assertions + +import ( + "testing" + + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +func TestNamePredicate(t *testing.T) { + pred := Name("expected") + + t.Run("positive", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetName("expected") + + // when & then + assert.True(t, pred.Matches(obj)) + }) + + t.Run("negative", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetName("different") + + // when & then + assert.False(t, pred.Matches(obj)) + }) +} + +func TestInNamespacePredicate(t *testing.T) { + pred := InNamespace("expected") + + t.Run("positive", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetNamespace("expected") + + // when & then + assert.True(t, pred.Matches(obj)) + }) + + t.Run("negative", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetNamespace("different") + + // when & then + assert.False(t, pred.Matches(obj)) + }) +} + +func TestWithKeyPredicate(t *testing.T) { + pred := ObjectKey(client.ObjectKey{Name: "expected", Namespace: "expected"}) + + t.Run("positive", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetName("expected") + obj.SetNamespace("expected") + + // when & then + assert.True(t, pred.Matches(obj)) + }) + + t.Run("different name", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetName("different") + obj.SetNamespace("expected") + + // when & then + assert.False(t, pred.Matches(obj)) + }) + + t.Run("different namespace", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetName("expected") + obj.SetNamespace("different") + + // when & then + assert.False(t, pred.Matches(obj)) + }) +} + +func TestLabelsPredicate(t *testing.T) { + pred := Labels(map[string]string{"ka": "va", "kb": "vb"}) + + t.Run("exact match", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetLabels(map[string]string{"ka": "va", "kb": "vb"}) + + // when & then + assert.True(t, pred.Matches(obj)) + }) + + t.Run("subset match", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetLabels(map[string]string{"ka": "va", "kb": "vb", "kc": "vc"}) + + // when & then + assert.True(t, pred.Matches(obj)) + }) + + t.Run("nil", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetLabels(nil) + + // when & then + assert.False(t, pred.Matches(obj)) + }) +} + +func TestAnnotationsPredicate(t *testing.T) { + pred := Annotations(map[string]string{"ka": "va", "kb": "vb"}) + + t.Run("exact match", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetAnnotations(map[string]string{"ka": "va", "kb": "vb"}) + + // when & then + assert.True(t, pred.Matches(obj)) + }) + + t.Run("subset match", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetAnnotations(map[string]string{"ka": "va", "kb": "vb", "kc": "vc"}) + + // when & then + assert.True(t, pred.Matches(obj)) + }) + + t.Run("nil", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetAnnotations(nil) + + // when & then + assert.False(t, pred.Matches(obj)) + }) +} From 9f5567b375a21c0e4a268073c7d852799db3dd78 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Wed, 21 Feb 2024 14:25:29 +0100 Subject: [PATCH 2/6] Rewrite the SpaceProvisionerConfig predicates to support diff reporting. --- .../spaceprovisionerconfig_assertions.go | 81 +++++++-- .../spaceprovisionerconfig_assertions_test.go | 166 ++++++++++++++++++ 2 files changed, 234 insertions(+), 13 deletions(-) create mode 100644 pkg/test/spaceprovisionerconfig/spaceprovisionerconfig_assertions_test.go diff --git a/pkg/test/spaceprovisionerconfig/spaceprovisionerconfig_assertions.go b/pkg/test/spaceprovisionerconfig/spaceprovisionerconfig_assertions.go index 13444a49..14197dd6 100644 --- a/pkg/test/spaceprovisionerconfig/spaceprovisionerconfig_assertions.go +++ b/pkg/test/spaceprovisionerconfig/spaceprovisionerconfig_assertions.go @@ -4,30 +4,85 @@ import ( toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/toolchain-common/pkg/condition" "github.com/codeready-toolchain/toolchain-common/pkg/test/assertions" + corev1 "k8s.io/api/core/v1" ) -type predicate func(*toolchainv1alpha1.SpaceProvisionerConfig) bool +type ( + ready struct{} + notReady struct{} + notReadyWithReason struct { + expectedReason string + } +) -var _ assertions.Predicate[*toolchainv1alpha1.SpaceProvisionerConfig] = predicate(nil) +var ( + _ assertions.Predicate[*toolchainv1alpha1.SpaceProvisionerConfig] = (*ready)(nil) + _ assertions.Predicate[*toolchainv1alpha1.SpaceProvisionerConfig] = (*notReady)(nil) + _ assertions.Predicate[*toolchainv1alpha1.SpaceProvisionerConfig] = (*notReadyWithReason)(nil) + _ assertions.PredicateMatchFixer[*toolchainv1alpha1.SpaceProvisionerConfig] = (*ready)(nil) + _ assertions.PredicateMatchFixer[*toolchainv1alpha1.SpaceProvisionerConfig] = (*notReady)(nil) + _ assertions.PredicateMatchFixer[*toolchainv1alpha1.SpaceProvisionerConfig] = (*notReadyWithReason)(nil) +) -func (p predicate) Matches(obj *toolchainv1alpha1.SpaceProvisionerConfig) bool { - return p(obj) +func (*ready) Matches(spc *toolchainv1alpha1.SpaceProvisionerConfig) bool { + return condition.IsTrueWithReason(spc.Status.Conditions, toolchainv1alpha1.ConditionReady, toolchainv1alpha1.SpaceProvisionerConfigValidReason) } -func Ready() assertions.Predicate[*toolchainv1alpha1.SpaceProvisionerConfig] { - return predicate(func(spc *toolchainv1alpha1.SpaceProvisionerConfig) bool { - return condition.IsTrueWithReason(spc.Status.Conditions, toolchainv1alpha1.ConditionReady, toolchainv1alpha1.SpaceProvisionerConfigValidReason) +func (*ready) FixToMatch(spc *toolchainv1alpha1.SpaceProvisionerConfig) *toolchainv1alpha1.SpaceProvisionerConfig { + spc.Status.Conditions, _ = condition.AddOrUpdateStatusConditions(spc.Status.Conditions, toolchainv1alpha1.Condition{ + Type: toolchainv1alpha1.ConditionReady, + Status: corev1.ConditionTrue, + Reason: toolchainv1alpha1.SpaceProvisionerConfigValidReason, }) + return spc +} + +func Ready() assertions.Predicate[*toolchainv1alpha1.SpaceProvisionerConfig] { + return &ready{} +} + +func (*notReady) Matches(spc *toolchainv1alpha1.SpaceProvisionerConfig) bool { + return condition.IsFalse(spc.Status.Conditions, toolchainv1alpha1.ConditionReady) +} + +func (*notReady) FixToMatch(spc *toolchainv1alpha1.SpaceProvisionerConfig) *toolchainv1alpha1.SpaceProvisionerConfig { + cnd, found := condition.FindConditionByType(spc.Status.Conditions, toolchainv1alpha1.ConditionReady) + if !found { + spc.Status.Conditions = condition.AddStatusConditions(spc.Status.Conditions, toolchainv1alpha1.Condition{ + Type: toolchainv1alpha1.ConditionReady, + Status: corev1.ConditionFalse, + }) + } else { + cnd.Status = corev1.ConditionFalse + spc.Status.Conditions, _ = condition.AddOrUpdateStatusConditions(spc.Status.Conditions, cnd) + } + return spc } func NotReady() assertions.Predicate[*toolchainv1alpha1.SpaceProvisionerConfig] { - return predicate(func(spc *toolchainv1alpha1.SpaceProvisionerConfig) bool { - return condition.IsFalse(spc.Status.Conditions, toolchainv1alpha1.ConditionReady) - }) + return ¬Ready{} +} + +func (p *notReadyWithReason) Matches(spc *toolchainv1alpha1.SpaceProvisionerConfig) bool { + return condition.IsFalseWithReason(spc.Status.Conditions, toolchainv1alpha1.ConditionReady, p.expectedReason) +} + +func (p *notReadyWithReason) FixToMatch(spc *toolchainv1alpha1.SpaceProvisionerConfig) *toolchainv1alpha1.SpaceProvisionerConfig { + cnd, found := condition.FindConditionByType(spc.Status.Conditions, toolchainv1alpha1.ConditionReady) + if !found { + spc.Status.Conditions = condition.AddStatusConditions(spc.Status.Conditions, toolchainv1alpha1.Condition{ + Type: toolchainv1alpha1.ConditionReady, + Status: corev1.ConditionFalse, + Reason: p.expectedReason, + }) + } else { + cnd.Status = corev1.ConditionFalse + cnd.Reason = p.expectedReason + spc.Status.Conditions, _ = condition.AddOrUpdateStatusConditions(spc.Status.Conditions, cnd) + } + return spc } func NotReadyWithReason(reason string) assertions.Predicate[*toolchainv1alpha1.SpaceProvisionerConfig] { - return predicate(func(spc *toolchainv1alpha1.SpaceProvisionerConfig) bool { - return condition.IsFalseWithReason(spc.Status.Conditions, toolchainv1alpha1.ConditionReady, reason) - }) + return ¬ReadyWithReason{expectedReason: reason} } diff --git a/pkg/test/spaceprovisionerconfig/spaceprovisionerconfig_assertions_test.go b/pkg/test/spaceprovisionerconfig/spaceprovisionerconfig_assertions_test.go new file mode 100644 index 00000000..6be1cd94 --- /dev/null +++ b/pkg/test/spaceprovisionerconfig/spaceprovisionerconfig_assertions_test.go @@ -0,0 +1,166 @@ +package spaceprovisionerconfig + +import ( + "testing" + + toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" + "github.com/codeready-toolchain/toolchain-common/pkg/condition" + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" +) + +func TestReadyPredicate(t *testing.T) { + t.Run("matching", func(t *testing.T) { + // given + pred := &ready{} + spc := NewSpaceProvisionerConfig("spc", "default", WithReadyConditionValid()) + + // when & then + assert.True(t, pred.Matches(spc)) + }) + + t.Run("fixer with no conditions", func(t *testing.T) { + // given + pred := &ready{} + spc := NewSpaceProvisionerConfig("spc", "default") + + // when + spc = pred.FixToMatch(spc) + + // then + assert.True(t, condition.IsTrue(spc.Status.Conditions, toolchainv1alpha1.ConditionReady)) + }) + t.Run("fixer with different conditions", func(t *testing.T) { + // given + pred := &ready{} + spc := NewSpaceProvisionerConfig("spc", "default") + spc.Status.Conditions = []toolchainv1alpha1.Condition{ + { + Type: toolchainv1alpha1.ConditionType("made up"), + Status: corev1.ConditionTrue, + }, + } + + // when + spc = pred.FixToMatch(spc) + + // then + assert.True(t, condition.IsTrue(spc.Status.Conditions, toolchainv1alpha1.ConditionReady)) + assert.Len(t, spc.Status.Conditions, 2) + }) + t.Run("fixer with wrong condition", func(t *testing.T) { + // given + pred := &ready{} + spc := NewSpaceProvisionerConfig("spc", "default", WithReadyConditionInvalid("because")) + + // when + spc = pred.FixToMatch(spc) + + // then + assert.True(t, condition.IsTrueWithReason(spc.Status.Conditions, toolchainv1alpha1.ConditionReady, toolchainv1alpha1.SpaceProvisionerConfigValidReason)) + }) +} + +func TestNotReadyPredicate(t *testing.T) { + t.Run("matching", func(t *testing.T) { + // given + pred := ¬Ready{} + spc := NewSpaceProvisionerConfig("spc", "default", WithReadyConditionInvalid("any reason")) + + // when & then + assert.True(t, pred.Matches(spc)) + }) + + t.Run("fixer with no conditions", func(t *testing.T) { + // given + pred := ¬Ready{} + spc := NewSpaceProvisionerConfig("spc", "default") + + // when + spc = pred.FixToMatch(spc) + + // then + assert.True(t, condition.IsFalse(spc.Status.Conditions, toolchainv1alpha1.ConditionReady)) + }) + t.Run("fixer with different conditions", func(t *testing.T) { + // given + pred := ¬Ready{} + spc := NewSpaceProvisionerConfig("spc", "default") + spc.Status.Conditions = []toolchainv1alpha1.Condition{ + { + Type: toolchainv1alpha1.ConditionType("made up"), + Status: corev1.ConditionTrue, + }, + } + + // when + spc = pred.FixToMatch(spc) + + // then + assert.True(t, condition.IsFalse(spc.Status.Conditions, toolchainv1alpha1.ConditionReady)) + assert.Len(t, spc.Status.Conditions, 2) + }) + t.Run("fixer with wrong condition", func(t *testing.T) { + // given + pred := ¬Ready{} + spc := NewSpaceProvisionerConfig("spc", "default", WithReadyConditionValid()) + + // when + spc = pred.FixToMatch(spc) + + // then + assert.True(t, condition.IsFalse(spc.Status.Conditions, toolchainv1alpha1.ConditionReady)) + }) +} + +func TestNotReadyWithReasonPredicate(t *testing.T) { + t.Run("matching", func(t *testing.T) { + // given + pred := ¬ReadyWithReason{expectedReason: "the right reason"} + spc := NewSpaceProvisionerConfig("spc", "default", WithReadyConditionInvalid("any reason")) + + // when & then + assert.True(t, pred.Matches(spc)) + }) + + t.Run("fixer with no conditions", func(t *testing.T) { + // given + pred := ¬ReadyWithReason{expectedReason: "the right reason"} + spc := NewSpaceProvisionerConfig("spc", "default") + + // when + spc = pred.FixToMatch(spc) + + // then + assert.True(t, condition.IsFalseWithReason(spc.Status.Conditions, toolchainv1alpha1.ConditionReady, "the right reason")) + }) + t.Run("fixer with different conditions", func(t *testing.T) { + // given + pred := ¬ReadyWithReason{expectedReason: "the right reason"} + spc := NewSpaceProvisionerConfig("spc", "default") + spc.Status.Conditions = []toolchainv1alpha1.Condition{ + { + Type: toolchainv1alpha1.ConditionType("made up"), + Status: corev1.ConditionTrue, + }, + } + + // when + spc = pred.FixToMatch(spc) + + // then + assert.True(t, condition.IsFalseWithReason(spc.Status.Conditions, toolchainv1alpha1.ConditionReady, "the right reason")) + assert.Len(t, spc.Status.Conditions, 2) + }) + t.Run("fixer with wrong condition", func(t *testing.T) { + // given + pred := ¬ReadyWithReason{expectedReason: "the right reason"} + spc := NewSpaceProvisionerConfig("spc", "default", WithReadyConditionInvalid("the wrong reason")) + + // when + spc = pred.FixToMatch(spc) + + // then + assert.True(t, condition.IsFalseWithReason(spc.Status.Conditions, toolchainv1alpha1.ConditionReady, "the right reason")) + }) +} From f9ca8d4872ffa2726371dab1b48a913a895bd08c Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Wed, 21 Feb 2024 15:40:41 +0100 Subject: [PATCH 3/6] fix failing test and add more for the basic predicate fixers. --- pkg/test/assertions/predicates_test.go | 179 +++++++++++++++++- .../spaceprovisionerconfig_assertions_test.go | 2 +- 2 files changed, 175 insertions(+), 6 deletions(-) diff --git a/pkg/test/assertions/predicates_test.go b/pkg/test/assertions/predicates_test.go index 4c3b303d..7d4c3958 100644 --- a/pkg/test/assertions/predicates_test.go +++ b/pkg/test/assertions/predicates_test.go @@ -9,7 +9,7 @@ import ( ) func TestNamePredicate(t *testing.T) { - pred := Name("expected") + pred := &named{name: "expected"} t.Run("positive", func(t *testing.T) { // given @@ -28,10 +28,22 @@ func TestNamePredicate(t *testing.T) { // when & then assert.False(t, pred.Matches(obj)) }) + + t.Run("fix", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetName("different") + + // when + obj = pred.FixToMatch(obj).(*corev1.ConfigMap) + + // then + assert.Equal(t, "expected", obj.Name) + }) } func TestInNamespacePredicate(t *testing.T) { - pred := InNamespace("expected") + pred := &inNamespace{namespace: "expected"} t.Run("positive", func(t *testing.T) { // given @@ -50,10 +62,22 @@ func TestInNamespacePredicate(t *testing.T) { // when & then assert.False(t, pred.Matches(obj)) }) + + t.Run("fix", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetNamespace("different") + + // when + obj = pred.FixToMatch(obj).(*corev1.ConfigMap) + + // then + assert.Equal(t, "expected", obj.Namespace) + }) } func TestWithKeyPredicate(t *testing.T) { - pred := ObjectKey(client.ObjectKey{Name: "expected", Namespace: "expected"}) + pred := &withKey{NamespacedName: client.ObjectKey{Name: "expected", Namespace: "expected"}} t.Run("positive", func(t *testing.T) { // given @@ -84,10 +108,53 @@ func TestWithKeyPredicate(t *testing.T) { // when & then assert.False(t, pred.Matches(obj)) }) + + t.Run("fix name", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetName("different") + obj.SetNamespace("expected") + + // when + obj = pred.FixToMatch(obj).(*corev1.ConfigMap) + + // then + assert.Equal(t, "expected", obj.Name) + assert.Equal(t, "expected", obj.Namespace) + }) + + t.Run("fix namespace", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetName("expected") + obj.SetNamespace("difference") + + // when + obj = pred.FixToMatch(obj).(*corev1.ConfigMap) + + // then + assert.Equal(t, "expected", obj.Name) + assert.Equal(t, "expected", obj.Namespace) + }) + + t.Run("fix both", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetName("different") + obj.SetNamespace("difference") + + // when + obj = pred.FixToMatch(obj).(*corev1.ConfigMap) + + // then + assert.Equal(t, "expected", obj.Name) + assert.Equal(t, "expected", obj.Namespace) + }) } func TestLabelsPredicate(t *testing.T) { - pred := Labels(map[string]string{"ka": "va", "kb": "vb"}) + expectedLabels := map[string]string{"ka": "va", "kb": "vb"} + pred := &hasLabels{requiredLabels: expectedLabels} t.Run("exact match", func(t *testing.T) { // given @@ -115,10 +182,62 @@ func TestLabelsPredicate(t *testing.T) { // when & then assert.False(t, pred.Matches(obj)) }) + + t.Run("fix nil labels", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetLabels(nil) + + // when + obj = pred.FixToMatch(obj).(*corev1.ConfigMap) + + // then + assert.Equal(t, expectedLabels, obj.GetLabels()) + }) + t.Run("fix empty labels", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetLabels(map[string]string{}) + + // when + obj = pred.FixToMatch(obj).(*corev1.ConfigMap) + + // then + assert.Equal(t, expectedLabels, obj.GetLabels()) + }) + t.Run("fix different labels", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetLabels(map[string]string{"kd": "vd"}) + + // when + obj = pred.FixToMatch(obj).(*corev1.ConfigMap) + + // then + assert.Len(t, obj.GetLabels(), 3) + assert.Equal(t, "va", obj.GetLabels()["ka"]) + assert.Equal(t, "vb", obj.GetLabels()["kb"]) + assert.Equal(t, "vd", obj.GetLabels()["kd"]) + }) + t.Run("fix partially matching labels", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetLabels(map[string]string{"ka": "va", "kb": "different", "kd": "vd"}) + + // when + obj = pred.FixToMatch(obj).(*corev1.ConfigMap) + + // then + assert.Len(t, obj.GetLabels(), 3) + assert.Equal(t, "va", obj.GetLabels()["ka"]) + assert.Equal(t, "vb", obj.GetLabels()["kb"]) + assert.Equal(t, "vd", obj.GetLabels()["kd"]) + }) } func TestAnnotationsPredicate(t *testing.T) { - pred := Annotations(map[string]string{"ka": "va", "kb": "vb"}) + expectedAnnotations := map[string]string{"ka": "va", "kb": "vb"} + pred := &hasAnnotations{expectedAnnotations} t.Run("exact match", func(t *testing.T) { // given @@ -146,4 +265,54 @@ func TestAnnotationsPredicate(t *testing.T) { // when & then assert.False(t, pred.Matches(obj)) }) + t.Run("fix nil annotations", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetAnnotations(nil) + + // when + obj = pred.FixToMatch(obj).(*corev1.ConfigMap) + + // then + assert.Equal(t, expectedAnnotations, obj.GetAnnotations()) + }) + t.Run("fix empty annotations", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetAnnotations(map[string]string{}) + + // when + obj = pred.FixToMatch(obj).(*corev1.ConfigMap) + + // then + assert.Equal(t, expectedAnnotations, obj.GetAnnotations()) + }) + t.Run("fix different annotations", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetAnnotations(map[string]string{"kd": "vd"}) + + // when + obj = pred.FixToMatch(obj).(*corev1.ConfigMap) + + // then + assert.Len(t, obj.GetAnnotations(), 3) + assert.Equal(t, "va", obj.GetAnnotations()["ka"]) + assert.Equal(t, "vb", obj.GetAnnotations()["kb"]) + assert.Equal(t, "vd", obj.GetAnnotations()["kd"]) + }) + t.Run("fix partially matching annotations", func(t *testing.T) { + // given + obj := &corev1.ConfigMap{} + obj.SetAnnotations(map[string]string{"ka": "va", "kb": "different", "kd": "vd"}) + + // when + obj = pred.FixToMatch(obj).(*corev1.ConfigMap) + + // then + assert.Len(t, obj.GetAnnotations(), 3) + assert.Equal(t, "va", obj.GetAnnotations()["ka"]) + assert.Equal(t, "vb", obj.GetAnnotations()["kb"]) + assert.Equal(t, "vd", obj.GetAnnotations()["kd"]) + }) } diff --git a/pkg/test/spaceprovisionerconfig/spaceprovisionerconfig_assertions_test.go b/pkg/test/spaceprovisionerconfig/spaceprovisionerconfig_assertions_test.go index 6be1cd94..eb43e913 100644 --- a/pkg/test/spaceprovisionerconfig/spaceprovisionerconfig_assertions_test.go +++ b/pkg/test/spaceprovisionerconfig/spaceprovisionerconfig_assertions_test.go @@ -117,7 +117,7 @@ func TestNotReadyWithReasonPredicate(t *testing.T) { t.Run("matching", func(t *testing.T) { // given pred := ¬ReadyWithReason{expectedReason: "the right reason"} - spc := NewSpaceProvisionerConfig("spc", "default", WithReadyConditionInvalid("any reason")) + spc := NewSpaceProvisionerConfig("spc", "default", WithReadyConditionInvalid("the right reason")) // when & then assert.True(t, pred.Matches(spc)) From fa8b9f06040eb8e5df99aacdcbd4ef135bd3f93f Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Thu, 22 Feb 2024 14:00:09 +0100 Subject: [PATCH 4/6] Adding a short note about the role of - and + in the diffs --- pkg/test/assertions/assertions.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/test/assertions/assertions.go b/pkg/test/assertions/assertions.go index 4c0b5d8d..c6be34e5 100644 --- a/pkg/test/assertions/assertions.go +++ b/pkg/test/assertions/assertions.go @@ -98,5 +98,5 @@ func Explain[T client.Object](predicate Predicate[client.Object], actual T) stri expected := fix.FixToMatch(actual.DeepCopyObject().(client.Object)) diff := cmp.Diff(expected, actual) - return fmt.Sprintf("%s because of the following differences:\n%s", prefix, diff) + return fmt.Sprintf("%s because of the following differences (- indicates the expected values, + the actual values):\n%s", prefix, diff) } From 554737c3dce27d7cc34601e834b8779d2833a520 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Thu, 22 Feb 2024 14:34:03 +0100 Subject: [PATCH 5/6] fix the test --- pkg/test/assertions/assertions_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/test/assertions/assertions_test.go b/pkg/test/assertions/assertions_test.go index b4c3357d..f1dd2644 100644 --- a/pkg/test/assertions/assertions_test.go +++ b/pkg/test/assertions/assertions_test.go @@ -21,7 +21,7 @@ func TestExplain(t *testing.T) { expl := Explain(pred, actual) // then - assert.True(t, strings.HasPrefix(expl, "predicate 'assertions.named' didn't match the object because of the following differences:")) + assert.True(t, strings.HasPrefix(expl, "predicate 'assertions.named' didn't match the object because of the following differences (- indicates the expected values, + the actual values):")) assert.Contains(t, expl, "-") assert.Contains(t, expl, "\"expected\"") assert.Contains(t, expl, "+") From 2c2e36ecada6dd55d5961e48a3d3e8b43da439b7 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Thu, 22 Feb 2024 16:44:36 +0100 Subject: [PATCH 6/6] Make most of the AssertThat logic testable and write a simple test for it. --- pkg/test/assertions/assertions.go | 13 +++++++++++- pkg/test/assertions/assertions_test.go | 29 ++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/pkg/test/assertions/assertions.go b/pkg/test/assertions/assertions.go index c6be34e5..160c98e8 100644 --- a/pkg/test/assertions/assertions.go +++ b/pkg/test/assertions/assertions.go @@ -28,6 +28,16 @@ import ( // the Explain function. func AssertThat(t *testing.T, object client.Object, predicates ...Predicate[client.Object]) { t.Helper() + message := assertThat(object, predicates...) + if message != "" { + assert.Fail(t, "some predicates failed to match", message) + } +} + +// assertThat contains the actual logic of the AssertThat function. This is separated out into +// its own testable function because we cannot cannot capture the result of assert.Fail() in +// another test. +func assertThat(object client.Object, predicates ...Predicate[client.Object]) string { results := make([]bool, len(predicates)) failure := false for i, p := range predicates { @@ -45,8 +55,9 @@ func AssertThat(t *testing.T, object client.Object, predicates ...Predicate[clie sb.WriteString(Explain(p, object)) } } - assert.Fail(t, "some predicates failed to match", sb.String()) + return sb.String() } + return "" } // Explain produces a textual explanation for why the provided predicate didn't match. The explanation diff --git a/pkg/test/assertions/assertions_test.go b/pkg/test/assertions/assertions_test.go index f1dd2644..eba56926 100644 --- a/pkg/test/assertions/assertions_test.go +++ b/pkg/test/assertions/assertions_test.go @@ -43,6 +43,35 @@ func TestExplain(t *testing.T) { }) } +func TestAssertThat(t *testing.T) { + t.Run("positive case", func(t *testing.T) { + // given + actual := &corev1.ConfigMap{} + actual.SetName("actual") + actual.SetLabels(map[string]string{"k": "v"}) + + // when + message := assertThat(actual, Has(Name("actual")), Has(Labels(map[string]string{"k": "v"}))) + + // then + assert.Empty(t, message) + }) + + t.Run("negative case", func(t *testing.T) { + // given + actual := &corev1.ConfigMap{} + actual.SetName("actual") + actual.SetLabels(map[string]string{"k": "v"}) + + // when + message := assertThat(actual, Has(Name("expected")), Has(Labels(map[string]string{"k": "another value"}))) + + // then + assert.Contains(t, message, "predicate 'assertions.named' didn't match the object because of the following differences") + assert.Contains(t, message, "predicate 'assertions.hasLabels' didn't match the object because of the following differences") + }) +} + type predicateWithoutFixing struct{} var _ Predicate[client.Object] = (*predicateWithoutFixing)(nil)