Repository: golang/mock
Pull request #97 (2017-08-03 18:09:43)

Return verbose errors why expected method call did not match
Hi. Thanks for gomock! I just started using it and thought it was not enough verbose when it comes to errors messages.

## The benefits
Before this PR, in my tests, I see an error: `no matching expected call`
```
GOROOT=/usr/local/go
GOPATH=/ide/work
/usr/local/go/bin/go test -c -i -o /tmp/TestHTTP_makeRequestUsingCustomClient_in_http_client_test_gogo 34-mock-http-client-with-gomock
/tmp/TestHTTP_makeRequestUsingCustomClient_in_http_client_test_gogo -test.v -test.run ^TestHTTP_makeRequestUsingCustomClient$
	controller.go:129: no matching expected call: *myhttp32.MockHTTPClientInterface.Do([0xc420074b00])
	controller.go:174: missing call(s) to *myhttp32.MockHTTPClientInterface.Do(is equal to &{GET some-urla HTTP/1.1 1 1 map[] <nil> <nil> 0 [] false  map[] map[] <nil> map[]   <nil> <nil> <nil> <nil>})
	controller.go:181: aborting test due to missing call(s)

Process finished with exit code 1
```
after this PR:
```
GOROOT=/usr/local/go
GOPATH=/ide/work
/usr/local/go/bin/go test -c -i -o /tmp/TestHTTP_makeRequestUsingCustomClient_in_http_client_test_gogo 34-mock-http-client-with-gomock
/tmp/TestHTTP_makeRequestUsingCustomClient_in_http_client_test_gogo -test.v -test.run ^TestHTTP_makeRequestUsingCustomClient$
	controller.go:132: no matching expected call: *myhttp32.MockHTTPClientInterface.Do([0xc42000af00]) /ide/work/src/34-mock-http-client-with-gomock/http_client.go:85
		
		The expected argument of index: 0 of this call: /ide/work/src/34-mock-http-client-with-gomock/http_client_test.go:68 did not match the actual argument.
		Actual argument: is equal to &{GET some-urla HTTP/1.1 1 1 map[] <nil> <nil> 0 [] false  map[] map[] <nil> map[]   <nil> <nil> <nil> <nil>}, expected: &{GET some-url HTTP/1.1 1 1 map[] <nil> <nil> 0 [] false  map[] map[] <nil> map[]   <nil> <nil> <nil> <nil>}
	controller.go:177: missing call(s) to *myhttp32.MockHTTPClientInterface.Do(is equal to &{GET some-urla HTTP/1.1 1 1 map[] <nil> <nil> 0 [] false  map[] map[] <nil> map[]   <nil> <nil> <nil> <nil>}) /ide/work/src/34-mock-http-client-with-gomock/http_client_test.go:68
	controller.go:184: aborting test due to missing call(s)

Process finished with exit code 1
```

## This PR contains
  1. The changes that https://github.com/golang/mock/pull/25 introduces. But it had conflicts in relation to [latest master](https://github.com/golang/mock/commit/13f360950a79f5864a972c786a10a50e44b69541) which I fixed.
  2. Changes to `FindMatch` method. It now also returns an error which explains why there was no call match. I added tests for each error explanation message kind.
  3. I removed the square brackets around call origin (path and line to method invocations), because this way, in [Gogland IDE](https://www.jetbrains.com/go/), they are displayed as clickable links.

## You may not like
   * the wording of error messages
   * the errors messages like :"Actual argument: is equal to 15, expected: 3".  It comes from [here](https://github.com/xmik/mock/blob/verbose-errors/gomock/controller_test.go#L302). It would be prettier if the error message was:  "Actual argument: 15, expected: 3". This is because in the [method returning this message](https://github.com/xmik/mock/blob/verbose-errors/gomock/call.go#L205), the `m` object is of type `Matcher`, and it has no public fields, so we can only use its `String()` method. I don't think it is greatly important.



Please voice any concerns and treat me as unexperienced in Go, as I've just recently started using it.


Patch:
diff --git a/gomock/call.go b/gomock/call.go
index cc8dfff..9f3ae9c 100644
--- a/gomock/call.go
+++ b/gomock/call.go
@@ -17,6 +17,7 @@ package gomock
 import (
 	"fmt"
 	"reflect"
+	"strconv"
 	"strings"
 )
 
@@ -29,6 +30,7 @@ type Call struct {
 	methodType reflect.Type  // the type of the method
 	args       []Matcher     // the args
 	rets       []interface{} // the return values (if any)
+	origin     string        // file and line number of call setup
 
 	preReqs []*Call // prerequisite calls
 
@@ -79,8 +81,8 @@ func (c *Call) Do(f interface{}) *Call {
 func (c *Call) Return(rets ...interface{}) *Call {
 	mt := c.methodType
 	if len(rets) != mt.NumOut() {
-		c.t.Fatalf("wrong number of arguments to Return for %T.%v: got %d, want %d",
-			c.receiver, c.method, len(rets), mt.NumOut())
+		c.t.Fatalf("wrong number of arguments to Return for %T.%v: got %d, want %d [%s]",
+			c.receiver, c.method, len(rets), mt.NumOut(), c.origin)
 	}
 	for i, ret := range rets {
 		if got, want := reflect.TypeOf(ret), mt.Out(i); got == want {
@@ -91,8 +93,8 @@ func (c *Call) Return(rets ...interface{}) *Call {
 			case reflect.Chan, reflect.Func, reflect.Interface, reflect.Map, reflect.Ptr, reflect.Slice:
 				// ok
 			default:
-				c.t.Fatalf("argument %d to Return for %T.%v is nil, but %v is not nillable",
-					i, c.receiver, c.method, want)
+				c.t.Fatalf("argument %d to Return for %T.%v is nil, but %v is not nillable [%s]",
+					i, c.receiver, c.method, want, c.origin)
 			}
 		} else if got.AssignableTo(want) {
 			// Assignable type relation. Make the assignment now so that the generated code
@@ -101,8 +103,8 @@ func (c *Call) Return(rets ...interface{}) *Call {
 			v.Set(reflect.ValueOf(ret))
 			rets[i] = v.Interface()
 		} else {
-			c.t.Fatalf("wrong type of argument %d to Return for %T.%v: %v is not assignable to %v",
-				i, c.receiver, c.method, got, want)
+			c.t.Fatalf("wrong type of argument %d to Return for %T.%v: %v is not assignable to %v [%s]",
+				i, c.receiver, c.method, got, want, c.origin)
 		}
 	}
 
@@ -125,7 +127,8 @@ func (c *Call) SetArg(n int, value interface{}) *Call {
 	// TODO: This will break on variadic methods.
 	// We will need to check those at invocation time.
 	if n < 0 || n >= mt.NumIn() {
-		c.t.Fatalf("SetArg(%d, ...) called for a method with %d args", n, mt.NumIn())
+		c.t.Fatalf("SetArg(%d, ...) called for a method with %d args [%s]",
+			n, mt.NumIn(), c.origin)
 	}
 	// Permit setting argument through an interface.
 	// In the interface case, we don't (nay, can't) check the type here.
@@ -134,12 +137,14 @@ func (c *Call) SetArg(n int, value interface{}) *Call {
 	case reflect.Ptr:
 		dt := at.Elem()
 		if vt := reflect.TypeOf(value); !vt.AssignableTo(dt) {
-			c.t.Fatalf("SetArg(%d, ...) argument is a %v, not assignable to %v", n, vt, dt)
+			c.t.Fatalf("SetArg(%d, ...) argument is a %v, not assignable to %v [%s]",
+				n, vt, dt, c.origin)
 		}
 	case reflect.Interface:
 		// nothing to do
 	default:
-		c.t.Fatalf("SetArg(%d, ...) referring to argument of non-pointer non-interface type %v", n, at)
+		c.t.Fatalf("SetArg(%d, ...) referring to argument of non-pointer non-interface type %v [%s]",
+			n, at, c.origin)
 	}
 	c.setArgs[n] = reflect.ValueOf(value)
 	return c
@@ -158,7 +163,7 @@ func (c *Call) isPreReq(other *Call) bool {
 // After declares that the call may only match after preReq has been exhausted.
 func (c *Call) After(preReq *Call) *Call {
 	if c == preReq {
-		c.t.Fatalf("A call isn't allowed to be it's own prerequisite")
+		c.t.Fatalf("A call isn't allowed to be its own prerequisite")
 	}
 	if preReq.isPreReq(c) {
 		c.t.Fatalf("Loop in call order: %v is a prerequisite to %v (possibly indirectly).", c, preReq)
@@ -168,7 +173,7 @@ func (c *Call) After(preReq *Call) *Call {
 	return c
 }
 
-// Returns true iff the minimum number of calls have been made.
+// Returns true if the minimum number of calls have been made.
 func (c *Call) satisfied() bool {
 	return c.numCalls >= c.minCalls
 }
@@ -184,31 +189,35 @@ func (c *Call) String() string {
 		args[i] = arg.String()
 	}
 	arguments := strings.Join(args, ", ")
-	return fmt.Sprintf("%T.%v(%s)", c.receiver, c.method, arguments)
+	return fmt.Sprintf("%T.%v(%s) %s", c.receiver, c.method, arguments, c.origin)
 }
 
 // Tests if the given call matches the expected call.
-func (c *Call) matches(args []interface{}) bool {
+// If yes, returns nil. If no, returns error with message explaining why it does not match.
+func (c *Call) matches(args []interface{}) error {
 	if len(args) != len(c.args) {
-		return false
+		return fmt.Errorf("Expected call at %s has the wrong number of arguments. Got: %s, want: %s",
+			c.origin, strconv.Itoa(len(args)), strconv.Itoa(len(c.args)))
 	}
 	for i, m := range c.args {
 		if !m.Matches(args[i]) {
-			return false
+			return fmt.Errorf("Expected call at %s doesn't match the argument at index %s.\nGot: %v\nWant: %v\n",
+				c.origin, strconv.Itoa(i), args[i], m)
 		}
 	}
 
 	// Check that all prerequisite calls have been satisfied.
 	for _, preReqCall := range c.preReqs {
 		if !preReqCall.satisfied() {
-			return false
+			return fmt.Errorf("Expected call at %s doesn't have a prerequisite call satisfied:\n%v\nshould be called before:\n%v",
+				c.origin, preReqCall, c)
 		}
 	}
 
-	return true
+	return nil
 }
 
-// dropPrereqs tells the expected Call to not re-check prerequite calls any
+// dropPrereqs tells the expected Call to not re-check prerequisite calls any
 // longer, and to return its current set.
 func (c *Call) dropPrereqs() (preReqs []*Call) {
 	preReqs = c.preReqs
diff --git a/gomock/callset.go b/gomock/callset.go
index 1b7de4c..a88a172 100644
--- a/gomock/callset.go
+++ b/gomock/callset.go
@@ -14,6 +14,11 @@
 
 package gomock
 
+import (
+	"errors"
+	"fmt"
+)
+
 // callSet represents a set of expected calls, indexed by receiver and method
 // name.
 type callSet map[interface{}]map[string][]*Call
@@ -47,30 +52,35 @@ func (cs callSet) Remove(call *Call) {
 	}
 }
 
-// FindMatch searches for a matching call. Returns nil if no call matched.
-func (cs callSet) FindMatch(receiver interface{}, method string, args []interface{}) *Call {
+// FindMatch searches for a matching call. Returns error with explanation message if no call matched.
+func (cs callSet) FindMatch(receiver interface{}, method string, args []interface{}) (*Call, error) {
 	methodMap, ok := cs[receiver]
 	if !ok {
-		return nil
+		return nil, errors.New("there are no expected method calls for that receiver")
 	}
 	calls, ok := methodMap[method]
 	if !ok {
-		return nil
+		return nil, fmt.Errorf("there are no expected calls of the method: %s for that receiver", method)
 	}
 
 	// Search through the unordered set of calls expected on a method on a
 	// receiver.
+	callsErrors := ""
 	for _, call := range calls {
 		// A call should not normally still be here if exhausted,
 		// but it can happen if, for instance, .Times(0) was used.
 		// Pretend the call doesn't match.
 		if call.exhausted() {
+			callsErrors += "\nThe call was exhausted."
 			continue
 		}
-		if call.matches(args) {
-			return call
+		err := call.matches(args)
+		if err != nil {
+			callsErrors += "\n" + err.Error()
+		} else {
+			return call, nil
 		}
 	}
 
-	return nil
+	return nil, fmt.Errorf(callsErrors)
 }
diff --git a/gomock/controller.go b/gomock/controller.go
index 6bff78d..d06dfbb 100644
--- a/gomock/controller.go
+++ b/gomock/controller.go
@@ -58,6 +58,7 @@ package gomock
 import (
 	"fmt"
 	"reflect"
+	"runtime"
 	"sync"
 )
 
@@ -114,7 +115,8 @@ func (ctrl *Controller) RecordCallWithMethodType(receiver interface{}, method st
 	ctrl.mu.Lock()
 	defer ctrl.mu.Unlock()
 
-	call := &Call{t: ctrl.t, receiver: receiver, method: method, methodType: methodType, args: margs, minCalls: 1, maxCalls: 1}
+	origin := callerInfo(2)
+	call := &Call{t: ctrl.t, receiver: receiver, method: method, methodType: methodType, args: margs, origin: origin, minCalls: 1, maxCalls: 1}
 
 	ctrl.expectedCalls.Add(call)
 	return call
@@ -124,9 +126,10 @@ func (ctrl *Controller) Call(receiver interface{}, method string, args ...interf
 	ctrl.mu.Lock()
 	defer ctrl.mu.Unlock()
 
-	expected := ctrl.expectedCalls.FindMatch(receiver, method, args)
-	if expected == nil {
-		ctrl.t.Fatalf("no matching expected call: %T.%v(%v)", receiver, method, args)
+	expected, err := ctrl.expectedCalls.FindMatch(receiver, method, args)
+	if err != nil {
+		origin := callerInfo(2)
+		ctrl.t.Fatalf("Unexpected call to %T.%v(%v) at %s because: %s", receiver, method, args, origin, err)
 	}
 
 	// Two things happen here:
@@ -181,3 +184,10 @@ func (ctrl *Controller) Finish() {
 		ctrl.t.Fatalf("aborting test due to missing call(s)")
 	}
 }
+
+func callerInfo(skip int) string {
+	if _, file, line, ok := runtime.Caller(skip + 1); ok {
+		return fmt.Sprintf("%s:%d", file, line)
+	}
+	return "unknown file"
+}


Test patch:
diff --git a/gomock/controller_test.go b/gomock/controller_test.go
index 57f7957..d4e3bdc 100644
--- a/gomock/controller_test.go
+++ b/gomock/controller_test.go
@@ -20,6 +20,7 @@ import (
 	"testing"
 
 	"github.com/golang/mock/gomock"
+	"strings"
 )
 
 type ErrorReporter struct {
@@ -53,7 +54,7 @@ func (e *ErrorReporter) assertFail(msg string) {
 }
 
 // Use to check that code triggers a fatal test failure.
-func (e *ErrorReporter) assertFatal(fn func()) {
+func (e *ErrorReporter) assertFatal(fn func(), expectedErrMsgs ...string) {
 	defer func() {
 		err := recover()
 		if err == nil {
@@ -66,6 +67,18 @@ func (e *ErrorReporter) assertFatal(fn func()) {
 			e.t.Error("Expected fatal failure, but got a", actual)
 		} else if token, ok := err.(*struct{}); ok && token == &e.fatalToken {
 			// This is okay - the panic is from Fatalf().
+			if expectedErrMsgs != nil {
+				// assert that the actual error message
+				// contains expectedErrMsgs
+
+				// 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) {
+						e.t.Errorf("Error message:\ngot: %q\nwant to contain: %q\n", actualErrMsg, expectedErrMsg)
+					}
+				}
+			}
 			return
 		} else {
 			// Some other panic.
@@ -119,6 +132,16 @@ func (s *Subject) BarMethod(arg string) int {
 	return 0
 }
 
+// A type purely for ActOnTestStructMethod
+type TestStruct struct {
+	Number  int
+	Message string
+}
+
+func (s *Subject) ActOnTestStructMethod(arg TestStruct, arg1 int) int {
+	return 0
+}
+
 func assertEqual(t *testing.T, expected interface{}, actual interface{}) {
 	if !reflect.DeepEqual(expected, actual) {
 		t.Errorf("Expected %+v, but got %+v", expected, actual)
@@ -140,6 +163,31 @@ func TestNoCalls(t *testing.T) {
 	reporter.assertPass("No calls expected or made.")
 }
 
+func TestNoRecordedCallsForAReceiver(t *testing.T) {
+	reporter, ctrl := createFixtures(t)
+	subject := new(Subject)
+
+	reporter.assertFatal(func() {
+		ctrl.Call(subject, "NotRecordedMethod", "argument")
+	}, "Unexpected call to", "there are no expected method calls for that receiver")
+	ctrl.Finish()
+}
+
+func TestNoRecordedMatchingMethodNameForAReceiver(t *testing.T) {
+	reporter, ctrl := createFixtures(t)
+	subject := new(Subject)
+
+	ctrl.RecordCall(subject, "FooMethod", "argument")
+	reporter.assertFatal(func() {
+		ctrl.Call(subject, "NotRecordedMethod", "argument")
+	}, "Unexpected call to", "there are no expected calls of the method: NotRecordedMethod for that receiver")
+	reporter.assertFatal(func() {
+		// The expected call wasn't made.
+		ctrl.Finish()
+	})
+}
+
+// This tests that a call with an arguments of some primitive type matches a recorded call.
 func TestExpectedMethodCall(t *testing.T) {
 	reporter, ctrl := createFixtures(t)
 	subject := new(Subject)
@@ -187,11 +235,68 @@ func TestUnexpectedArgCount(t *testing.T) {
 	reporter.assertFatal(func() {
 		// This call is made with the wrong number of arguments...
 		ctrl.Call(subject, "FooMethod", "argument", "extra_argument")
-	})
+	}, "Unexpected call to", "wrong number of arguments", "Got: 2, want: 1")
 	reporter.assertFatal(func() {
 		// ... so is this.
 		ctrl.Call(subject, "FooMethod")
+	}, "Unexpected call to", "wrong number of arguments", "Got: 0, want: 1")
+	reporter.assertFatal(func() {
+		// The expected call wasn't made.
+		ctrl.Finish()
+	})
+}
+
+// This tests that a call with complex arguments (a struct and some primitive type) matches a recorded call.
+func TestExpectedMethodCall_CustomStruct(t *testing.T) {
+	reporter, ctrl := createFixtures(t)
+	subject := new(Subject)
+
+	expectedArg0 := TestStruct{Number: 123, Message: "hello"}
+	ctrl.RecordCall(subject, "ActOnTestStructMethod", expectedArg0, 15)
+	ctrl.Call(subject, "ActOnTestStructMethod", expectedArg0, 15)
+
+	reporter.assertPass("Expected method call made.")
+}
+
+func TestUnexpectedArgValue_FirstArg(t *testing.T) {
+	reporter, ctrl := createFixtures(t)
+	defer reporter.recoverUnexpectedFatal()
+	subject := new(Subject)
+
+	expectedArg0 := TestStruct{Number: 123, Message: "hello"}
+	ctrl.RecordCall(subject, "ActOnTestStructMethod", expectedArg0, 15)
+
+	reporter.assertFatal(func() {
+		// 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}\nWant: is equal to {123 hello}")
+
+	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}\nWant: is equal to {123 hello}")
+
+	reporter.assertFatal(func() {
+		// The expected call wasn't made.
+		ctrl.Finish()
 	})
+}
+
+func TestUnexpectedArgValue_SecondtArg(t *testing.T) {
+	reporter, ctrl := createFixtures(t)
+	defer reporter.recoverUnexpectedFatal()
+	subject := new(Subject)
+
+	expectedArg0 := TestStruct{Number: 123, Message: "hello"}
+	ctrl.RecordCall(subject, "ActOnTestStructMethod", expectedArg0, 15)
+
+	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\nWant: is equal to 15")
+
 	reporter.assertFatal(func() {
 		// The expected call wasn't made.
 		ctrl.Finish()
@@ -393,11 +498,12 @@ func TestOrderedCallsInCorrect(t *testing.T) {
 
 	ctrl.Call(subjectOne, "FooMethod", "1")
 	reporter.assertFatal(func() {
+		// FooMethod(2) should be called before BarMethod(3)
 		ctrl.Call(subjectTwo, "BarMethod", "3")
-	})
+	}, "Unexpected call to", "Subject.BarMethod([3])", "doesn't have a prerequisite call satisfied")
 }
 
-// Test that calls that are prerequites to other calls but have maxCalls >
+// Test that calls that are prerequisites to other calls but have maxCalls >
 // minCalls are removed from the expected call set.
 func TestOrderedCallsWithPreReqMaxUnbounded(t *testing.T) {
 	reporter, ctrl, subjectOne, subjectTwo := commonTestOrderedCalls(t)