🌱 End-to-end tests for APIExportEndpointSlice #2608

fgiloux · 2023-01-12T09:54:11Z

Summary

This PR introduces end-to-end tests covering APIExportEndpointSlice reconcilation.

Related issue(s)

Contributes to #2332

fgiloux · 2023-01-12T09:55:17Z

/hold will be rebased on top of #2469 when it has merged

fgiloux · 2023-01-12T16:49:32Z

shard creation introduces side effects with other end-to-end tests.
This can be addressed in #2612
This PR could either get progressed without the check for the endpoint URLs (currently commented out) or wait for #2612 to be addressed.

fgiloux · 2023-01-16T19:47:17Z

/unhold

test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go

p0lyn0mial · 2023-01-12T16:52:28Z

test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go

+		ObjectMeta: metav1.ObjectMeta{
+			Name: "my-export",
+		},
+		Spec: apisv1alpha1.APIExportSpec{},


is this required?

no it is not. Removing it.

p0lyn0mial · 2023-01-12T16:57:45Z

test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go

+	mergePatch, err := jsonpatch.CreateMergePatch(encodeJSON(t, slice), encodeJSON(t, patchedSlice))
+	require.NoError(t, err)
+
+	t.Logf("Try to patch the APIExportEndpointSlice to point at a Partition")


nit: would rename to Patching . . . for consistency with the previous comments

p0lyn0mial · 2023-01-17T10:52:48Z

test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go

+	}
+}
+
+func newPrivateEnvironment(t *testing.T) *environment {


I'd remove environment, addWorkspacesAndClient, addRootClient and I'd change the signature of createAndTestResources method to accept clients. I think it would be easier to read and extend in the future.

The paths to the two logical clusters would also need to be passed as parameters. The code of addWorkspacesAndClient would need to be replicated in both test cases.
I am not sure that the result would improve readability and extensibility. To me it is more of a style preference. Do you have any concrete point in mind where it would make a difference?

p0lyn0mial · 2023-01-17T11:09:34Z

test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go

+
+	t.Logf("Retrying to create the APIExportEndpointSlice after the APIExport has been created")
+	require.Eventually(t, func() bool {
+		slice, err = sliceClient.Cluster(e.partitionClusterPath).Create(ctx, slice, metav1.CreateOptions{})


nit: since we are creating the slice here would it make sense to change the name of the cluster to serviceProviderPath?

I am not sure. Both workspaces, the one with the APIExport and the one with the Partition and APIExportEndpointSlice are managed by the service provider.

p0lyn0mial · 2023-01-17T11:11:55Z

test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go

+	t.Logf("Adding a Partition to the APIExportEndpointSlice")
+	patchedSlice := slice.DeepCopy()
+	patchedSlice.Spec.Partition = partition.Name
+	mergePatch, err := jsonpatch.CreateMergePatch(encodeJSON(t, slice), encodeJSON(t, patchedSlice))


nit: i'd just update, seems we could get rid of encodeJSON methods

p0lyn0mial · 2023-01-17T11:12:56Z

test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go

+		require.NoError(t, err)
+		return conditions.IsTrue(s, apisv1alpha1.APIExportValid)
+	}, wait.ForeverTestTimeout, 100*time.Millisecond, "expected valid APIExport")
+


shouldn't we examine APIExportEndpoints at this point ?

For that we would need to make assumptions on the shards in a shared environment, which I think it is better to avoid as long as we have not defined clear conventions.
Endpoints population is specifically tested in the private environment. If we could do it here we would not need the private cluster. I would prefer not to need it but it comes back to have clear conventions consistently followed (possibly enforced) across the e2e tests, e.g.:

3 shards with constant labels never modified nor deleted

any shard created/modified/deleted during testing uses different labels

e2e tests should not be sensible to shards created/modified in other test cases: nothing should get scheduled on them implicitly, without explicit scheduling workspaces should get created on the original 3 shards.

In general we can assume that an environment created by the test binary is stable (either single or multi-shard) and doesn't change. Tests that are destructive should set-up their own env.

Validating if APIExportEndpointSlice without a Partition has all shards assigned seems worth checking.

Is it ? The test case appears to be adding a shard that matches a partition assigned to a APIExportEndpointSlice.

This is not the same as "Validating if a APIExportEndpointSlice without a Partition has all (default) shards assigned" or is it?

Added an end-to-end test for APIExportEndpointSlice without partition in the private environment.

p0lyn0mial · 2023-01-17T11:13:47Z

test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go

+	require.Eventually(t, func() bool {
+		s, err := e.kcpClusterClient.Cluster(e.partitionClusterPath).ApisV1alpha1().APIExportEndpointSlices().Get(ctx, slice.Name, metav1.GetOptions{})
+		require.NoError(t, err)
+		return conditions.IsFalse(s, apisv1alpha1.PartitionValid) && conditions.GetReason(s, apisv1alpha1.PartitionValid) == apisv1alpha1.PartitionInvalidReferenceReason


shouldn't we examine APIExportEndpoints with invalid partition?

I can check that there are no endpoints.

p0lyn0mial · 2023-01-17T11:15:25Z

test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go

+		require.NoError(t, err)
+		return len(s.Status.APIExportEndpoints) == 0
+	}, wait.ForeverTestTimeout, 100*time.Millisecond, "not expecting any endpoint")
+


would it make sense to add a scenario with a partition that would select some shards ? could be just 1 in multi-shard env.

As per the above this is covered with the private server.

p0lyn0mial · 2023-01-18T10:01:25Z

test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go

+	e.addWorkspacesAndClient(t)
+	e.addRootClient(t)
+
+	slice := e.createAndTestResources(ctx, t)


mhm, are we already testing it in TestAPIExportEndpointSlice ?

these are setting up prerequisites for the tests that are run in the private environment.

sttts · 2023-01-26T10:56:56Z

test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go

+	}, wait.ForeverTestTimeout, 100*time.Millisecond, "expecting a single endpoint for the root shard, got %d", len(sliceWithAll.Status.APIExportEndpoints))
+}
+
+func TestAPIExportEndpointSlice(t *testing.T) {


have a hard time what these tests actually do. How is TestAPIExportEndpointSlice and TestAPIExportEndpointSlicePrivate different?

Hiding the core test logic in helpers is an anti-pattern. Build helpers (DSL-like) to build up the test setup, but put the actual thing tested here as high level as possible.

And this core test code should read like a story, i.e. not full of helpers again, rather this should be a series of steps plus require.SomeThing lines.

TestAPIExportEndpointSlicePrivate manipulates Shards, hence the need for a private cluster. It requires however the APIExport, APIExportEndpointSlice and other resources. The building of these resources is what is tested in TestAPIExportEndpointSlice. Lukasz explicitly asked for the tests to be run on both: a private and a shared cluster (for the part compatible with it). Without this ask TestAPIExportEndpointSlice would not be needed.

sttts · 2023-01-26T11:06:38Z

test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go

+	"github.com/kcp-dev/kcp/test/e2e/framework"
+)
+
+type environment struct {


most tests where we used this pattern turned out to be hard to read. As written below, don't follow the temptation to put the actual tests (= do X + require statements) into helpers. This rule of thumb basically excludes this pattern of abstracting the environments.

Removed the struct and put everything in the test cases, no functions anymore.

p0lyn0mial · 2023-01-27T08:43:22Z

test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go

+
+	// Create Organization and Workspaces
+	orgPath, _ := framework.NewOrganizationFixture(t, server)
+	var exportWs *tenancyv1alpha1.Workspace


nit: you could remove declaration since the variable will be created in line 49

p0lyn0mial · 2023-01-27T08:43:32Z

test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go

+	var exportWs *tenancyv1alpha1.Workspace
+	exportClusterPath, exportWs := framework.NewWorkspaceFixture(t, server, orgPath, framework.WithName("export-workspace"))
+	t.Logf("Export Workspace %v", exportWs)
+	var partitionWs *tenancyv1alpha1.Workspace


nit: you could remove declaration since the variable will be created in line 49

p0lyn0mial · 2023-01-27T08:46:02Z

test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go

+	orgPath, _ := framework.NewOrganizationFixture(t, server)
+	var exportWs *tenancyv1alpha1.Workspace
+	exportClusterPath, exportWs := framework.NewWorkspaceFixture(t, server, orgPath, framework.WithName("export-workspace"))
+	t.Logf("Export Workspace %v", exportWs)


NewWorkspaceFixture already logs into the log file, i.e. Created %s workspace %s as /clusters/%s

besides it looks like exportWs is unused :)

p0lyn0mial · 2023-01-27T08:46:29Z

test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go

+	t.Logf("Export Workspace %v", exportWs)
+	var partitionWs *tenancyv1alpha1.Workspace
+	partitionClusterPath, partitionWs := framework.NewWorkspaceFixture(t, server, orgPath, framework.WithName("partition"))
+	t.Logf("Partition Workspace %v", partitionWs)


NewWorkspaceFixture already logs into the log file, i.e. Created %s workspace %s as /clusters/%s

besides it looks like partitionWs is unused :)

p0lyn0mial · 2023-01-27T09:08:49Z

test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go

+	// Create Organization and Workspaces
+	orgPath, _ := framework.NewOrganizationFixture(t, server)
+	var exportWs *tenancyv1alpha1.Workspace
+	exportClusterPath, exportWs := framework.NewWorkspaceFixture(t, server, orgPath, framework.WithName("export-workspace"))


could we remove framework.WithName("export-workspace") ? I think that would allow for running the test multiple times without side-effects.

p0lyn0mial · 2023-01-27T09:19:33Z

test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go

+	require.Eventually(t, func() bool {
+		s, err := kcpClusterClient.Cluster(partitionClusterPath).ApisV1alpha1().APIExportEndpointSlices().Get(ctx, slice.Name, metav1.GetOptions{})
+		require.NoError(t, err)
+		return len(s.Status.APIExportEndpoints) == 0


what is the point of doing this call in require.Eventually since it will stop after the first iteration?

p0lyn0mial · 2023-01-27T09:21:45Z

test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go

+	"github.com/kcp-dev/kcp/test/e2e/framework"
+)
+
+func TestAPIExportEndpointSlice(t *testing.T) {


I would rename to TestAPIExportEndpointSliceWithPartition

p0lyn0mial · 2023-01-27T09:23:48Z

test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go

+	"github.com/kcp-dev/kcp/test/e2e/framework"
+)
+
+func TestAPIExportEndpointSlice(t *testing.T) {


It looks like we are missing a happy-case scenario. An SP creates a slices and it gets assigned all/default endpoints.

I think that we discussed this point earlier. It is in the private test case where shards are guaranteed not to be modified by any other test.

The destructive scenario is different. I think it is about checking if adding a shard that matches partition requirements results in adding endpoints to the slice.

I think we are missing a default case - when I add a slice will it be filled with endpoints that correspond to the current number of shards?

The default case should work on single and multi-shard environment and should not add, remove or modify shards.

The scenario is covered here: https://github.com/kcp-dev/kcp/pull/2608/files#diff-43025ba07d287623f79a84eaa3f3616ca31e4402bd6326683517094a172261efR289-R309
I don't see the point in making the shared cluster test fragile on purpose.

oh, sorry I missed it. Wouldn't it be better to have scenarios that have well defined limited scope :) ?

p0lyn0mial · 2023-01-27T09:24:46Z

test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go

+
+	// Create Organization and Workspaces
+	orgPath, _ := framework.NewOrganizationFixture(t, server)
+	var exportWs *tenancyv1alpha1.Workspace


the same comments apply here

p0lyn0mial · 2023-01-27T09:33:02Z

test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go

+		},
+	}
+
+	t.Logf("Creating an APIExportEndpointSlice with reference to a nonexistent APIExport")


could we simplify this test case and test only what is specific for it, i.e. adding a shard that matches partition requirements results in adding endpoints to the slice ?

I can remove a bit of it but the prerequisites still need to get set up.

p0lyn0mial · 2023-01-27T11:45:36Z

test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go

+	require.Eventually(t, func() bool {
+		s, err := kcpClusterClient.Cluster(partitionClusterPath).ApisV1alpha1().APIExportEndpointSlices().Get(ctx, slice.Name, metav1.GetOptions{})
+		require.NoError(t, err)
+		return len(s.Status.APIExportEndpoints) == 1


shouldn't we check the content of s.Status.APIExportEndpoints it should match shard.Spec.VirtualWorkspaceURL and the export name ?

p0lyn0mial

also should we add a few negative scenarios (can be a new PR), like testing

kcp/pkg/reconciler/apis/apiexportendpointslice/apiexportendpointslice_reconcile.go

Line 73 in 0a230b9

// Don't keep the endpoints if the APIExport has been deleted
kcp/pkg/reconciler/apis/apiexportendpointslice/apiexportendpointslice_reconcile.go

Line 120 in 0a230b9

// Don't keep the endpoints if the Partition has been deleted

?

fgiloux · 2023-01-27T13:00:48Z

also should we add a few negative scenarios (can be a new PR), like testing

Do we want to replicate the unit tests in the e2e tests? I am a bit concerned that they already take a significant amount of time to run.

Signed-off-by: Frederic Giloux <[email protected]>

p0lyn0mial · 2023-01-30T09:06:50Z

/lgtm

sttts · 2023-01-30T09:06:53Z

/approve

openshift-ci · 2023-01-30T09:07:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts

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~~ [sttts]

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

fgiloux · 2023-01-30T10:06:05Z

/retest

fgiloux · 2023-01-30T15:20:41Z

/retest

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

openshift-ci bot requested review from csams and davidfestal January 12, 2023 09:56

openshift-ci bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jan 12, 2023

fgiloux force-pushed the slice-e2e branch 4 times, most recently from 86a6d9e to c274fcf Compare January 12, 2023 15:40

fgiloux force-pushed the slice-e2e branch 8 times, most recently from 49fb946 to 1e23c0c Compare January 16, 2023 19:29

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

fgiloux requested review from p0lyn0mial and sttts January 16, 2023 20:55

p0lyn0mial reviewed Jan 17, 2023

View reviewed changes

fgiloux force-pushed the slice-e2e branch 2 times, most recently from 8b484a0 to 6e73137 Compare January 17, 2023 14:42

p0lyn0mial reviewed Jan 18, 2023

View reviewed changes

fgiloux force-pushed the slice-e2e branch 2 times, most recently from 71638de to 149bf32 Compare January 18, 2023 17:48

sttts reviewed Jan 26, 2023

View reviewed changes

fgiloux force-pushed the slice-e2e branch from 149bf32 to 53c4e0a Compare January 26, 2023 20:06

p0lyn0mial reviewed Jan 27, 2023

View reviewed changes

fgiloux force-pushed the slice-e2e branch from 53c4e0a to 8e91bda Compare January 27, 2023 11:08

p0lyn0mial reviewed Jan 27, 2023

View reviewed changes

Add end-to-end tests for APIExportEndpointSlice

3689050

Signed-off-by: Frederic Giloux <[email protected]>

fgiloux force-pushed the slice-e2e branch from 8e91bda to 3689050 Compare January 27, 2023 13:54

openshift-ci bot assigned p0lyn0mial Jan 30, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 30, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 30, 2023

openshift-merge-robot merged commit 22d06c1 into kcp-dev:main Jan 30, 2023

fgiloux deleted the slice-e2e branch February 7, 2023 15:24

fgiloux mentioned this pull request Feb 10, 2023

🌱 PartitionSet e2e #2642

Merged

🌱 End-to-end tests for APIExportEndpointSlice #2608

🌱 End-to-end tests for APIExportEndpointSlice #2608

Conversation

fgiloux commented Jan 12, 2023

Summary

Related issue(s)

fgiloux commented Jan 12, 2023

fgiloux commented Jan 12, 2023

fgiloux commented Jan 16, 2023

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

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

fgiloux Jan 27, 2023 • edited Loading

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

fgiloux Jan 27, 2023 • edited Loading

Choose a reason for hiding this comment

p0lyn0mial Jan 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial left a comment

Choose a reason for hiding this comment

fgiloux commented Jan 27, 2023

p0lyn0mial commented Jan 30, 2023

sttts commented Jan 30, 2023

openshift-ci bot commented Jan 30, 2023

fgiloux commented Jan 30, 2023

fgiloux commented Jan 30, 2023

fgiloux Jan 27, 2023 •

edited

Loading

fgiloux Jan 27, 2023 •

edited

Loading

p0lyn0mial Jan 27, 2023 •

edited

Loading