Enable diff reporting on predicates (#362)

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,90 @@ 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...)
+	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 {
+		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))
+			}
+		}
+		return sb.String()
+	}
+	return ""
+}
+
+// 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 (- indicates the expected values, + the actual values):\n%s", prefix, diff)
 }

pkg/test/assertions/assertions_test.go

@@ -0,0 +1,81 @@
+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 (- indicates the expected values, + the actual values):"))
+		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")
+	})
+}
+
+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)
+
+func (*predicateWithoutFixing) Matches(obj client.Object) bool {
+	return false
+}

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}