test: refactor WatchForSync to take opt struct

This refactor adds flexibility to WatchForSync to accept options in
addition to WatchOption. This is built on by a subsequent PR.

The usage of ResourceGroup predicates are removed because
WatchForAllSyncs watches RSyncs, so these predicates were invalid.

Author: sdowell

e2e/nomostest/wait_for_sync.go

@@ -37,13 +37,12 @@ import (
 )
 
 type watchForAllSyncsOptions struct {
-	timeout            time.Duration
 	readyCheck         bool
 	syncRootRepos      bool
 	syncNamespaceRepos bool
 	skipRootRepos      map[string]bool
 	skipNonRootRepos   map[types.NamespacedName]bool
-	predicates         []testpredicates.Predicate
+	watchForSyncOpts   []WatchForSyncOption
 }
 
 // WatchForAllSyncsOptions is an optional parameter for WaitForRepoSyncs.
@@ -52,14 +51,8 @@ type WatchForAllSyncsOptions func(*watchForAllSyncsOptions)
 // WithTimeout provides the timeout to WaitForRepoSyncs.
 func WithTimeout(timeout time.Duration) WatchForAllSyncsOptions {
 	return func(options *watchForAllSyncsOptions) {
-		options.timeout = timeout
-	}
-}
-
-// WithPredicates adds additional predicates for all reconcilers to WaitForRepoSyncs.
-func WithPredicates(predicates ...testpredicates.Predicate) WatchForAllSyncsOptions {
-	return func(options *watchForAllSyncsOptions) {
-		options.predicates = append(predicates, predicates...)
+		options.watchForSyncOpts = append(options.watchForSyncOpts,
+			WithWatchOptions(testwatcher.WatchTimeout(timeout)))
 	}
 }
 
@@ -114,13 +107,12 @@ func SkipReadyCheck() WatchForAllSyncsOptions {
 func (nt *NT) WatchForAllSyncs(options ...WatchForAllSyncsOptions) error {
 	nt.T.Helper()
 	opts := watchForAllSyncsOptions{
-		timeout:            nt.DefaultWaitTimeout,
 		readyCheck:         true,
 		syncRootRepos:      true,
 		syncNamespaceRepos: true,
 		skipRootRepos:      make(map[string]bool),
 		skipNonRootRepos:   make(map[types.NamespacedName]bool),
-		predicates:         []testpredicates.Predicate{},
+		watchForSyncOpts:   []WatchForSyncOption{},
 	}
 	// Override defaults with specified options
 	for _, option := range options {
@@ -133,13 +125,6 @@ func (nt *NT) WatchForAllSyncs(options ...WatchForAllSyncsOptions) error {
 		}
 	}
 
-	watchOptions := []testwatcher.WatchOption{
-		testwatcher.WatchTimeout(opts.timeout),
-	}
-	if len(opts.predicates) > 0 {
-		watchOptions = append(watchOptions, testwatcher.WatchPredicates(opts.predicates...))
-	}
-
 	tg := taskgroup.New()
 
 	if opts.syncRootRepos {
@@ -151,7 +136,7 @@ func (nt *NT) WatchForAllSyncs(options ...WatchForAllSyncsOptions) error {
 			tg.Go(func() error {
 				return nt.WatchForSync(
 					kinds.RootSyncV1Beta1(), idPtr.Name, idPtr.Namespace, source,
-					watchOptions...)
+					opts.watchForSyncOpts...)
 			})
 		}
 	}
@@ -165,14 +150,29 @@ func (nt *NT) WatchForAllSyncs(options ...WatchForAllSyncsOptions) error {
 			tg.Go(func() error {
 				return nt.WatchForSync(
 					kinds.RepoSyncV1Beta1(), idPtr.Name, idPtr.Namespace, source,
-					watchOptions...)
+					opts.watchForSyncOpts...)
 			})
 		}
 	}
 
 	return tg.Wait()
 }
 
+type watchForSyncOptions struct {
+	watchOptions []testwatcher.WatchOption
+}
+
+// WatchForSyncOption is an optional parameter for WatchForSync.
+type WatchForSyncOption func(*watchForSyncOptions)
+
+// WithWatchOptions is an optional parameter to specify WatchOptions used for
+// both the RSync watch and ResourceGroup watch.
+func WithWatchOptions(watchOpts ...testwatcher.WatchOption) WatchForSyncOption {
+	return func(options *watchForSyncOptions) {
+		options.watchOptions = append(options.watchOptions, watchOpts...)
+	}
+}
+
 // WatchForSync watches the specified sync object until it's synced.
 //
 //   - gvk (required) is the sync object GroupVersionKind
@@ -184,9 +184,16 @@ func (nt *NT) WatchForSync(
 	gvk schema.GroupVersionKind,
 	name, namespace string,
 	source syncsource.SyncSource,
-	opts ...testwatcher.WatchOption,
+	options ...WatchForSyncOption,
 ) error {
 	nt.T.Helper()
+	opts := watchForSyncOptions{
+		watchOptions: []testwatcher.WatchOption{},
+	}
+	// Override defaults with specified options
+	for _, option := range options {
+		option(&opts)
+	}
 	if namespace == "" {
 		// If namespace is empty, use the default namespace
 		namespace = configsync.ControllerNamespace
@@ -222,9 +229,12 @@ func (nt *NT) WatchForSync(
 				testpredicates.ErrWrongType, gvk.Kind))
 	}
 
-	opts = append(opts, testwatcher.WatchPredicates(predicates...))
+	rsyncWatchOptions := []testwatcher.WatchOption{
+		testwatcher.WatchPredicates(predicates...),
+	}
+	rsyncWatchOptions = append(rsyncWatchOptions, opts.watchOptions...)
 
-	err = nt.Watcher.WatchObject(gvk, name, namespace, opts...)
+	err = nt.Watcher.WatchObject(gvk, name, namespace, rsyncWatchOptions...)
 	if err != nil {
 		return fmt.Errorf("waiting for sync: %w", err)
 	}

e2e/testcases/profiling_test.go

@@ -24,7 +24,6 @@ import (
 	"kpt.dev/configsync/e2e/nomostest/gitproviders"
 	"kpt.dev/configsync/e2e/nomostest/ntopts"
 	nomostesting "kpt.dev/configsync/e2e/nomostest/testing"
-	"kpt.dev/configsync/e2e/nomostest/testpredicates"
 	"kpt.dev/configsync/pkg/api/configsync"
 	"kpt.dev/configsync/pkg/api/configsync/v1beta1"
 	"kpt.dev/configsync/pkg/core/k8sobjects"
@@ -68,9 +67,7 @@ func TestProfilingResourcesByObjectCount(t *testing.T) {
 
 		// Validate that the resources sync without the reconciler running out of
 		// memory, getting OOMKilled, and crash looping.
-		nt.Must(nt.WatchForAllSyncs(nomostest.WithPredicates(
-			testpredicates.AllResourcesReconciled(nt.Scheme)),
-		))
+		nt.Must(nt.WatchForAllSyncs())
 
 		nt.T.Logf("Verify the number of Deployment objects")
 		nt.Must(validateNumberOfObjectsEquals(nt, kinds.Deployment(), deployCount,
@@ -83,9 +80,7 @@ func TestProfilingResourcesByObjectCount(t *testing.T) {
 
 		// Validate that the resources sync without the reconciler running out of
 		// memory, getting OOMKilled, and crash looping.
-		nt.Must(nt.WatchForAllSyncs(nomostest.WithPredicates(
-			testpredicates.AllResourcesReconciled(nt.Scheme)),
-		))
+		nt.Must(nt.WatchForAllSyncs())
 	}
 }
 
@@ -158,9 +153,7 @@ func TestProfilingResourcesByObjectCountWithMultiSync(t *testing.T) {
 
 		// Validate that the resources sync without the reconciler running out of
 		// memory, getting OOMKilled, and crash looping.
-		nt.Must(nt.WatchForAllSyncs(nomostest.WithPredicates(
-			testpredicates.AllResourcesReconciled(nt.Scheme)),
-		))
+		nt.Must(nt.WatchForAllSyncs())
 
 		nt.T.Logf("Verify the number of Deployment objects")
 		nt.Must(validateNumberOfObjectsEquals(nt, kinds.Deployment(), totalDeployCount,
@@ -179,9 +172,7 @@ func TestProfilingResourcesByObjectCountWithMultiSync(t *testing.T) {
 
 		// Validate that the resources sync without the reconciler running out of
 		// memory, getting OOMKilled, and crash looping.
-		nt.Must(nt.WatchForAllSyncs(nomostest.WithPredicates(
-			testpredicates.AllResourcesReconciled(nt.Scheme)),
-		))
+		nt.Must(nt.WatchForAllSyncs())
 
 		nt.T.Logf("Verify all Deployments deleted")
 		nt.Must(validateNumberOfObjectsEquals(nt, kinds.Deployment(), 0,
@@ -277,9 +268,7 @@ func TestProfilingByObjectCountAndSyncCount(t *testing.T) {
 
 		// Validate that the resources sync without the reconciler running out of
 		// memory, getting OOMKilled, and crash looping.
-		nt.Must(nt.WatchForAllSyncs(nomostest.WithPredicates(
-			testpredicates.AllResourcesReconciled(nt.Scheme)),
-		))
+		nt.Must(nt.WatchForAllSyncs())
 
 		nt.T.Logf("Verify the number of Deployment objects")
 		nt.Must(validateNumberOfObjectsEquals(nt, kinds.Deployment(), totalDeployCount,