client.Patch implementations are no longer gomock-friendly

[ialidzhikov]

After #1406, the client.Patch implementations are no longer gomock-friendly.

So we have unit tests that use gomock. Let's have the following example to get a better understanding.

• pkg/utils.go

package utils

import (
	"context"

	"sigs.k8s.io/controller-runtime/pkg/client"
)

func RemoveAnnotation(ctx context.Context, c client.Client, obj client.Object, annotation string) error {
	withAnnotation := obj.DeepCopyObject()

	annotations := obj.GetAnnotations()
	delete(annotations, annotation)
	obj.SetAnnotations(annotations)

	return c.Patch(ctx, obj, client.MergeFrom(withAnnotation))
}

• pkg/utils_test.go

package utils_test

import (
	"context"
	"testing"

	"github.com/ialidzhikov/example/pkg/utils"
	
	"github.com/golang/mock/gomock"
	. "github.com/onsi/ginkgo"
	. "github.com/onsi/gomega"
	"sigs.k8s.io/controller-runtime/pkg/client"
	corev1 "k8s.io/api/core/v1"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

func TestUtils(t *testing.T) {
	RegisterFailHandler(Fail)
	RunSpecs(t, "Utils Suite")
}

var _ = Describe("Utils", func() {

	var (
		ctrl *gomock.Controller
		c    *utils.MockClient
	)

	BeforeEach(func() {
		ctrl = gomock.NewController(GinkgoT())
		c = utils.NewMockClient(ctrl)
	})

	AfterEach(func() {
		ctrl.Finish()
	})

	Describe("#RemoveAnnotation", func() {
		It("should delete specific annotation", func() {
			obj := &corev1.Secret{
				ObjectMeta: metav1.ObjectMeta{
					Name:        "test-name",
					Namespace:   "test-namespace",
					Annotations: map[string]string{
						"foo": "bar",
						"bar": "baz",
					},
				},
			}
			objWithAnnotation := obj.DeepCopy()
			objWithoutAnnotation := &corev1.Secret{
				ObjectMeta: metav1.ObjectMeta{
					Name:        "test-name",
					Namespace:   "test-namespace",
					Annotations: map[string]string{
						"bar": "baz",
					},
				},
			}

			ctx := context.TODO()
			c.EXPECT().Patch(ctx, objWithoutAnnotation, client.MergeFrom(objWithAnnotation))

			err := utils.RemoveAnnotation(ctx, c, obj, "foo")
			Expect(err).NotTo(HaveOccurred())
		})
	})
})

Note: MockClient is a mock generated with gomock

With sigs.k8s.io/controller-runtime < v0.8.3 this test is working perfectly fine.

$ go test -mod=vendor -timeout 30s -run ^TestUtils$ github.com/ialidzhikov/example/pkg/utils
ok  	github.com/ialidzhikov/example/pkg/utils	0.787s

#1406 (also cherry-picked into release-0.8 branch) introduces a field of type func (createPatch ) in the mergeFromPatch struct. This breaks the naive gomock expectations. The reason is that the
mergeFromPatch struct has now a function attribute (createPatch), and as gomock is using reflect.DeepEqual under the hood, the comparison now fails:

Func values are deeply equal if both are nil; otherwise they are not deeply equal.
(from https://golang.org/pkg/reflect/#DeepEqual)

Example of such failure with sigs.k8s.io/[email protected]:

$ go test -mod=vendor -timeout 30s -run ^TestUtils$ github.com/ialidzhikov/example/pkg/utils
Running Suite: Utils Suite
==========================
Random Seed: 1616082953
Will run 1 of 1 specs

• Failure [0.001 seconds]
Utils
/git/example/pkg/utils/utils_test.go:23
  #RemoveAnnotation
  /git/example/pkg/utils/utils_test.go:39
    should delete specific annotation [It]
    /git/example/pkg/utils/utils_test.go:40

    Unexpected call to *utils.MockClient.Patch([context.TODO &Secret{ObjectMeta:{test-name  test-namespace    0 0001-01-01 00:00:00 +0000 UTC <nil> <nil> map[] map[bar:baz] [] []  []},Data:map[string][]byte{},Type:,StringData:map[string]string{},Immutable:nil,} 0xc000407a00]) at /git/example/pkg/utils/mocks.go:138 because:
    expected call at /git/example/pkg/utils/utils_test.go:63 doesn't match the argument at index 2.
    Got: &{application/merge-patch+json 0x1dbea60 0xc00042f680 {false}}
    Want: is equal to &{application/merge-patch+json 0x1dbea60 0xc00042f400 {false}}

    /git/example/pkg/utils/mocks.go:138
------------------------------


Summarizing 1 Failure:

[Fail] Utils #RemoveAnnotation [It] should delete specific annotation
/git/example/pkg/utils/mocks.go:138

Ran 1 of 1 Specs in 0.001 seconds
FAIL! -- 0 Passed | 1 Failed | 0 Pending | 0 Skipped
--- FAIL: TestUtils (0.00s)
FAIL
FAIL	github.com/ialidzhikov/example/pkg/utils	0.671s
FAIL

[alvaroaleman]

I have a hard time understanding what exactly go mock is doing here and why. Why is it looking at the private properties of the patch implementation rather than at its exported methods and the result of calling them?

None of our api compatibility testing checks compatibility of unexported types. If there is any expectation around compatibility of those, it needs to be agreed upon, formalized and verified in CI.

I can already tell you though that I am not a fan doing any of that. The very reason for exporting some things and not exporting others is to convey the message if a compatibility promise is given or not.

/cc @DirectXMan12 @vincepri @estroz

[ialidzhikov]

/close

[k8s-ci-robot]

@ialidzhikov: Closing this issue.

In response to this:

/close

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.