🌱 Add composable matchers to internal/testtypes #5219

killianmuldoon · 2021-09-07T21:33:07Z

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.

Fixes #5157

k8s-ci-robot · 2021-09-07T21:33:17Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign cecilerobertmichon after the PR has been reviewed.
You can assign the PR to them by writing /assign @cecilerobertmichon in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

controllers/topology/current_state_test.go

controllers/topology/controller_test.go

internal/testtypes/builders.go

internal/testtypes/matchers.go

controllers/topology/controller_test.go

killianmuldoon · 2021-09-15T21:39:40Z

@fabriziopandini this is now reworked and ready for review.

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.

k8s-ci-robot · 2021-09-16T10:38:04Z

@killianmuldoon: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

fabriziopandini · 2021-09-16T12:43:50Z

controllers/topology/blueprint_test.go

-				cmp.Diff(tt.want.MachineDeployments, got.MachineDeployments, ignoreResourceVersion))
+			// Use EqualObject where an object is created and passed through the fake client. Elsewhere the Equal methoc
+			// is enough to establish inequality.
+			g.Expect(tt.want.ClusterClass).To(EqualObject(got.ClusterClass, IgnoreAutogeneratedMetadata))


Suggested change

g.Expect(tt.want.ClusterClass).To(EqualObject(got.ClusterClass, IgnoreAutogeneratedMetadata))

g.Expect(tt.want.ClusterClass).To(EqualObject(got.ClusterClass, IgnoreAutogeneratedMetadata))

I like this! super clean

fabriziopandini · 2021-09-16T12:46:48Z

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.


Let's open an issue for this

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

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.

fabriziopandini · 2021-09-16T12:48:20Z

internal/testtypes/matchers.go

+
+// NegatedFailureMessage returns a string comparing the full objects after an unexpected match has occurred.
+func (m *Matcher) NegatedFailureMessage(other interface{}) (message string) {
+	return format.Message(other, "not to match", m.original)


Let's return diff also here

There shouldn't be a diff here though right? This will print only when the objects are expected not to match but do so unexpectedly. I can add the diff for now - it will just be an empty string if there's nothing in there.

fabriziopandini · 2021-09-16T13:04:07Z

lgtm apart two nits and rebase

sbueringer

looks very nice!

sbueringer · 2021-09-16T16:02:19Z

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 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.

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.

I'll move it to the new PR

sbueringer · 2021-09-16T16:09:23Z

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.


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

sbueringer · 2021-09-16T16:13:10Z

internal/testtypes/matchers.go

+// These package variables hold pre-created commonly used options that can be used to reduce the manual work involved in
+// identifying the paths that need to be compared for testing equality between objects.
+var (
+


Suggested change

nit

sbueringer · 2021-09-16T16:15:39Z

internal/testtypes/matchers.go

+		{"metadata", "managedFields"},
+		{"metadata", "deletionGracePeriodSeconds"},
+		{"metadata", "deletionTimestamp"},
+		{"metadata", "ownerReferences"},


Do we currently need to ignore ownerReferences? If not, it might be good to remove them from the list. When they are set, they are set intentionally by our code. (afaik)

Not needed and you're right that we do set them. I'll remove it.

sbueringer · 2021-09-16T16:18:05Z

internal/testtypes/matchers.go

+	options *MatchOptions
+}
+
+// EqualObject applies the MatchOptions and returns a Matcher for the passed Kubernetes runtime.Object. This function


I'm not sure I would include applies the MatchOptions it almost sounds like we apply them to the object. I probably would just omit it.

sbueringer · 2021-09-16T16:19:48Z

internal/testtypes/matchers.go

+	}
+}
+
+// Match compares the object it's based on to the passed object and returns true if the objects are the same according to


Suggested change

// Match compares the object it's based on to the passed object and returns true if the objects are the same according to

// Match compares the current object to the passed object and returns true if the objects are the same according to

nit

sbueringer · 2021-09-16T16:21:07Z

internal/testtypes/matchers.go

+		return true, nil
+	}
+	if otherIsNil || originalIsNil {
+		return false, fmt.Errorf("can not compare an object with a nil. original :%v , other %v", m.original, other)


Suggested change

return false, fmt.Errorf("can not compare an object with a nil. original :%v , other %v", m.original, other)

return false, fmt.Errorf("can not compare an object with a nil. original %v , other %v", m.original, other)

nit (I assume we want : either for both or for none of the objects)

sbueringer · 2021-09-16T16:23:44Z

internal/testtypes/matchers.go

+
+// Match compares the object it's based on to the passed object and returns true if the objects are the same according to
+// the Matcher MatchOptions.
+func (m *Matcher) Match(other interface{}) (success bool, err error) {


Suggested change

func (m *Matcher) Match(other interface{}) (success bool, err error) {

func (m *Matcher) Match(actual interface{}) (success bool, err error) {

I looked at a gomega matcher and this seems to be called actual there. I think it would be nice to do the same in our code. (same for other funcs)

sbueringer · 2021-09-16T16:27:35Z

internal/testtypes/matchers.go

+	return format.Message(other, "not to match", m.original)
+}
+
+// calculateDiff applies the MatchOptions and identified the diff between the Matcher object and the passed object.


Suggested change

// calculateDiff applies the MatchOptions and identified the diff between the Matcher object and the passed object.

// calculateDiff applies the MatchOptions and identifies the diff between the Matcher object and the passed object.

nit, (or maybe calculates)

sbueringer · 2021-09-16T16:29:20Z

internal/testtypes/matchers.go

+}
+
+// filterDiff limits the diff to allowPaths if given and excludes ignorePaths if given. It returns the altered diff.
+func filterDiff(diff []byte, allowedPaths, ignorePaths [][]string) ([]byte, error) {


Suggested change

func filterDiff(diff []byte, allowedPaths, ignorePaths [][]string) ([]byte, error) {

func filterDiff(diff []byte, allowPaths, ignorePaths [][]string) ([]byte, error) {

nit (to keep it consistent with the field name)

JoelSpeed · 2021-09-17T08:53:16Z

Found a typo while having a read through, otherwise looks great

killianmuldoon · 2021-09-17T14:48:26Z

@JoelSpeed did you mark the typo in your review? I can't seem to see it

JoelSpeed · 2021-09-17T08:49:20Z

controllers/topology/blueprint_test.go

-				cmp.Diff(tt.want.ControlPlane, got.ControlPlane, ignoreResourceVersion))
-			g.Expect(cmp.Diff(tt.want.MachineDeployments, got.MachineDeployments, ignoreResourceVersion)).To(Equal(""),
-				cmp.Diff(tt.want.MachineDeployments, got.MachineDeployments, ignoreResourceVersion))
+			// Use EqualObject where an object is created and passed through the fake client. Elsewhere the Equal methoc


Suggested change

// Use EqualObject where an object is created and passed through the fake client. Elsewhere the Equal methoc

// Use EqualObject where an object is created and passed through the fake client. Elsewhere the Equal method

JoelSpeed · 2021-09-17T14:49:39Z

@JoelSpeed did you mark the typo in your review? I can't seem to see it

I can't press the submit button very well clearly 😓

killianmuldoon · 2021-09-17T14:51:05Z

I can't press the submit button very well clearly 😓

Happens to us all!

killianmuldoon · 2021-09-17T14:51:52Z

Note: This PR has been closed in favor of #5259 which has an updated, more appropriate, branch name given the changes in this PR.

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

@killianmuldoon Are you referring to the "title" (aka 🌱 Add composable matchers to internal/testtypes)? (just confused because it seems to be the same).

If yes, you can just edit it.

EDIT: Okay I saw in your other PR that you're referring to the branch name.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 7, 2021

k8s-ci-robot requested review from enxebre and JoelSpeed September 7, 2021 21:33

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 7, 2021

killianmuldoon force-pushed the pr-integration-test branch 4 times, most recently from dab9113 to 4371995 Compare September 7, 2021 22:12

fabriziopandini reviewed Sep 8, 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 15, 2021

killianmuldoon force-pushed the pr-integration-test branch 2 times, most recently from 3669a32 to 0fc889e Compare September 15, 2021 21:39

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

killianmuldoon force-pushed the pr-integration-test branch from 0fc889e to 9671b1d Compare September 15, 2021 21:51

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

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

fabriziopandini reviewed Sep 16, 2021

View reviewed changes

sbueringer reviewed Sep 16, 2021

View reviewed changes

killianmuldoon closed this Sep 17, 2021

killianmuldoon deleted the pr-integration-test branch September 17, 2021 14:33

killianmuldoon restored the pr-integration-test branch September 17, 2021 14:34

JoelSpeed reviewed Sep 17, 2021

View reviewed changes

	g.Expect(tt.want.ClusterClass).To(EqualObject(got.ClusterClass, IgnoreAutogeneratedMetadata))
	g.Expect(tt.want.ClusterClass).To(EqualObject(got.ClusterClass, IgnoreAutogeneratedMetadata))

	// Match compares the object it's based on to the passed object and returns true if the objects are the same according to
	// Match compares the current object to the passed object and returns true if the objects are the same according to

	return false, fmt.Errorf("can not compare an object with a nil. original :%v , other %v", m.original, other)
	return false, fmt.Errorf("can not compare an object with a nil. original %v , other %v", m.original, other)

	func (m *Matcher) Match(other interface{}) (success bool, err error) {
	func (m *Matcher) Match(actual interface{}) (success bool, err error) {

	// calculateDiff applies the MatchOptions and identified the diff between the Matcher object and the passed object.
	// calculateDiff applies the MatchOptions and identifies the diff between the Matcher object and the passed object.

	func filterDiff(diff []byte, allowedPaths, ignorePaths [][]string) ([]byte, error) {
	func filterDiff(diff []byte, allowPaths, ignorePaths [][]string) ([]byte, error) {

	// Use EqualObject where an object is created and passed through the fake client. Elsewhere the Equal methoc
	// Use EqualObject where an object is created and passed through the fake client. Elsewhere the Equal method

🌱 Add composable matchers to internal/testtypes #5219

🌱 Add composable matchers to internal/testtypes #5219

Uh oh!

Conversation

killianmuldoon commented Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Sep 7, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

killianmuldoon commented Sep 15, 2021

Uh oh!

k8s-ci-robot commented Sep 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabriziopandini commented Sep 16, 2021

Uh oh!

sbueringer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented Sep 17, 2021

Uh oh!

killianmuldoon commented Sep 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented Sep 17, 2021

Uh oh!

killianmuldoon commented Sep 17, 2021

Uh oh!

killianmuldoon commented Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbueringer commented Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

killianmuldoon commented Sep 7, 2021 •

edited

Loading

killianmuldoon commented Sep 17, 2021 •

edited

Loading

sbueringer commented Sep 20, 2021 •

edited

Loading