Merge pull request #2703 from stevekuznetsov/skuznets/eventually-cond…

…ition

🌱  test/e2e: DRY up waiting on conditions

test/e2e/apibinding/apibinding_deletion_test.go

@@ -178,14 +178,9 @@ func TestAPIBindingDeletion(t *testing.T) {
 	}, wait.ForeverTestTimeout, 100*time.Millisecond)
 
 	t.Logf("apibinding should have BindingResourceDeleteSuccess with false status")
-	require.Eventually(t, func() bool {
-		apibinding, err := kcpClusterClient.Cluster(consumerPath).ApisV1alpha1().APIBindings().Get(ctx, apiBinding.Name, metav1.GetOptions{})
-		if err != nil {
-			return false
-		}
-
-		return conditions.IsFalse(apibinding, apisv1alpha1.BindingResourceDeleteSuccess)
-	}, wait.ForeverTestTimeout, 100*time.Millisecond)
+	framework.EventuallyCondition(t, func() (conditions.Getter, error) {
+		return kcpClusterClient.Cluster(consumerPath).ApisV1alpha1().APIBindings().Get(ctx, apiBinding.Name, metav1.GetOptions{})
+	}, framework.IsNot(apisv1alpha1.BindingResourceDeleteSuccess))
 
 	t.Logf("ensure resource does not have create verb when deleting")
 	require.Eventually(t, func() bool {

test/e2e/apibinding/apibinding_permissionclaims_test.go

@@ -28,7 +28,6 @@ import (
 	"github.com/kcp-dev/logicalcluster/v3"
 	"github.com/stretchr/testify/require"
 
-	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/discovery/cached/memory"
@@ -68,23 +67,13 @@ func TestAPIBindingPermissionClaimsConditions(t *testing.T) {
 
 	apifixtures.CreateSheriffsSchemaAndExport(ctx, t, providerPath, kcpClusterClient, "wild.wild.west", "board the wanderer")
 
-	identityHash := ""
-	framework.Eventually(t, func() (done bool, str string) {
-		sheriffExport, err := kcpClusterClient.Cluster(providerPath).ApisV1alpha1().APIExports().Get(ctx, "wild.wild.west", metav1.GetOptions{})
-		if err != nil {
-			return false, err.Error()
-		}
+	framework.EventuallyCondition(t, func() (conditions.Getter, error) {
+		return kcpClusterClient.Cluster(providerPath).ApisV1alpha1().APIExports().Get(ctx, "wild.wild.west", metav1.GetOptions{})
+	}, framework.Is(apisv1alpha1.APIExportIdentityValid), "could not wait for APIExport to be valid with identity hash")
 
-		if conditions.IsTrue(sheriffExport, apisv1alpha1.APIExportIdentityValid) {
-			identityHash = sheriffExport.Status.IdentityHash
-			return true, ""
-		}
-		condition := conditions.Get(sheriffExport, apisv1alpha1.APIExportIdentityValid)
-		if condition != nil {
-			return false, fmt.Sprintf("not done waiting for API Export condition status:%v - reason: %v - message: %v", condition.Status, condition.Reason, condition.Message)
-		}
-		return false, "not done waiting for APIExportIdentity to be marked valid, no condition exists"
-	}, wait.ForeverTestTimeout, 100*time.Millisecond, "could not wait for APIExport to be valid with identity hash")
+	sheriffExport, err := kcpClusterClient.Cluster(providerPath).ApisV1alpha1().APIExports().Get(ctx, "wild.wild.west", metav1.GetOptions{})
+	require.NoError(t, err)
+	identityHash := sheriffExport.Status.IdentityHash
 
 	t.Logf("Found identity hash: %v", identityHash)
 	apifixtures.BindToExport(ctx, t, providerPath, "wild.wild.west", consumerPath, kcpClusterClient)
@@ -97,21 +86,9 @@ func TestAPIBindingPermissionClaimsConditions(t *testing.T) {
 
 	// validate the invalid claims condition occurs
 	t.Logf("validate that the permission claim's conditions are false and invalid claims is the reason")
-	framework.Eventually(t, func() (bool, string) {
-		// get the binding
-		binding, err := kcpClusterClient.Cluster(consumerPath).ApisV1alpha1().APIBindings().Get(ctx, "cowboys", metav1.GetOptions{})
-		require.NoError(t, err, "failed to get binding")
-
-		cond := conditions.Get(binding, apisv1alpha1.PermissionClaimsValid)
-		if cond == nil {
-			return false, fmt.Sprintf("not done waiting for permission claims to be invalid, no %q condition exists:\n%s", apisv1alpha1.PermissionClaimsValid, toYAML(t, binding.Status.Conditions))
-		}
-
-		if cond.Status == v1.ConditionFalse && cond.Reason == apisv1alpha1.InvalidPermissionClaimsReason {
-			return true, ""
-		}
-		return false, fmt.Sprintf("not done waiting for condition to be invalid reason:\n%s", toYAML(t, binding.Status.Conditions))
-	}, wait.ForeverTestTimeout, 100*time.Millisecond, "unable to see invalid identity hash")
+	framework.EventuallyCondition(t, func() (conditions.Getter, error) {
+		return kcpClusterClient.Cluster(consumerPath).ApisV1alpha1().APIBindings().Get(ctx, "cowboys", metav1.GetOptions{})
+	}, framework.IsNot(apisv1alpha1.PermissionClaimsValid).WithReason(apisv1alpha1.InvalidPermissionClaimsReason), "unable to see invalid identity hash")
 
 	t.Logf("update to correct hash")
 	// have to use eventually because controllers may be modifying the APIBinding
@@ -127,45 +104,19 @@ func TestAPIBindingPermissionClaimsConditions(t *testing.T) {
 	}, wait.ForeverTestTimeout, 100*time.Millisecond, "error updating to correct hash")
 
 	t.Logf("Validate that the permission claims are valid")
-	framework.Eventually(t, func() (bool, string) {
-		// get the binding
-		binding, err := kcpClusterClient.Cluster(consumerPath).ApisV1alpha1().APIBindings().Get(ctx, "cowboys", metav1.GetOptions{})
-		if err != nil {
-			return false, err.Error()
-		}
-
-		cond := conditions.Get(binding, apisv1alpha1.PermissionClaimsValid)
-		if cond == nil {
-			return false, "not done waiting for permission claims to be valid, no condition exits"
-		}
-
-		if cond.Status != v1.ConditionTrue {
-			return false, fmt.Sprintf("not done waiting for the condition to be valid, reason: %v - message: %v", cond.Reason, cond.Message)
-		}
-		if !reflect.DeepEqual(makePermissionClaims(identityHash), binding.Status.ExportPermissionClaims) {
-			return false, fmt.Sprintf("ExportPermissionClaims unexpected %v", cmp.Diff(makePermissionClaims(identityHash), binding.Status.ExportPermissionClaims))
-		}
-		return true, ""
-	}, wait.ForeverTestTimeout, 100*time.Millisecond, "unable to see valid claims condition")
+	framework.EventuallyCondition(t, func() (conditions.Getter, error) {
+		return kcpClusterClient.Cluster(consumerPath).ApisV1alpha1().APIBindings().Get(ctx, "cowboys", metav1.GetOptions{})
+	}, framework.Is(apisv1alpha1.PermissionClaimsValid), "unable to see valid claims")
+	binding, err := kcpClusterClient.Cluster(consumerPath).ApisV1alpha1().APIBindings().Get(ctx, "cowboys", metav1.GetOptions{})
+	require.NoError(t, err)
+	if !reflect.DeepEqual(makePermissionClaims(identityHash), binding.Status.ExportPermissionClaims) {
+		require.Emptyf(t, cmp.Diff(makePermissionClaims(identityHash), binding.Status.ExportPermissionClaims), "ExportPermissionClaims incorrect")
+	}
 
 	t.Logf("Validate that the permission claims were all applied")
-	framework.Eventually(t, func() (bool, string) {
-		// get the binding
-		binding, err := kcpClusterClient.Cluster(consumerPath).ApisV1alpha1().APIBindings().Get(ctx, "cowboys", metav1.GetOptions{})
-		if err != nil {
-			return false, err.Error()
-		}
-
-		cond := conditions.Get(binding, apisv1alpha1.PermissionClaimsApplied)
-		if cond == nil {
-			return false, "not done waiting for permission claims to be applied, no condition exits"
-		}
-
-		if cond.Status == v1.ConditionTrue {
-			return true, ""
-		}
-		return false, fmt.Sprintf("not done waiting for the condition to be valid, reason: %v - message: %v", cond.Reason, cond.Message)
-	}, wait.ForeverTestTimeout, 100*time.Millisecond, "unable to see valid claims condition")
+	framework.EventuallyCondition(t, func() (conditions.Getter, error) {
+		return kcpClusterClient.Cluster(consumerPath).ApisV1alpha1().APIBindings().Get(ctx, "cowboys", metav1.GetOptions{})
+	}, framework.Is(apisv1alpha1.PermissionClaimsApplied), "unable to see claims applied")
 }
 
 func makePermissionClaims(identityHash string) []apisv1alpha1.PermissionClaim {

test/e2e/apibinding/apibinding_protected_test.go

@@ -101,13 +101,12 @@ func TestProtectedAPI(t *testing.T) {
 	}, wait.ForeverTestTimeout, time.Millisecond*100, "failed to create APIBinding")
 
 	t.Logf("Make sure APIBinding %q in workspace %q is completed and up-to-date", apiBinding.Name, consumerPath)
-	require.Eventually(t, func() bool {
-		b, err := kcpClusterClient.Cluster(consumerPath).ApisV1alpha1().APIBindings().Get(ctx, apiBinding.Name, metav1.GetOptions{})
-		require.NoError(t, err)
-
-		return conditions.IsTrue(b, apisv1alpha1.InitialBindingCompleted) &&
-			conditions.IsTrue(b, apisv1alpha1.BindingUpToDate)
-	}, wait.ForeverTestTimeout, time.Millisecond*100, "APIBinding %q in workspace %q did not complete", apiBinding.Name, consumerPath)
+	framework.EventuallyCondition(t, func() (conditions.Getter, error) {
+		return kcpClusterClient.Cluster(consumerPath).ApisV1alpha1().APIBindings().Get(ctx, apiBinding.Name, metav1.GetOptions{})
+	}, framework.Is(apisv1alpha1.InitialBindingCompleted))
+	framework.EventuallyCondition(t, func() (conditions.Getter, error) {
+		return kcpClusterClient.Cluster(consumerPath).ApisV1alpha1().APIBindings().Get(ctx, apiBinding.Name, metav1.GetOptions{})
+	}, framework.Is(apisv1alpha1.BindingUpToDate))
 
 	t.Logf("Make sure gateway API resource shows up in workspace %q group version discovery", consumerPath)
 	consumerWorkspaceClient, err := kcpclientset.NewForConfig(cfg)

test/e2e/apibinding/apibinding_test.go

@@ -299,11 +299,9 @@ func TestAPIBinding(t *testing.T) {
 		}, wait.ForeverTestTimeout, time.Millisecond*100)
 
 		t.Logf("Make sure %s cowboys2 conflict with already bound %s cowboys", provider2Path, providerPath)
-		require.Eventually(t, func() bool {
-			b, err := kcpClusterClient.Cluster(consumerWorkspace).ApisV1alpha1().APIBindings().Get(ctx, "cowboys2", metav1.GetOptions{})
-			require.NoError(t, err)
-			return conditions.IsFalse(b, apisv1alpha1.InitialBindingCompleted) && conditions.GetReason(b, apisv1alpha1.InitialBindingCompleted) == apisv1alpha1.NamingConflictsReason
-		}, wait.ForeverTestTimeout, 100*time.Millisecond, "expected naming conflict")
+		framework.EventuallyCondition(t, func() (conditions.Getter, error) {
+			return kcpClusterClient.Cluster(consumerWorkspace).ApisV1alpha1().APIBindings().Get(ctx, "cowboys2", metav1.GetOptions{})
+		}, framework.IsNot(apisv1alpha1.InitialBindingCompleted).WithReason(apisv1alpha1.NamingConflictsReason), "expected naming conflict")
 	}
 
 	verifyVirtualWorkspaceURLs := func(serviceProviderClusterName logicalcluster.Name) {

test/e2e/conversion/conversion_test.go

@@ -19,7 +19,6 @@ package conversion
 import (
 	"context"
 	"embed"
-	"fmt"
 	"strings"
 	"testing"
 	"time"
@@ -76,16 +75,9 @@ func TestAPIConversion(t *testing.T) {
 	}, wait.ForeverTestTimeout, 100.*time.Millisecond, "failed to set up test resources")
 
 	t.Logf("Waiting for initial binding to complete")
-	framework.Eventually(t, func() (bool, string) {
-		apiBinding, err := kcpClusterClient.Cluster(orgPath).ApisV1alpha1().APIBindings().Get(ctx, "widgets.example.io", metav1.GetOptions{})
-		require.NoError(t, err, "error getting APIBinding")
-
-		if conditions.IsTrue(apiBinding, apisv1alpha1.InitialBindingCompleted) {
-			return true, ""
-		}
-
-		return false, fmt.Sprintf("%v", apiBinding.Status)
-	}, wait.ForeverTestTimeout, 100*time.Millisecond, "APIBinding never completed its initial binding")
+	framework.EventuallyCondition(t, func() (conditions.Getter, error) {
+		return kcpClusterClient.Cluster(orgPath).ApisV1alpha1().APIBindings().Get(ctx, "widgets.example.io", metav1.GetOptions{})
+	}, framework.Is(apisv1alpha1.InitialBindingCompleted), "APIBinding never completed its initial binding")
 
 	framework.Eventually(t, func() (success bool, reason string) {
 		t.Logf("Resetting the RESTMapper so it can pick up widgets")

test/e2e/fixtures/apifixtures/apibinding.go

@@ -23,11 +23,9 @@ import (
 	"time"
 
 	"github.com/kcp-dev/logicalcluster/v3"
-	"github.com/stretchr/testify/require"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
-	"sigs.k8s.io/yaml"
 
 	apisv1alpha1 "github.com/kcp-dev/kcp/pkg/apis/apis/v1alpha1"
 	"github.com/kcp-dev/kcp/pkg/apis/third_party/conditions/util/conditions"
@@ -70,17 +68,7 @@ func BindToExport(
 		return true, ""
 	}, wait.ForeverTestTimeout, 100*time.Millisecond)
 
-	framework.Eventually(t, func() (bool, string) {
-		b, err := clusterClient.Cluster(bindingClusterName).ApisV1alpha1().APIBindings().Get(ctx, binding.Name, metav1.GetOptions{})
-		require.NoError(t, err, "error getting APIBinding %s|%s", bindingClusterName, binding.Name)
-
-		return conditions.IsTrue(b, apisv1alpha1.InitialBindingCompleted), toYAML(t, b.Status.Conditions)
-	}, wait.ForeverTestTimeout, 100*time.Millisecond)
-}
-
-func toYAML(t *testing.T, obj interface{}) string {
-	t.Helper()
-	bs, err := yaml.Marshal(obj)
-	require.NoError(t, err, "error converting to YAML")
-	return string(bs)
+	framework.EventuallyCondition(t, func() (conditions.Getter, error) {
+		return clusterClient.Cluster(bindingClusterName).ApisV1alpha1().APIBindings().Get(ctx, binding.Name, metav1.GetOptions{})
+	}, framework.Is(apisv1alpha1.InitialBindingCompleted))
 }

test/e2e/framework/util.go

@@ -45,6 +45,7 @@ import (
 	"github.com/martinlindhe/base36"
 	"github.com/stretchr/testify/require"
 
+	corev1 "k8s.io/api/core/v1"
 	kcpapiextensionsclientset "k8s.io/apiextensions-apiserver/pkg/client/kcp/clientset/versioned"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -342,18 +343,79 @@ func Eventually(t *testing.T, condition func() (success bool, reason string), wa
 
 // EventuallyReady asserts that the object returned by getter() eventually has a ready condition.
 func EventuallyReady(t *testing.T, getter func() (conditions.Getter, error), msgAndArgs ...interface{}) {
+	t.Helper()
+	EventuallyCondition(t, getter, Is(conditionsv1alpha1.ReadyCondition), msgAndArgs...)
+}
+
+type ConditionEvaluator struct {
+	conditionType   conditionsv1alpha1.ConditionType
+	conditionStatus corev1.ConditionStatus
+	conditionReason *string
+}
+
+func (c *ConditionEvaluator) matches(object conditions.Getter) (*conditionsv1alpha1.Condition, string, bool) {
+	condition := conditions.Get(object, c.conditionType)
+	if condition == nil {
+		return nil, c.descriptor(), false
+	}
+	if condition.Status != c.conditionStatus {
+		return condition, c.descriptor(), false
+	}
+	if c.conditionReason != nil && condition.Reason != *c.conditionReason {
+		return condition, c.descriptor(), false
+	}
+	return condition, c.descriptor(), true
+}
+
+func (c *ConditionEvaluator) descriptor() string {
+	var descriptor string
+	switch c.conditionStatus {
+	case corev1.ConditionTrue:
+		descriptor = "to be"
+	case corev1.ConditionFalse:
+		descriptor = "not to be"
+	case corev1.ConditionUnknown:
+		descriptor = "to not know if it is"
+	}
+	descriptor += fmt.Sprintf(" %s", c.conditionType)
+	if c.conditionReason != nil {
+		descriptor += fmt.Sprintf(" (with reason %s)", *c.conditionReason)
+	}
+	return descriptor
+}
+
+func Is(conditionType conditionsv1alpha1.ConditionType) *ConditionEvaluator {
+	return &ConditionEvaluator{
+		conditionType:   conditionType,
+		conditionStatus: corev1.ConditionTrue,
+	}
+}
+
+func IsNot(conditionType conditionsv1alpha1.ConditionType) *ConditionEvaluator {
+	return &ConditionEvaluator{
+		conditionType:   conditionType,
+		conditionStatus: corev1.ConditionFalse,
+	}
+}
+
+func (c *ConditionEvaluator) WithReason(reason string) *ConditionEvaluator {
+	c.conditionReason = &reason
+	return c
+}
+
+// EventuallyCondition asserts that the object returned by getter() eventually has a condition that matches the evaluator.
+func EventuallyCondition(t *testing.T, getter func() (conditions.Getter, error), evaluator *ConditionEvaluator, msgAndArgs ...interface{}) {
 	t.Helper()
 	Eventually(t, func() (bool, string) {
 		obj, err := getter()
 		require.NoError(t, err, "Error fetching object")
-		done := conditions.IsTrue(obj, conditionsv1alpha1.ReadyCondition)
+		condition, descriptor, done := evaluator.matches(obj)
 		var reason string
 		if !done {
-			condition := conditions.Get(obj, conditionsv1alpha1.ReadyCondition)
 			if condition != nil {
-				reason = fmt.Sprintf("Not done waiting for object to be ready: %s: %s", condition.Reason, condition.Message)
+				reason = fmt.Sprintf("Not done waiting for object %s: %s: %s", descriptor, condition.Reason, condition.Message)
 			} else {
-				reason = "Not done waiting for object to be ready: no condition present"
+				reason = fmt.Sprintf("Not done waiting for object %s: no condition present", descriptor)
 			}
 		}
 		return done, reason

test/e2e/garbagecollector/garbagecollector_test.go

@@ -29,7 +29,6 @@ import (
 	"github.com/kcp-dev/logicalcluster/v3"
 	"github.com/stretchr/testify/require"
 
-	corev1 "k8s.io/api/core/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	kcpapiextensionsclientset "k8s.io/apiextensions-apiserver/pkg/client/kcp/clientset/versioned"
 	kcpapiextensionsv1client "k8s.io/apiextensions-apiserver/pkg/client/kcp/clientset/versioned/typed/apiextensions/v1"
@@ -179,18 +178,9 @@ func TestGarbageCollectorTypesFromBinding(t *testing.T) {
 			}, wait.ForeverTestTimeout, 100*time.Millisecond, "error creating APIBinding")
 
 			t.Logf("Wait for the binding to be ready")
-			framework.Eventually(t, func() (bool, string) {
-				binding, err := kcpClusterClient.Cluster(userPath).ApisV1alpha1().APIBindings().Get(c, binding.Name, metav1.GetOptions{})
-				require.NoError(t, err, "error getting binding %s", binding.Name)
-				condition := conditions.Get(binding, apisv1alpha1.InitialBindingCompleted)
-				if condition == nil {
-					return false, fmt.Sprintf("no %s condition exists", apisv1alpha1.InitialBindingCompleted)
-				}
-				if condition.Status == corev1.ConditionTrue {
-					return true, ""
-				}
-				return false, fmt.Sprintf("not done waiting for the binding to be initially bound, reason: %v - message: %v", condition.Reason, condition.Message)
-			}, wait.ForeverTestTimeout, time.Millisecond*100)
+			framework.EventuallyCondition(t, func() (conditions.Getter, error) {
+				return kcpClusterClient.Cluster(userPath).ApisV1alpha1().APIBindings().Get(c, binding.Name, metav1.GetOptions{})
+			}, framework.Is(apisv1alpha1.InitialBindingCompleted))
 
 			wildwestClusterClient, err := wildwestclientset.NewForConfig(server.BaseConfig(t))
 			require.NoError(t, err, "failed to construct wildwest cluster client for server")

test/e2e/quota/quota_test.go

@@ -182,18 +182,9 @@ func TestKubeQuotaCoreV1TypesFromBinding(t *testing.T) {
 			}, wait.ForeverTestTimeout, 100*time.Millisecond, "error creating APIBinding")
 
 			t.Logf("Wait for binding to be ready")
-			framework.Eventually(t, func() (bool, string) {
-				binding, err := kcpClusterClient.Cluster(userPath).ApisV1alpha1().APIBindings().Get(ctx, binding.Name, metav1.GetOptions{})
-				require.NoError(t, err, "error getting binding %s", binding.Name)
-				condition := conditions.Get(binding, apisv1alpha1.InitialBindingCompleted)
-				if condition == nil {
-					return false, fmt.Sprintf("no %s condition exists", apisv1alpha1.InitialBindingCompleted)
-				}
-				if condition.Status == corev1.ConditionTrue {
-					return true, ""
-				}
-				return false, fmt.Sprintf("not done waiting for the binding to be initially bound, reason: %v - message: %v", condition.Reason, condition.Message)
-			}, wait.ForeverTestTimeout, time.Millisecond*100)
+			framework.EventuallyCondition(t, func() (conditions.Getter, error) {
+				return kcpClusterClient.Cluster(userPath).ApisV1alpha1().APIBindings().Get(ctx, binding.Name, metav1.GetOptions{})
+			}, framework.Is(apisv1alpha1.InitialBindingCompleted))
 
 			t.Logf("Wait for being able to list Services in the user workspace")
 			framework.Eventually(t, func() (bool, string) {
@@ -322,10 +313,7 @@ func TestKubeQuotaNormalCRDs(t *testing.T) {
 			sheriff := NewSheriff(group, fmt.Sprintf("ws%d-%d", wsIndex, i))
 			i++
 			_, err := dynamicClusterClient.Cluster(ws).Resource(sheriffsGVR).Namespace("default").Create(ctx, sheriff, metav1.CreateOptions{})
-			if err != nil {
-				return apierrors.IsForbidden(err), err.Error()
-			}
-			return false, "expected an error trying to create a sheriff"
+			return apierrors.IsForbidden(err), fmt.Sprintf("expected a forbidden error, got: %v", err)
 		}, wait.ForeverTestTimeout, 100*time.Millisecond, "quota never rejected sheriff creation")
 	}
 }