Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable diff reporting on predicates #362

94 changes: 89 additions & 5 deletions pkg/test/assertions/assertions.go
Original file line number Diff line number Diff line change
@@ -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"
)
Expand All @@ -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]) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you are changing a signature of an existing function let's be careful with breaking components which uses it. We need to get the other PRs ready or we can consider introducing a new function and keep the current one deprecated while updating all the dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have checked the code base in all repositories for this. Since this was introduced only very recently, it was used only on a couple of places, and none of them used the removed vararg parameter. So at this time, this is a "source compatible" change.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. Thanks for checking.

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 {
metlos marked this conversation as resolved.
Show resolved Hide resolved
// 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)
}
81 changes: 81 additions & 0 deletions pkg/test/assertions/assertions_test.go
Original file line number Diff line number Diff line change
@@ -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
}
113 changes: 108 additions & 5 deletions pkg/test/assertions/predicates.go
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand All @@ -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}
Expand All @@ -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}
Expand All @@ -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 {
Expand All @@ -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}
Expand All @@ -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 {
Expand All @@ -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}
Expand Down
Loading
Loading