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

wip generic predicates #435

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions pkg/test/assertions/assertions.go → pkg/test/assertions.go
Original file line number Diff line number Diff line change
@@ -1,18 +1,16 @@
package assertions
package test

import (
"fmt"
"reflect"
"strings"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/stretchr/testify/assert"
"sigs.k8s.io/controller-runtime/pkg/client"
)

// AssertThat is a helper function that tests that the provided object satisfies given predicate.
// It is a exactly implemented as:
// If it accepted only a single predicate, it would be exactly implemented as:
//
// assert.True(t, predicate.Matches(object))
//
Expand All @@ -26,7 +24,7 @@ import (
//
// 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]) {
func AssertThat(t T, object any, predicates ...Predicate[any]) {
t.Helper()
message := assertThat(object, predicates...)
if message != "" {
Expand All @@ -37,7 +35,7 @@ func AssertThat(t *testing.T, object client.Object, predicates ...Predicate[clie
// 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 {
func assertThat(object any, predicates ...Predicate[any]) string {
results := make([]bool, len(predicates))
failure := false
for i, p := range predicates {
Expand Down Expand Up @@ -70,7 +68,7 @@ func assertThat(object client.Object, predicates ...Predicate[client.Object]) st
//
// 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 {
func Explain[T any](predicate Predicate[any], actual T) string {
// this is used for reporting the type of the predicate
var reportedPredicateType reflect.Type

Expand All @@ -86,7 +84,7 @@ func Explain[T client.Object](predicate Predicate[client.Object], actual T) stri
predVal = predVal.Elem()
}
typName := predVal.Type().Name()
if strings.HasPrefix(typName, "cast[") {
if strings.HasPrefix(typName, "cast[") || strings.HasPrefix(typName, "fixingCast[") {
// 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()
Expand All @@ -101,12 +99,12 @@ func Explain[T client.Object](predicate Predicate[client.Object], actual T) stri
}

prefix := fmt.Sprintf("predicate '%s' didn't match the object", reportedPredicateType.String())
fix, ok := predicate.(PredicateMatchFixer[client.Object])
fix, ok := predicate.(PredicateMatchFixer[any])
if !ok {
return prefix
}

expected := fix.FixToMatch(actual.DeepCopyObject().(client.Object))
expected := fix.FixToMatch(actual)
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)
Expand Down
81 changes: 0 additions & 81 deletions pkg/test/assertions/assertions_test.go

This file was deleted.

126 changes: 126 additions & 0 deletions pkg/test/assertions_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
package test

import (
"strings"
"testing"

toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1"
"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 'test.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 := Is[client.Object](&predicateWithoutFixing{})

// when
expl := Explain(pred, actual)

// then
assert.Equal(t, "predicate 'test.predicateWithoutFixing' didn't match the object", expl)
})

t.Run("with a slice", func(t *testing.T) {
actual := []int{1, 2, 3}
pred := MockPredicate[[]int]{}
pred.MatchesFunc = func(v []int) bool {
return false
}
pred.FixToMatchFunc = func(v []int) []int {
return []int{1, 2}
}

expl := Explain(Is[[]int](pred), actual)

assert.True(t, strings.HasPrefix(expl, "predicate 'test.MockPredicate[[]int]' didn't match the object because of the following"))
})

t.Run("with conditions", func(t *testing.T) {
actual := []toolchainv1alpha1.Condition{
{
Type: toolchainv1alpha1.ConditionType("test"),
Status: corev1.ConditionFalse,
Reason: "because",
},
}

pred := ConditionThat(toolchainv1alpha1.ConditionType("test"), HasStatus(corev1.ConditionTrue))

Check failure on line 70 in pkg/test/assertions_test.go

View workflow job for this annotation

GitHub Actions / GolangCI Lint

undefined: ConditionThat (typecheck)

expl := Explain(Has(pred), actual)

assert.True(t, strings.HasPrefix(expl, "predicate 'test.conditionsPredicate' didn't match the object because of the following"))
})
}
Comment on lines +13 to +76
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid duplication of the test cases and to make the tests easier to read, we can use a table-driven approach, if you agree. Something like this:

Suggested change
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 'test.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 := Is[client.Object](&predicateWithoutFixing{})
// when
expl := Explain(pred, actual)
// then
assert.Equal(t, "predicate 'test.predicateWithoutFixing' didn't match the object", expl)
})
t.Run("with a slice", func(t *testing.T) {
actual := []int{1, 2, 3}
pred := MockPredicate[[]int]{}
pred.MatchesFunc = func(v []int) bool {
return false
}
pred.FixToMatchFunc = func(v []int) []int {
return []int{1, 2}
}
expl := Explain(Is[[]int](pred), actual)
assert.True(t, strings.HasPrefix(expl, "predicate 'test.MockPredicate[[]int]' didn't match the object because of the following"))
})
t.Run("with conditions", func(t *testing.T) {
actual := []toolchainv1alpha1.Condition{
{
Type: toolchainv1alpha1.ConditionType("test"),
Status: corev1.ConditionFalse,
Reason: "because",
},
}
pred := ConditionThat(toolchainv1alpha1.ConditionType("test"), HasStatus(corev1.ConditionTrue))
expl := Explain(Has(pred), actual)
assert.True(t, strings.HasPrefix(expl, "predicate 'test.conditionsPredicate' didn't match the object because of the following"))
})
}
func TestExplain(t *testing.T) {
tests := []struct {
name string
actual any
predicate Predicate[any]
expectedText string
contains []string
}{
{
name: "with diff",
actual: &corev1.Secret{Name: "actual"},
predicate: Has(Name("expected")),
expectedText: "predicate 'test.named' didn't match the object because of the following differences",
contains: []string{"-", "\"expected\"", "+", "\"actual\""},
},
{
name: "without diff",
actual: &corev1.Secret{Name: "actual"},
predicate: Is[client.Object](&predicateWithoutFixing{}),
expectedText: "predicate 'test.predicateWithoutFixing' didn't match the object",
contains: nil,
},
{
name: "with a slice",
actual: []int{1, 2, 3},
predicate: MockPredicate[[]int]{
MatchesFunc: func(v []int) bool { return false },
FixToMatchFunc: func(v []int) []int {
return []int{1, 2}
},
},
expectedText: "predicate 'test.MockPredicate[[]int]' didn't match the object because of the following",
contains: nil,
},
{
name: "with conditions",
actual: []toolchainv1alpha1.Condition{
{
Type: toolchainv1alpha1.ConditionType("test"),
Status: corev1.ConditionFalse,
Reason: "because",
},
},
predicate: Has(ConditionThat(
toolchainv1alpha1.ConditionType("test"),
HasStatus(corev1.ConditionTrue),
)),
expectedText: "predicate 'test.conditionsPredicate' didn't match the object because of the following",
contains: nil,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// when
expl := Explain(tt.predicate, tt.actual)
// then
assert.True(t, strings.HasPrefix(expl, tt.expectedText))
if tt.contains != nil {
for _, part := range tt.contains {
assert.Contains(t, expl, part)
}
}
})
}
}


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"})
Comment on lines +81 to +83
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's worth doing some aux function for creating the ConfigMap just to avoid duplication (there are two occurrences in these test, and another one in TestNamePredicate. maybe will need it more for the upcoming tests). WDYT?

Suggested change
actual := &corev1.ConfigMap{}
actual.SetName("actual")
actual.SetLabels(map[string]string{"k": "v"})
actual := newConfigMap("actual", 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 'test.named' didn't match the object because of the following differences")
assert.Contains(t, message, "predicate 'test.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
}

type MockPredicate[T any] struct {
MatchesFunc func(v T) bool
FixToMatchFunc func(v T) T
}

func (p MockPredicate[T]) Matches(v T) bool {
return p.MatchesFunc(v)
}

func (p MockPredicate[T]) FixToMatch(v T) T {
return p.FixToMatchFunc(v)
}
Loading
Loading