support proto.Message in Eq()

[ebilling]

Actual behavior
Protobufs generated using google.golang.org/protobuf/cmd/protoc-gen-go are not being reliably matched by the gomock Matcher.

Expected behavior
The default matcher for gomock should use the proto.Equal() function when a protobuf is passed as an argument.

To Reproduce Steps to reproduce the behavior

1. Generate protobufs with latest protoc-gen-go from google.golang.org/protobuf/cmd/protoc-gen-go. It create a mutex in the protobuf which is likely causing the problem.
2. Create two separate protobufs of equal content as the proto being matched and the proto being sent.
3. Pass one of the protos to the command being mocked and the other as the proto to match against in the gomock EXPECT.

Additional Information

• gomock mode (reflect or source): source
• gomock version or git ref: aff3767
• golang version: go1.15
• proton-gen-go version: v1.22.0

Triage Notes for the Maintainers

For my personal use, I've created a proto matcher.

import "github.com/golang/protobuf/proto"

// ProtoMatcher is used along with gomock.EXPECT() statements. Protobufs can sometimes fail to match with a standard matcher
// ex: myService.EXPECT().Command(gomock.Any(), NewProtoMatcher(request)).Return(response, nil).AnyTimes()
type ProtoMatcher struct {
        proto.Message
}

// NewProtoMatcher returns a matcher suitable for use in gomock.Select()
// ex: mockDBService.EXPECT().Select(gomock.Any(), NewProtoMatcher(onboardResourcesRequest)).Return(selectOnboardResourcesResponse, nil).AnyTimes()
func NewProtoMatcher(p proto.Message) *ProtoMatcher {
        return &ProtoMatcher{
                Message: p,
        }
}

// Matches returns true if v is a protobuf and the contents match the one in the ProtoMatcher. Returns false if v is not a proto.Message
func (p ProtoMatcher) Matches(v interface{}) bool {
        vp, ok := v.(proto.Message)
        if !ok {
                return false
        }
        return proto.Equal(vp, p.Message)
}

[codyoss]

I get the frustration with proto messages, and you are right to use proto.Equal. I don't think it is as simple as type checking for a proto.Message though. For instance the following would not be caught

type MyType struct {
   ProtoMessage proto.Message
}

We could perhaps support just a top level check, doing a bunch of type un-wrapping though sounds like it could get messy.

@cvgw Thoughts?

[cvgw]

@codyoss I would prefer not to add type specific behavior for proto.Message (or any other user/community defined type)

gomock.Eq ends up calling reflect.DeepEqual which I think is totally fair and appropriate for a general equivalency test.

Protos are clearly a popular and common case, but I see no reason why they couldn't fade from popularity and be replaced by something else.

I think a user-implemented custom matcher is the way to go here.

If there is some extension or change to the API that would help make that easier lets definitely consider that.

[codyoss]

Closing. Recommend approach for this is to use a custom matcher.

[ebilling]

Like I said, I built a matcher for myself. The problem I'm running into is that DeepEqual on a protobuf is causing infinite loops (but not reliably). I was looking for a way for my fellow engineers to not stumble into this situation.

Happy to contribute the above code if that's of interest or you have a good spot to put it.

[cvgw]

Like I said, I built a matcher for myself. The problem I'm running into is that DeepEqual on a protobuf is causing infinite loops (but not reliably). I was looking for a way for my fellow engineers to not stumble into this situation.

Happy to contribute the above code if that's of interest or you have a good spot to put it.

@ebilling
I missed the part about it causing infinite loops, can you share more details about that?

I think we probably want to keep type specific matchers out of the core library, but it would be fairly straightforward to host on Github as it's own module if you wanted to share it with other people.

[adiedjx]

Can someone help me with an example to use custom matcher with proto, from what i understand i would need to my extend /(use composition) the proto struct to implement matcher interface, however my test functions expect proto message and not the extended struct.