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

[brandonbodnar-wk]

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

[devdinu]

@brandonbodnar-wk Yep, its hard to debug this, and we need better error message. Also it returns unexported argumentMatcher which could be changed.
But this would be backward incompatible (breaking) change. @ernesto-jimenez Could we introduce a new func to do this change?

We can log the errors needed in the MatchedBy func with the help of testing.T we've.

[brandonbodnar-wk]

Thanks @devdinu. This is a breaking change, but as far as I can tell, it will only break for someone that was reliant on the existing argumentMatcher.Matches method as it's signature changes from Matches(argument interface{}) bool to Matches(argument interface{}) error. Other existing users of MatchedBy not directly using the argumentMatcher.Matches method but instead passing the return as a parameter to the Mock.On function should be fine without needing to make a code change as far as I can tell (as shown by the existing tests continuing to work).

However, if the maintainers feel this break is too much, I can break the functionality into a separate function from MatchedBy.

@ernesto-jimenez evaluated altering the return value of MatchedBy to return an interface in #333. Looks like that PR has staled out. I am happy to incorporate @ernesto-jimenez idea of the ArgumentMatcher from there here if that gets to the desired outcome of custom error messages for custom argument matchers. Though, that would also be a breaking change in the same manner/scope that this PR is now (i.e., most users should be fine, but those relying directly on argumentMatcher.Matches will break.

[brandonbodnar-wk]

I've added an additional unexported method and moved the error producing logic into that method. I've also changed the existing Matches method to use the output of that new method, but without any changes to the existing method's signature.

This should prevent this PR from introducing any breaking changes, which still allowing the new functionality.

[georgelesica-wf]

@brandonbodnar-wk do you still want to do this? Can you resolve merge conflicts?

[brandonbodnar-wk]

@georgelesica-wf Sorry for the delay. I've updated this PR with latest from master to address the merge conflict.

[georgelesica-wf]

Awesome, I've requested one more quick review and then I'll merge.

[benechols-wf]

+1

[georgelesica-wf]

Looks good, thanks!

[georgelesica-wf]

Switched to another PR since there were merge conflicts and this fork is no longer accessible. See #781