Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Make client.Patch implementations gomock-friendly #1439

Closed
wants to merge 1 commit into from
Closed

✨ Make client.Patch implementations gomock-friendly #1439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions pkg/client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package client_test
import (
"context"
"fmt"
"reflect"
"sync/atomic"
"time"

Expand Down Expand Up @@ -3302,6 +3303,12 @@ var _ = Describe("Patch", func() {
By("returning a patch with data containing the annotation change and the resourceVersion change")
Expect(data).To(Equal([]byte(fmt.Sprintf(`{"metadata":{"annotations":{"%s":"%s"},"resourceVersion":"%s"}}`, annotationKey, annotationValue, cm.ResourceVersion))))
})

It("deeply equals to the same patch", func() {
patch1 := client.MergeFrom(cm)
patch2 := client.MergeFrom(cm)
Expect(reflect.DeepEqual(patch1, patch2)).To(BeTrue())
})
})

Describe("StrategicMergeFrom", func() {
Expand Down Expand Up @@ -3380,6 +3387,12 @@ var _ = Describe("Patch", func() {
`"spec":{"template":{"spec":{"$setElementOrder/containers":[{"name":"main"},{"name":"sidecar"}],"containers":[{"image":"foo:v2","name":"main"}]}}}}`,
dep.ResourceVersion))))
})

It("deeply equals to the same patch", func() {
patch1 := client.StrategicMergeFrom(dep)
patch2 := client.StrategicMergeFrom(dep)
Expect(reflect.DeepEqual(patch1, patch2)).To(BeTrue())
})
})
})

Expand Down
28 changes: 18 additions & 10 deletions pkg/client/patch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,15 @@ type MergeFromOptions struct {
OptimisticLock bool
}

type patchHandler interface {
createPatch(originalJSON, modifiedJSON []byte, dataStruct interface{}) ([]byte, error)
}

type mergeFromPatch struct {
ialidzhikov marked this conversation as resolved.
Show resolved Hide resolved
patchType types.PatchType
createPatch func(originalJSON, modifiedJSON []byte, dataStruct interface{}) ([]byte, error)
from Object
opts MergeFromOptions
patchType types.PatchType
patchHandler patchHandler
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the definition of "gomock friendly" is "can not use function types"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally speaking, everything that is unexported is considered to be an implementation detail consumers can not rely on. This is for example why cmp will refuse to compare private types, see https://github.com/google/go-cmp third bullet point

Copy link
Contributor Author

@ialidzhikov ialidzhikov Mar 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I agree that it is an implementation details.
On the other side this PR tries to improve the user experience for users that consume the controller-runtime client pkg - in more details it improves the readability of unit tests with controller-runtime client that use gomock (the most popular mocking library in the golang ecosystem).

This is how an expectation for a Patch call with gomock looks like with the current master

	expectedPatch := client.MergeFrom(mergeFrom)
	expectedData, expectedErr := expectedPatch.Data(obj)
	Expect(expectedErr).To(BeNil())

	c.EXPECT().Patch(ctx, obj, gomock.Any()).DoAndReturn(func(_ context.Context, _ client.Object, patch client.Patch, _ ...client.PatchOption) error {
		data, err := patch.Data(obj)
		Expect(err).To(BeNil())
		Expect(patch.Type()).To(Equal(expectedPatch.Type()))
		Expect(data).To(Equal(expectedData))
		return nil
	})

This is how it would look like with this PR:

	 c.EXPECT().Patch(ctx, obj, client.MergeFrom(mergeFrom))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How exactly do you assess in the second example that the patch type is correct, that the data is correct and that it won't error if patch.Data is called?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why I have to asses this when the given client.Patch arg equals to the expected one? In my unit test I would be only interested if the client.Client is called with the expected args (expected obj, expected patch, etc). The fact that Patch func of client.Client internally is using the Data func of the passed patch arg is not something that I should verify in my unit test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because you are looking at how the Patch looks, not at how it behaves even though you only care about the latter (and we export the latter and not the former). If we were to introduce a bug that made a patch always return no data, your tests would happily pass but your product would be broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but I think this is the fundamental principle of the unit testing. When you write a unit test, you are only interested in the functional correctness of your unit (your type). The functional correctness of the external types you use should be ensured with (unit) tests in the corresponding library/pkg.
That's why we have mocks and spies that help us to isolate the unit under test (our type) from all dependant types (external ones). When you start testing also the dependent types, this is no longer a unit test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit tests just means that you are testing one isolated unit (here: That Patch() is called with the correct value). You are testing for "correct value" by comparing two unexported struct instances. This is just invalid and has nothing to do with unit testing or not. The things you as a user of controller-runtime are allowed to make any assumptions about are exported.
Please feel free to bring this up in the next kubebuilder and controller-runtime meeting if you disagree.
/hold

from Object
opts MergeFromOptions
}

// Type implements Patch.
Expand Down Expand Up @@ -124,19 +128,23 @@ func (s *mergeFromPatch) Data(obj Object) ([]byte, error) {
return nil, err
}

data, err := s.createPatch(originalJSON, modifiedJSON, obj)
data, err := s.patchHandler.createPatch(originalJSON, modifiedJSON, obj)
if err != nil {
return nil, err
}

return data, nil
}

func createMergePatch(originalJSON, modifiedJSON []byte, _ interface{}) ([]byte, error) {
type mergePatchHandler struct{}

func (mergePatchHandler) createPatch(originalJSON, modifiedJSON []byte, dataStruct interface{}) ([]byte, error) {
return jsonpatch.CreateMergePatch(originalJSON, modifiedJSON)
}

func createStrategicMergePatch(originalJSON, modifiedJSON []byte, dataStruct interface{}) ([]byte, error) {
type strategicMergePatchHandler struct{}

func (strategicMergePatchHandler) createPatch(originalJSON, modifiedJSON []byte, dataStruct interface{}) ([]byte, error) {
return strategicpatch.CreateTwoWayMergePatch(originalJSON, modifiedJSON, dataStruct)
}

Expand All @@ -148,7 +156,7 @@ func createStrategicMergePatch(originalJSON, modifiedJSON []byte, dataStruct int
// See https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/ for more details on
// the difference between merge-patch and strategic-merge-patch.
func MergeFrom(obj Object) Patch {
return &mergeFromPatch{patchType: types.MergePatchType, createPatch: createMergePatch, from: obj}
return &mergeFromPatch{patchType: types.MergePatchType, patchHandler: mergePatchHandler{}, from: obj}
}

// MergeFromWithOptions creates a Patch that patches using the merge-patch strategy with the given object as base.
Expand All @@ -158,7 +166,7 @@ func MergeFromWithOptions(obj Object, opts ...MergeFromOption) Patch {
for _, opt := range opts {
opt.ApplyToMergeFrom(options)
}
return &mergeFromPatch{patchType: types.MergePatchType, createPatch: createMergePatch, from: obj, opts: *options}
return &mergeFromPatch{patchType: types.MergePatchType, patchHandler: mergePatchHandler{}, from: obj, opts: *options}
}

// StrategicMergeFrom creates a Patch that patches using the strategic-merge-patch strategy with the given object as base.
Expand All @@ -175,7 +183,7 @@ func StrategicMergeFrom(obj Object, opts ...MergeFromOption) Patch {
for _, opt := range opts {
opt.ApplyToMergeFrom(options)
}
return &mergeFromPatch{patchType: types.StrategicMergePatchType, createPatch: createStrategicMergePatch, from: obj, opts: *options}
return &mergeFromPatch{patchType: types.StrategicMergePatchType, patchHandler: strategicMergePatchHandler{}, from: obj, opts: *options}
}

// mergePatch uses a raw merge strategy to patch the object.
Expand Down