-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
✨ Make client.Patch implementations gomock-friendly #1439
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ialidzhikov The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @ialidzhikov. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
5ac6fa4
to
f8d053d
Compare
from Object | ||
opts MergeFromOptions | ||
patchType types.PatchType | ||
patchHandler patchHandler |
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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))
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Signed-off-by: ialidzhikov <i.alidjikov@gmail.com>
f8d053d
to
97b9ad3
Compare
/close |
@ialidzhikov: Closed this PR. In response to this:
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. |
Fixes #1438