🌱 Add composable matchers to internal/testtypes #5259

killianmuldoon · 2021-09-17T14:35:53Z

This commit adds composable matching objects to the internal
testtypes package. These types can be used for advanced comparison
between objects in the Cluster API unit and integration tests.
A simple integration test is included to show the basic working of
the matchers.

Fixes #5157

Note: This is a duplicate / update of #5157 - the branch name was not correct ahead for merge history.

killianmuldoon · 2021-09-17T16:22:00Z

/retest

sbueringer · 2021-09-20T08:21:31Z

controllers/topology/util_test.go

@@ -21,9 +21,8 @@ import (

 	"github.com/pkg/errors"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4"
-	"sigs.k8s.io/cluster-api/internal/testtypes"
+	. "sigs.k8s.io/cluster-api/internal/testtypes"


Moved from the old PR:

@sbueringer sbueringer 4 days ago Member
I'm not a fan of dot imports to be honest :/ (because of the "magic-factor")
I understand why we want it for matchers, I'm not sure if we should also use it for our builder funcs.

@killianmuldoon killianmuldoon 3 days ago Author Member
Yeah - it's a compromise here from having one package with the two different functions. We > are using . imports across our testing for Gomega. Do you have strong feelings about this? I can definitely see both sides.

@killianmuldoon It's fine for me to keep it, if that's the consensus. Overall I would like to be very careful with dot imports. Gomega is basically the only one I ever saw in Go and I think that's a good thing because it makes the code less readable.

An option could be to have the matchers and builders in separate packages and only import the matchers with a dot import (like the gomega matchers).

I would like to hear opinions from @vincepri and @fabriziopandini on that, though.

I'm personally ok with this dot import given that it is our own "additional layer" on top of gomega/testing and it makes tests more readable. However, no problem to accept whatever we agree on.

I definitely agree with having the dot imports for the matchers. It's more the builder funcs and everything else we currently have and will add in the future in the testtypes package.

I definitely agree there's a danger here - dot-by-default isn't something we want for the entire testtypes package I don't think. We could have testtypes/matchers below it - other than worries about too many small packages is there any issue with doing it that way and only doing dot import on matchers?

I don't have a concern about a separate package, only wondering if a slightly different structure would make more sense:

internal testtypes testmatchers

Or

internal test types matchers

If I understood correctly the matchers are an extension to gomega matchers for CAPI/Kubernetes so they are not really specific to testtypes they are just also part of our "test library" (so I'm mostly challenging if they should really be a sub-package of testtypes)

fabriziopandini · 2021-09-20T09:32:02Z

controllers/topology/desired_state_test.go

@@ -20,7 +20,7 @@ import (
 	"strings"
 	"testing"

-	"sigs.k8s.io/cluster-api/internal/testtypes"
+	. "sigs.k8s.io/cluster-api/internal/testtypes"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"



Suggested change

fabriziopandini · 2021-09-20T09:32:27Z

controllers/topology/current_state_test.go

@@ -19,11 +19,8 @@ package topology
 import (
 	"testing"

-	"github.com/google/go-cmp/cmp/cmpopts"
+	. "sigs.k8s.io/cluster-api/internal/testtypes"



Suggested change

fabriziopandini · 2021-09-20T09:32:59Z

controllers/topology/reconcile_state_test.go

@@ -21,9 +21,8 @@ import (
 	"regexp"
 	"testing"

-	"sigs.k8s.io/cluster-api/internal/testtypes"
+	. "sigs.k8s.io/cluster-api/internal/testtypes"



Suggested change

fabriziopandini · 2021-09-20T09:36:02Z

controllers/topology/util_test.go

@@ -21,9 +21,8 @@ import (

 	"github.com/pkg/errors"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4"
-	"sigs.k8s.io/cluster-api/internal/testtypes"
+	. "sigs.k8s.io/cluster-api/internal/testtypes"


I'm personally ok with this dot import given that it is our own "additional layer" on top of gomega/testing and it makes tests more readable. However, no problem to accept whatever we agree on.

sbueringer · 2021-09-20T10:01:05Z

internal/testtypes/matchers.go

+)
+
+// This code is a modified version of the mergePatch code from the controllers/topology/internal/mergepatch pkg.
+// TODO: Create a shared patchdiff library that can be shared for both patching and matching.


Replicating my comment from the old PR:

Are we sure we want a separate package for that? Sometimes a little bit copying is better then a new package/dependency

Replicating my response 🙃 (sorry I didn't notice the bad branch name earlier!)

Copying is definitely the right way for now - but having this code in multiple places doesn't seem like the right solution in the long term as its usage might grow - especially if mergepatch goes out of internal. I think having an issue for this is a good idea, and we can decide down the line if we want to actually execute on it.

Sorry looked at the wrong place for the response and didn't see it :/

Might be a good idea to open an issue for it. Not sure how the usual "process" is. TODO in the code or issue or both

I think maybe we leave the TODO out of the code and open an issue - I always feel like a TODO should focus on something more short term, whereas this might not be resolved for a while. An issue (at least with the Kubernetes bots keeping us honest) forces us to make a decision on how to proceed on a regular basis.

I'll definitely open an issue as soon as we've decided the exact form to merge this in - and I'll take out the TODO if noone has any objections.

killianmuldoon · 2021-09-23T14:16:21Z

With the new import style we have builder.NewInfrastructureClusterBuilder() which is repetitive and not as nice as it should be. I think we could transform this (and all the other builders) to builder.InfrastructureCluster and it would read much better - @sbueringer @fabriziopandini WDYT?

killianmuldoon · 2021-09-23T14:41:18Z

I've added a commit with the updated builder names - let me know if it looks like the right approach.

The only issue I see is with blocks like:

CRDs: []apiextensionsv1.CustomResourceDefinition{ *builder.GenericBootstrapConfigCRD.DeepCopy(), *builder.GenericBootstrapConfigTemplateCRD.DeepCopy(), *builder.GenericControlPlaneCRD.DeepCopy(), *builder.GenericControlPlaneTemplateCRD.DeepCopy(), *builder.GenericInfrastructureMachineCRD.DeepCopy(), *builder.GenericInfrastructureMachineTemplateCRD.DeepCopy(), *builder.GenericInfrastructureClusterCRD.DeepCopy(), *builder.GenericInfrastructureClusterTemplateCRD.DeepCopy(), *builder.GenericRemediationCRD.DeepCopy(), *builder.GenericRemediationTemplateCRD.DeepCopy(), },

fabriziopandini · 2021-09-23T16:33:36Z

lgtm apart a small nit.
I like how this is shaping out and I think we can have a follow up iteration on CRDs

sbueringer · 2021-09-23T17:04:32Z

Looks great!
/lgtm
from my side

This commit adds composable matching objects to the internal testtypes package. These types can be used for advanced comparison between objects in the Cluster API unit and integration tests. A simple integration test is included to show the basic working of the matchers.

killianmuldoon · 2021-09-23T17:14:22Z

@fabriziopandini I think I've hit all the nits now and I've squashed the commits.

fabriziopandini · 2021-09-23T17:17:04Z

/lgtm
/approve

k8s-ci-robot · 2021-09-23T17:17:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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:

~~OWNERS~~ [fabriziopandini]

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

sbueringer · 2021-09-23T17:20:04Z

/lgtm

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2021

k8s-ci-robot requested review from enxebre and ykakarap September 17, 2021 14:36

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 17, 2021

killianmuldoon mentioned this pull request Sep 17, 2021

🌱 Add composable matchers to internal/testtypes #5219

Closed

killianmuldoon changed the title ~~Add composable matchers to internal/testtypes~~ 🌱 Add composable matchers to internal/testtypes Sep 17, 2021

killianmuldoon force-pushed the pr-object-matcher branch from cbbc31a to 7d049e7 Compare September 17, 2021 15:38

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2021

sbueringer reviewed Sep 20, 2021

View reviewed changes

fabriziopandini reviewed Sep 20, 2021

View reviewed changes

sbueringer reviewed Sep 20, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 20, 2021

killianmuldoon force-pushed the pr-object-matcher branch from 7d049e7 to a5fa1c9 Compare September 23, 2021 14:13

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2021

killianmuldoon force-pushed the pr-object-matcher branch from a5fa1c9 to 12d7adc Compare September 23, 2021 14:16

k8s-ci-robot assigned sbueringer Sep 23, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 23, 2021

killianmuldoon force-pushed the pr-object-matcher branch from 34f3fd6 to 6ce8913 Compare September 23, 2021 17:13

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 23, 2021

k8s-ci-robot assigned fabriziopandini Sep 23, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 23, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 23, 2021

killianmuldoon mentioned this pull request Sep 23, 2021

Create custom matchers for internal/testtypes #5157

Closed

k8s-ci-robot merged commit 5c7d80d into kubernetes-sigs:main Sep 23, 2021

k8s-ci-robot added this to the v0.4 milestone Sep 23, 2021

This was referenced Sep 24, 2021

Create unified patching/matching library #5316

Closed

Integrate internal/matchers into Komega style test abstraction in Controller Runtime #5338

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Add composable matchers to internal/testtypes #5259

🌱 Add composable matchers to internal/testtypes #5259

killianmuldoon commented Sep 17, 2021

killianmuldoon commented Sep 17, 2021

sbueringer Sep 20, 2021

sbueringer Sep 20, 2021

fabriziopandini Sep 20, 2021

sbueringer Sep 20, 2021

killianmuldoon Sep 20, 2021

sbueringer Sep 20, 2021 •

edited

Loading

fabriziopandini Sep 20, 2021

fabriziopandini Sep 20, 2021

fabriziopandini Sep 20, 2021

fabriziopandini Sep 20, 2021

sbueringer Sep 20, 2021

killianmuldoon Sep 20, 2021

sbueringer Sep 20, 2021

killianmuldoon Sep 20, 2021 •

edited

Loading

killianmuldoon commented Sep 23, 2021 •

edited

Loading

killianmuldoon commented Sep 23, 2021 •

edited

Loading

fabriziopandini commented Sep 23, 2021

sbueringer commented Sep 23, 2021

killianmuldoon commented Sep 23, 2021

fabriziopandini commented Sep 23, 2021

k8s-ci-robot commented Sep 23, 2021

sbueringer commented Sep 23, 2021

🌱 Add composable matchers to internal/testtypes #5259

🌱 Add composable matchers to internal/testtypes #5259

Conversation

killianmuldoon commented Sep 17, 2021

killianmuldoon commented Sep 17, 2021

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

sbueringer Sep 20, 2021 • 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

killianmuldoon Sep 20, 2021 • edited Loading

Choose a reason for hiding this comment

killianmuldoon commented Sep 23, 2021 • edited Loading

killianmuldoon commented Sep 23, 2021 • edited Loading

fabriziopandini commented Sep 23, 2021

sbueringer commented Sep 23, 2021

killianmuldoon commented Sep 23, 2021

fabriziopandini commented Sep 23, 2021

k8s-ci-robot commented Sep 23, 2021

sbueringer commented Sep 23, 2021

sbueringer Sep 20, 2021 •

edited

Loading

killianmuldoon Sep 20, 2021 •

edited

Loading

killianmuldoon commented Sep 23, 2021 •

edited

Loading

killianmuldoon commented Sep 23, 2021 •

edited

Loading