From 6e34dff0614cfaa4468551c28f282a46f36d884b Mon Sep 17 00:00:00 2001 From: Spencer Connaughton Date: Sun, 22 May 2022 21:39:19 -0400 Subject: [PATCH] Adopt cmp.Diff for showing unmatched arguments. --- go.mod | 1 + go.sum | 4 ++ gomock/call.go | 31 +++++++++++-- gomock/callset_test.go | 8 ++-- gomock/controller.go | 19 +++++++- gomock/controller_test.go | 21 ++++++--- gomock/matchers.go | 91 +++++++++++++++++++++++++++++++++++++++ 7 files changed, 159 insertions(+), 16 deletions(-) diff --git a/go.mod b/go.mod index 4db0ebd..5c60552 100644 --- a/go.mod +++ b/go.mod @@ -3,6 +3,7 @@ module go.uber.org/mock go 1.20 require ( + github.com/google/go-cmp v0.6.0 golang.org/x/mod v0.11.0 golang.org/x/tools v0.2.0 ) diff --git a/go.sum b/go.sum index 38c7646..967ef2f 100644 --- a/go.sum +++ b/go.sum @@ -1,3 +1,7 @@ +github.com/google/go-cmp v0.5.8 h1:e6P7q2lk1O+qJJb4BtCQXlK8vWEO8V1ZeuEdJNOqZyg= +github.com/google/go-cmp v0.5.8/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= +github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= github.com/yuin/goldmark v1.4.13 h1:fVcFKWvrslecOb/tg+Cc05dkeYx540o0FuFt3nUVDoE= github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= golang.org/x/mod v0.11.0 h1:bUO06HqtnRcc/7l71XBe4WcqTZ+3AH1J59zWDDwLKgU= diff --git a/gomock/call.go b/gomock/call.go index e1ea826..0ba16ac 100644 --- a/gomock/call.go +++ b/gomock/call.go @@ -19,6 +19,8 @@ import ( "reflect" "strconv" "strings" + + "github.com/google/go-cmp/cmp" ) // Call represents an expected call to a mock. @@ -42,11 +44,13 @@ type Call struct { // can set the return values by returning a non-nil slice. Actions run in the // order they are created. actions []func([]any) []any + + cmpOpts cmp.Options // comparison options } // newCall creates a *Call. It requires the method type in order to support // unexported methods. -func newCall(t TestHelper, receiver any, method string, methodType reflect.Type, args ...any) *Call { +func newCall(t TestHelper, receiver any, method string, methodType reflect.Type, cmpOpts cmp.Options, args ...any) *Call { t.Helper() // TODO: check arity, types. @@ -76,7 +80,8 @@ func newCall(t TestHelper, receiver any, method string, methodType reflect.Type, return rets }} return &Call{t: t, receiver: receiver, method: method, methodType: methodType, - args: mArgs, origin: origin, minCalls: 1, maxCalls: 1, actions: actions} + args: mArgs, origin: origin, minCalls: 1, maxCalls: 1, actions: actions, + cmpOpts: cmpOpts} } // AnyTimes allows the expectation to be called 0 or more times @@ -331,10 +336,28 @@ func (c *Call) matches(args []any) error { } for i, m := range c.args { - if !m.Matches(args[i]) { + arg := args[i] + if !m.Matches(arg) { + if g, ok := m.(GotFormatter); ok { + return fmt.Errorf( + "expected call at %s doesn't match the argument at index %d.\nGot: %v\nWant: %v", + c.origin, i, g.Got(arg), m, + ) + } + if d, ok := m.(Differ); ok { + diff := d.Diff(arg, c.cmpOpts...) + // Recover if the diff is empty, implying the match failed on ignored fields. + if diff == "" { + return nil + } + return fmt.Errorf( + "expected call at %s doesn't match the argument at index %d.\nDiff (-want +got): %s", + c.origin, i, diff, + ) + } return fmt.Errorf( "expected call at %s doesn't match the argument at index %d.\nGot: %v\nWant: %v", - c.origin, i, formatGottenArg(m, args[i]), m, + c.origin, i, formatGottenArg(m, arg), m, ) } } diff --git a/gomock/callset_test.go b/gomock/callset_test.go index d8150c5..6f07518 100644 --- a/gomock/callset_test.go +++ b/gomock/callset_test.go @@ -30,7 +30,7 @@ func TestCallSetAdd(t *testing.T) { numCalls := 10 for i := 0; i < numCalls; i++ { - cs.Add(newCall(t, receiver, method, reflect.TypeOf(receiverType{}.Func))) + cs.Add(newCall(t, receiver, method, reflect.TypeOf(receiverType{}.Func), nil)) } call, err := cs.FindMatch(receiver, method, []any{}) @@ -47,13 +47,13 @@ func TestCallSetAdd_WhenOverridable_ClearsPreviousExpectedAndExhausted(t *testin var receiver any = "TestReceiver" cs := newOverridableCallSet() - cs.Add(newCall(t, receiver, method, reflect.TypeOf(receiverType{}.Func))) + cs.Add(newCall(t, receiver, method, reflect.TypeOf(receiverType{}.Func), nil)) numExpectedCalls := len(cs.expected[callSetKey{receiver, method}]) if numExpectedCalls != 1 { t.Fatalf("Expected 1 expected call in callset, got %d", numExpectedCalls) } - cs.Add(newCall(t, receiver, method, reflect.TypeOf(receiverType{}.Func))) + cs.Add(newCall(t, receiver, method, reflect.TypeOf(receiverType{}.Func), nil)) newNumExpectedCalls := len(cs.expected[callSetKey{receiver, method}]) if newNumExpectedCalls != 1 { t.Fatalf("Expected 1 expected call in callset, got %d", newNumExpectedCalls) @@ -100,7 +100,7 @@ func TestCallSetFindMatch(t *testing.T) { method := "TestMethod" args := []any{} - c1 := newCall(t, receiver, method, reflect.TypeOf(receiverType{}.Func)) + c1 := newCall(t, receiver, method, reflect.TypeOf(receiverType{}.Func), nil) cs.exhausted = map[callSetKey][]*Call{ {receiver: receiver, fname: method}: {c1}, } diff --git a/gomock/controller.go b/gomock/controller.go index 40bcdf8..b785f98 100644 --- a/gomock/controller.go +++ b/gomock/controller.go @@ -20,6 +20,8 @@ import ( "reflect" "runtime" "sync" + + "github.com/google/go-cmp/cmp" ) // A TestReporter is something that can be used to report test failures. It @@ -76,6 +78,7 @@ type Controller struct { mu sync.Mutex expectedCalls *callSet finished bool + cmpOpts cmp.Options } // NewController returns a new Controller. It is the preferred way to create a Controller. @@ -121,6 +124,20 @@ func (o overridableExpectationsOption) apply(ctrl *Controller) { ctrl.expectedCalls = newOverridableCallSet() } +type cmpOptions struct { + opts []cmp.Option +} + +func (o cmpOptions) apply(ctrl *Controller) { + ctrl.cmpOpts = o.opts +} + +// WithCmpOpts is a ControllerOption that configures the options to pass to +// cmp.Diff. +func WithCmpOpts(opts ...cmp.Option) cmpOptions { + return cmpOptions{opts: opts} +} + type cancelReporter struct { t TestHelper cancel func() @@ -181,7 +198,7 @@ func (ctrl *Controller) RecordCall(receiver any, method string, args ...any) *Ca func (ctrl *Controller) RecordCallWithMethodType(receiver any, method string, methodType reflect.Type, args ...any) *Call { ctrl.T.Helper() - call := newCall(ctrl.T, receiver, method, methodType, args...) + call := newCall(ctrl.T, receiver, method, methodType, ctrl.cmpOpts, args...) ctrl.mu.Lock() defer ctrl.mu.Unlock() diff --git a/gomock/controller_test.go b/gomock/controller_test.go index 03d9e64..0ed6e89 100644 --- a/gomock/controller_test.go +++ b/gomock/controller_test.go @@ -21,6 +21,7 @@ import ( "testing" "go.uber.org/mock/gomock" + "github.com/google/go-cmp/cmp/cmpopts" ) type ErrorReporter struct { @@ -74,8 +75,11 @@ func (e *ErrorReporter) assertFatal(fn func(), expectedErrMsgs ...string) { // check the last actualErrMsg, because the previous messages come from previous errors actualErrMsg := e.log[len(e.log)-1] for _, expectedErrMsg := range expectedErrMsgs { - if !strings.Contains(actualErrMsg, expectedErrMsg) { + i := strings.Index(actualErrMsg, expectedErrMsg) + if i == -1 { e.t.Errorf("Error message:\ngot: %q\nwant to contain: %q\n", actualErrMsg, expectedErrMsg) + } else { + actualErrMsg = actualErrMsg[i+len(expectedErrMsg):] } } } @@ -149,8 +153,9 @@ func (s *Subject) VariadicMethod(arg int, vararg ...string) {} // A type purely for ActOnTestStructMethod type TestStruct struct { - Number int - Message string + Number int + Message string + secretMessage string } func (s *Subject) ActOnTestStructMethod(arg TestStruct, arg1 int) int { @@ -171,7 +176,9 @@ func createFixtures(t *testing.T) (reporter *ErrorReporter, ctrl *gomock.Control // Controller. We use it to test that the mock considered tests // successful or failed. reporter = NewErrorReporter(t) - ctrl = gomock.NewController(reporter) + ctrl = gomock.NewController( + reporter, gomock.WithCmpOpts(cmpopts.IgnoreUnexported(TestStruct{})), + ) return } @@ -298,13 +305,13 @@ func TestUnexpectedArgValue_FirstArg(t *testing.T) { // the method argument (of TestStruct type) has 1 unexpected value (for the Message field) ctrl.Call(subject, "ActOnTestStructMethod", TestStruct{Number: 123, Message: "no message"}, 15) }, "Unexpected call to", "doesn't match the argument at index 0", - "Got: {123 no message} (gomock_test.TestStruct)\nWant: is equal to {123 hello %s} (gomock_test.TestStruct)") + "Diff (-want +got):", "gomock_test.TestStruct{", "Number: 123", "-", "Message: \"hello %s\",", "+", "Message: \"no message\",", "}") reporter.assertFatal(func() { // the method argument (of TestStruct type) has 2 unexpected values (for both fields) ctrl.Call(subject, "ActOnTestStructMethod", TestStruct{Number: 11, Message: "no message"}, 15) }, "Unexpected call to", "doesn't match the argument at index 0", - "Got: {11 no message} (gomock_test.TestStruct)\nWant: is equal to {123 hello %s} (gomock_test.TestStruct)") + "Diff (-want +got):", "gomock_test.TestStruct{", "-", "Number: 123,", "+", "Number: 11,", "-", "Message: \"hello %s\",", "+", "Message: \"no message\",", "}") reporter.assertFatal(func() { // The expected call wasn't made. @@ -323,7 +330,7 @@ func TestUnexpectedArgValue_SecondArg(t *testing.T) { reporter.assertFatal(func() { ctrl.Call(subject, "ActOnTestStructMethod", TestStruct{Number: 123, Message: "hello"}, 3) }, "Unexpected call to", "doesn't match the argument at index 1", - "Got: 3 (int)\nWant: is equal to 15 (int)") + "Diff (-want +got):", "int(", "-", "15,", "+", "3,", ")") reporter.assertFatal(func() { // The expected call wasn't made. diff --git a/gomock/matchers.go b/gomock/matchers.go index d0590d0..c03e2cc 100644 --- a/gomock/matchers.go +++ b/gomock/matchers.go @@ -19,6 +19,8 @@ import ( "reflect" "regexp" "strings" + + "github.com/google/go-cmp/cmp" ) // A Matcher is a representation of a class of values. @@ -31,6 +33,11 @@ type Matcher interface { String() string } +type Differ interface { + // Diff shows the difference between the value and x. + Diff(x interface{}, opts ...cmp.Option) string +} + // WantFormatter modifies the given Matcher's String() method to the given // Stringer. This allows for control on how the "Want" is formatted when // printing . @@ -94,6 +101,10 @@ func (anyMatcher) Matches(any) bool { return true } +func (anyMatcher) Diff(interface{}) string { + return "" +} + func (anyMatcher) String() string { return "is anything" } @@ -132,6 +143,10 @@ func (e eqMatcher) Matches(x any) bool { return false } +func (e eqMatcher) Diff(x interface{}, opts ...cmp.Option) string { + return cmp.Diff(e.x, x, opts...) +} + func (e eqMatcher) String() string { return fmt.Sprintf("is equal to %s (%T)", getString(e.x), e.x) } @@ -153,6 +168,10 @@ func (nilMatcher) Matches(x any) bool { return false } +func (nilMatcher) Diff(x interface{}, opts ...cmp.Option) string { + return cmp.Diff(nil, x, opts...) +} + func (nilMatcher) String() string { return "is nil" } @@ -196,6 +215,10 @@ func (m assignableToTypeOfMatcher) Matches(x any) bool { return reflect.TypeOf(x).AssignableTo(m.targetType) } +func (m assignableToTypeOfMatcher) Diff(x interface{}, opts ...cmp.Option) string { + return cmp.Diff(m.targetType, reflect.TypeOf(x), opts...) +} + func (m assignableToTypeOfMatcher) String() string { return "is assignable to " + m.targetType.Name() } @@ -234,6 +257,18 @@ func (am allMatcher) Matches(x any) bool { return true } +func (am allMatcher) Diff(x interface{}, opts ...cmp.Option) string { + ss := make([]string, 0, len(am.matchers)) + for _, matcher := range am.matchers { + if d, ok := matcher.(Differ); ok { + ss = append(ss, d.Diff(x)) + } else { + ss = append(ss, matcher.String()) + } + } + return strings.Join(ss, "; ") +} + func (am allMatcher) String() string { ss := make([]string, 0, len(am.matchers)) for _, matcher := range am.matchers { @@ -256,6 +291,16 @@ func (m lenMatcher) Matches(x any) bool { } } +func (m lenMatcher) Diff(x interface{}, opts ...cmp.Option) string { + v := reflect.ValueOf(x) + switch v.Kind() { + case reflect.Array, reflect.Chan, reflect.Map, reflect.Slice, reflect.String: + return cmp.Diff(m.i, v.Len(), opts...) + default: + return cmp.Diff(m.i, fmt.Sprintf("invalid: len(%T)", x), opts...) + } +} + func (m lenMatcher) String() string { return fmt.Sprintf("has length %d", m.i) } @@ -310,6 +355,52 @@ func (m inAnyOrderMatcher) Matches(x any) bool { return extraInGiven == 0 && missingFromWanted == 0 } +func (m inAnyOrderMatcher) Diff(x interface{}, opts ...cmp.Option) string { + given, ok := m.prepareValue(x) + if !ok { + return cmp.Diff(m.x, x, opts...) + } + wanted, ok := m.prepareValue(m.x) + if !ok { + return cmp.Diff(m.x, x, opts...) + } + + if given.Len() != wanted.Len() { + return cmp.Diff(m.x, x, opts...) + } + + usedFromGiven := make([]bool, given.Len()) + foundFromWanted := make([]bool, wanted.Len()) + for i := 0; i < wanted.Len(); i++ { + wantedMatcher := Eq(wanted.Index(i).Interface()) + for j := 0; j < given.Len(); j++ { + if usedFromGiven[j] { + continue + } + if wantedMatcher.Matches(given.Index(j).Interface()) { + foundFromWanted[i] = true + usedFromGiven[j] = true + break + } + } + } + + missingFromWanted := 0 + for _, found := range foundFromWanted { + if !found { + missingFromWanted++ + } + } + extraInGiven := 0 + for _, used := range usedFromGiven { + if !used { + extraInGiven++ + } + } + + return cmp.Diff(m.x, x, opts...) +} + func (m inAnyOrderMatcher) prepareValue(x any) (reflect.Value, bool) { xValue := reflect.ValueOf(x) switch xValue.Kind() {