🌱 cache: add replicateAPIResourceSchema test scenario #2240

p0lyn0mial · 2022-10-20T12:43:45Z

Summary

The new test checks if an APIResourceSchema is propagated to the cache server.
The test exercises a creation, modification and removal of the APIExport object.

Related issue(s)

part of #342

p0lyn0mial · 2022-10-20T12:46:11Z

test/e2e/reconciler/cache/replication_test.go

+			return kcpShardClusterClient.Cluster(cluster).ApisV1alpha1().APIResourceSchemas().Update(ctx, apiResSchema, metav1.UpdateOptions{})
+		},
+		updateSpecForSourceResource: func(res interface{}) error {
+			// no-op


seems like all fields are immutable, checking . . .

oh, there is an admission enforcing immutability.

stevekuznetsov

Seems reasonable, but would love to have it land after #2104

p0lyn0mial · 2022-10-20T20:14:18Z

/hold

will merge after #2104 lands

nrb · 2022-10-21T20:31:30Z

/unhold
2104 is in

stevekuznetsov · 2022-10-26T13:36:21Z

test/e2e/reconciler/cache/replication_test.go

-// TestAllAgainstInProcessCacheServer runs all test scenarios against a cache server that runs with a kcp server
-func TestAllAgainstInProcessCacheServer(t *testing.T) {
+// TestAllReplicationScenariosAgainstInProcessCacheServer runs all test scenarios against a cache server that runs with a kcp server
+func TestAllReplicationScenariosAgainstInProcessCacheServer(t *testing.T) {
 	t.Parallel()


This makes the suite parallel with the rest of the suites, but does not make each test case parallel - is that intentional? Do we want a t.Parallel() call in the t.Run()?

I don't think so, too much concurrency can negatively impact a CPU.
There is no difference on my local machine: 53s vs 52s

stevekuznetsov · 2022-10-26T13:38:14Z

test/e2e/reconciler/cache/replication_test.go

-// TestAllAgainstInProcessCacheServer runs all test scenarios against a cache server that runs with a kcp server
-func TestAllAgainstInProcessCacheServer(t *testing.T) {
+// TestAllReplicationScenariosAgainstInProcessCacheServer runs all test scenarios against a cache server that runs with a kcp server
+func TestAllReplicationScenariosAgainstInProcessCacheServer(t *testing.T) {


can we make these e.g. TestCacheServerInProcess and TestCacheServerStandalone? there's not real use to explaining that it's all scenarios, and there is nothing except for replication scenarios here

stevekuznetsov · 2022-10-26T13:38:32Z

test/e2e/reconciler/cache/replication_test.go

@@ -60,109 +61,264 @@ type testScenario struct {
 // scenarios all test scenarios that will be run against in-process and standalone cache server
 var scenarios = []testScenario{
 	{"TestReplicateAPIExport", replicateAPIExportScenario},
+	{"TestReplicateAPIResourceSchema", replicateAPIResourceSchemaScenario},


you don't need to format this as a Go test name, much more common to have e.g. "replicate APIResourceSchema" which will then turn into TestCacheServerInProcess/replicate_APIResourceSchema

stevekuznetsov · 2022-10-26T13:40:11Z

test/e2e/reconciler/cache/replication_test.go

+	resourceKind string
+
+	createSourceResource        func(logicalcluster.Name) error
+	getSourceResource           func(logicalcluster.Name) (interface{}, error)


I think we talked about this before, but there's nothing more specific that this can be, like runtime.Object? and we don't want to use generics, because?

in this particular case, we don't need strong types, we operate on meta.Accessor

Can we use meta.Accessor then?

sure, will change it.

changed to runtime.Object because I've realized we need full object for comparison.

stevekuznetsov · 2022-10-26T13:40:29Z

test/e2e/reconciler/cache/replication_test.go

+		updateSourceResource: func(cluster logicalcluster.Name, res interface{}) (interface{}, error) {
+			apiResSchema, ok := res.(*apisv1alpha1.APIResourceSchema)
+			if !ok {
+				return nil, fmt.Errorf("%T is not *APIResourceSchema", res)


With generics you don't need to write this code

yeah, this is the only case that would benefit from using generics, I can look into that in a follow-up PR.

stevekuznetsov · 2022-10-26T13:41:52Z

test/e2e/reconciler/cache/replication_test.go

+}
+
+// replicateAPIExportNegativeScenario checks if modified or even deleted cached APIExport will be reconciled to match the original object
+func replicateAPIExportNegativeScenario(ctx context.Context, t *testing.T, server framework.RunningServer, kcpShardClusterClient clientset.ClusterInterface, cacheKcpClusterClient clientset.ClusterInterface) {


Why is this APIExport-specific?

added the case for APIResourceSchema

stevekuznetsov · 2022-10-26T13:43:02Z

test/e2e/reconciler/cache/replication_test.go

-		if diff := cmp.Diff(cachedWildAPIExport, wildAPIExport, cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion")); len(diff) > 0 {
-			return false, fmt.Sprintf("replicated APIExport root/%s/%s is different that the original", cluster, wildAPIExport.Name)
+		delete(cachedResourceMeta.GetAnnotations(), genericapirequest.AnnotationKey)
+		if diff := cmp.Diff(cachedResource, originalResource, cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion")); len(diff) > 0 {


Love to see cmpopts - nice! I think you could likely get the annotation key in there too if you cared

I tried but couldn't make it to work on the annotation key

stevekuznetsov · 2022-10-26T13:43:17Z

test/e2e/reconciler/cache/replication_test.go

-			return false, fmt.Sprintf("replicated APIExport root/%s/%s is different that the original", cluster, wildAPIExport.Name)
+		delete(cachedResourceMeta.GetAnnotations(), genericapirequest.AnnotationKey)
+		if diff := cmp.Diff(cachedResource, originalResource, cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion")); len(diff) > 0 {
+			return false, fmt.Sprintf("replicated %s root|%s/%s is different that the original", scenario.resourceKind, cluster, cachedResourceMeta.GetName())
 		}
 		return true, ""
 	}, wait.ForeverTestTimeout, 400*time.Millisecond)


How did we arrive at this poll period?

just a random number

Let's use 100ms like the rest of the tests unless we really need to change it - for folks with fast workstations, 300ms extra poll time might double or triple test runtime :)

for folks with fast workstations, 300ms extra poll time might double or triple test runtime :)

that's why you want to use t.Parallel everywhere :)

Hey, what takes 65 minutes in GitHub Actions takes 54seconds on my machine ... ;)

sttts · 2022-11-03T12:01:52Z

test/e2e/reconciler/cache/replication_test.go

+}
+
+// baseScenario an auxiliary struct that is used by replicateResourceScenario
+type baseScenario struct {


am always getting a little nervous when seeing data structures like this and constructors in e2e tests. It's most often a sign for too complex tests, or for a lack of a DSL like builder language to construct the test cases in the actual Test* funcs.

Rule of thumb: if I look at some Test* func I want to know what it is doing without looking outside of this func. Builder pattern (with local funcs) is great way to provide the helpers to specify the test input in a precise and short way.

updated to use the builder pattern, ptal

…rious resources

…cateResourceScenario

tests if an APIResourceSchema is propagated to the cache server. The test exercises creation, modification and removal of the APIExport object.

…nario

sttts · 2022-11-04T13:53:42Z

/lgtm
/approved

openshift-ci · 2022-11-04T14:53:33Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

test/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot requested review from davidfestal and kylape October 20, 2022 12:43

p0lyn0mial requested a review from sttts October 20, 2022 12:45

p0lyn0mial commented Oct 20, 2022

View reviewed changes

p0lyn0mial force-pushed the cache-replication-e2e-apiresourceschema branch from 879041f to d95e92a Compare October 20, 2022 13:39

stevekuznetsov reviewed Oct 20, 2022

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 20, 2022

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 21, 2022

p0lyn0mial force-pushed the cache-replication-e2e-apiresourceschema branch 2 times, most recently from 52c86c3 to 70259dd Compare October 26, 2022 11:28

stevekuznetsov reviewed Oct 26, 2022

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2022

p0lyn0mial force-pushed the cache-replication-e2e-apiresourceschema branch from 70259dd to 084e18e Compare October 28, 2022 09:58

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2022

p0lyn0mial force-pushed the cache-replication-e2e-apiresourceschema branch from 084e18e to 808ca68 Compare October 28, 2022 10:04

sttts reviewed Nov 3, 2022

View reviewed changes

p0lyn0mial added 9 commits November 3, 2022 13:51

cache:replication:e2e: provide a generic test scenario for testing va…

6259907

…rious resources

cache:replication:e2e: update replicateAPIExportScenario to use repli…

7e92ff5

…cateResourceScenario

cache:replication:e2e: add replicateAPIResourceSchema test scenario

89bd46d

tests if an APIResourceSchema is propagated to the cache server. The test exercises creation, modification and removal of the APIExport object.

cache:replication:e2e: rename replication scenarios

6cb2179

cache:replication:e2e: extract verifyResourceExistence

712deb3

cache:replication:e2e: add ReplicateAPIExportNegative scenario

126f105

cache:replication:e2e: unify resources logging

d9af091

cache:replication:e2e: add TestReplicateAPIResourceSchemaNegative sce…

e067a45

…nario

cache:replication:e2e: refactor

9082f29

p0lyn0mial force-pushed the cache-replication-e2e-apiresourceschema branch 2 times, most recently from 8262ef3 to 7137372 Compare November 3, 2022 14:51

cache:replication:e2e: refactor to builder pattern

c1fbe65

p0lyn0mial force-pushed the cache-replication-e2e-apiresourceschema branch from 7137372 to c1fbe65 Compare November 3, 2022 16:33

openshift-ci bot assigned sttts Nov 4, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 4, 2022

p0lyn0mial added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2022

openshift-merge-robot merged commit b44cbc7 into kcp-dev:main Nov 4, 2022

p0lyn0mial mentioned this pull request Nov 23, 2022

✨ Adding ClusterWorkspaceShard to the resources stored in the cache server #2381

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 cache: add replicateAPIResourceSchema test scenario #2240

🌱 cache: add replicateAPIResourceSchema test scenario #2240

p0lyn0mial commented Oct 20, 2022

p0lyn0mial Oct 20, 2022

p0lyn0mial Oct 20, 2022

stevekuznetsov left a comment

p0lyn0mial commented Oct 20, 2022

nrb commented Oct 21, 2022

stevekuznetsov Oct 26, 2022

p0lyn0mial Oct 28, 2022

stevekuznetsov Oct 26, 2022

p0lyn0mial Oct 28, 2022

stevekuznetsov Oct 26, 2022

stevekuznetsov Oct 26, 2022

p0lyn0mial Oct 28, 2022

stevekuznetsov Oct 28, 2022

p0lyn0mial Oct 28, 2022

p0lyn0mial Oct 28, 2022

stevekuznetsov Oct 26, 2022

p0lyn0mial Oct 28, 2022

stevekuznetsov Oct 26, 2022

p0lyn0mial Oct 28, 2022

stevekuznetsov Oct 26, 2022

p0lyn0mial Oct 28, 2022

stevekuznetsov Oct 26, 2022

p0lyn0mial Oct 28, 2022

stevekuznetsov Oct 28, 2022

p0lyn0mial Oct 28, 2022

stevekuznetsov Oct 28, 2022

sttts Nov 3, 2022

p0lyn0mial Nov 3, 2022

sttts commented Nov 4, 2022

openshift-ci bot commented Nov 4, 2022

🌱 cache: add replicateAPIResourceSchema test scenario #2240

🌱 cache: add replicateAPIResourceSchema test scenario #2240

Conversation

p0lyn0mial commented Oct 20, 2022

Summary

Related issue(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevekuznetsov left a comment

Choose a reason for hiding this comment

p0lyn0mial commented Oct 20, 2022

nrb commented Oct 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Nov 4, 2022

openshift-ci bot commented Nov 4, 2022