mock: Allow custom matcher functions to return error strings displayed on a failed match

[georgelesica-wf]

NOTE: This is a copy of #639 since that fork is now inaccessible. Review has taken place there.

Custom Matchers are super helpful, but the failure messages are less so. This PR allows those using MatchedBy to return a custom error response for a failed match to control the output on failure. Additionally, this cleans up the output for the closest call arguments.

For example, the current version of testify gives the following output for the following test

Test

type testMock struct {
	mock.Mock
}

func (m testMock) MyMethod(a *http.Request) string {
	args := m.Called(a)
	return args.String(0)
}

func TestUsingCustomMatcher(t *testing.T) {
	m := new(testMock)
	m.Test(t)
	m.On("MyMethod", mock.MatchedBy(func(r *http.Request) bool {
		return r.Host == "www.example.com"
	})).Return("hello")

	m.MyMethod(&http.Request{
		Host: "www.not-example.com",
	})
}

Output

=== RUN   TestUsingCustomMatcher
--- FAIL: TestUsingCustomMatcher (0.00s)
	mock.go:237: 
		
		mock: Unexpected Method Call
		-----------------------------
		
		MyMethod(*http.Request)
				0: &http.Request{Method:"", URL:(*url.URL)(nil), Proto:"", ProtoMajor:0, ProtoMinor:0, Header:http.Header(nil), Body:io.ReadCloser(nil), GetBody:(func() (io.ReadCloser, error))(nil), ContentLength:0, TransferEncoding:[]string(nil), Close:false, Host:"www.not-example.com", Form:url.Values(nil), PostForm:url.Values(nil), MultipartForm:(*multipart.Form)(nil), Trailer:http.Header(nil), RemoteAddr:"", RequestURI:"", TLS:(*tls.ConnectionState)(nil), Cancel:(<-chan struct {})(nil), Response:(*http.Response)(nil), ctx:context.Context(nil)}
		
		The closest call I have is: 
		
		MyMethod(mock.argumentMatcher)
				0: mock.argumentMatcher{fn:reflect.Value{typ:(*reflect.rtype)(0x13998a0), ptr:(unsafe.Pointer)(0x1443d78), flag:0x13}}
		
		
		Diff: 0: PASS:  (*http.Request=&{ <nil>  0 0 map[] <nil> <nil> 0 [] false www.not-example.com map[] map[] <nil> map[]   <nil> <nil> <nil> <nil>}) not matched by func() bool
FAIL

Process finished with exit code 1

This PR now allows for
Test

type testMock struct {
	mock.Mock
}

func (m testMock) MyMethod(a *http.Request) string {
	args := m.Called(a)
	return args.String(0)
}

func TestUsingCustomMatcher(t *testing.T) {
	m := new(testMock)
	m.Test(t)
	m.On("MyMethod", mock.MatchedBy(func(r *http.Request) error {
		if r.Host != "www.example.com" {
			return fmt.Errorf("request not against www.example.com")
		}
		return nil
	})).Return("hello")

	m.MyMethod(&http.Request{
		Host: "www.not-example.com",
	})
}

Output

=== RUN   TestUsingCustomMatcher
--- FAIL: TestUsingCustomMatcher (0.00s)
	mock.go:237: 
		
		mock: Unexpected Method Call
		-----------------------------
		
		MyMethod(*http.Request)
				0: &http.Request{Method:"", URL:(*url.URL)(nil), Proto:"", ProtoMajor:0, ProtoMinor:0, Header:http.Header(nil), Body:io.ReadCloser(nil), GetBody:(func() (io.ReadCloser, error))(nil), ContentLength:0, TransferEncoding:[]string(nil), Close:false, Host:"www.not-example.com", Form:url.Values(nil), PostForm:url.Values(nil), MultipartForm:(*multipart.Form)(nil), Trailer:http.Header(nil), RemoteAddr:"", RequestURI:"", TLS:(*tls.ConnectionState)(nil), Cancel:(<-chan struct {})(nil), Response:(*http.Response)(nil), ctx:context.Context(nil)}
		
		The closest call I have is: 
		
		MyMethod(mock.argumentMatcher)
				0: MatchedBy(func(*http.Request) error)
		
		
		Diff: 0: FAIL:  (*http.Request=&{ <nil>  0 0 map[] <nil> <nil> 0 [] false www.not-example.com map[] map[] <nil> map[]   <nil> <nil> <nil> <nil>}) request not against www.example.com
FAIL

Process finished with exit code 1

[boyan-soubachov]

Would it make the code simpler if we changed to if ... else if ... else if ... else here? Reason being that 'target-less' switch statements aren't obvious in their order of execution if there is more than 1 possible match.

[boyan-soubachov]

Won't the %s formatting directives have the same effect as calling .String() on the input arguments, by definition?

[boyan-soubachov]

Would the code be simpler if we extract this into its own method? Something like returnsBoolOrError()? We can then also add a few good unit tests focusing just on it.

[glesica]

This isn't a breaking change (at least I don't think so), so I'm going to bump it to next+1 so we can expedite the next release.

[ian-h-chamberlain]

Hi, is there any interest in reviving this PR since it's been a few years that it was delayed? I'd imagine there might be some changes needed to work with e.g. #1578 so would it be better to have it in place before those changes go in?

I tried to take a look at the conflicts and they don't seem too bad, and I could really benefit from having these changes so I could take a stab at fixing it up and addressing some comments if it seems worth merging.