mock: do not let a surplus matcher match a missing argument

[dylanpulver]

Summary

Arguments.Diff used the plain string "(Missing)" to stand for an argument that one side does not have, so a generic matcher in a surplus position matched an argument that was never passed and the expectation passed silently.

Changes

• The sentinel for a missing argument is now an unexported missingArgument type with no constructor, instead of the string "(Missing)". No expectation can hold a value equal to it, so it can no longer be matched by accident or written by hand.
• Arguments.Diff records a difference for a missing argument on either side before any matcher is consulted. When the call and the expectation disagree about how many arguments there are, no matcher can reconcile that, so there is nothing for a matcher to decide.
• MatchedBy functions are no longer invoked with the sentinel. Previously the user's function was called with the string "(Missing)" as though it were a real argument.
• Added tests covering each matcher in a surplus position, the literal string case, the reverse direction where the call passes more arguments than the expectation declares, and a test that the same matchers still match when the argument is present.

Motivation

This is a bug fix.

Because the sentinel was an ordinary string, every matcher that accepts a string or accepts anything at all was happy to match an argument that did not exist. Writing more arguments in On than the method actually takes is a mistake, and testify was reporting it as a pass. That is the worst direction for a testing library to fail in, since the test still goes green.

The following cases all matched a missing argument before this change:

• mock.Anything, because the default branch matches whenever the expectation is Anything without looking at the actual value.
• mock.AnythingOfType("string"), because reflect.TypeOf(actual).Name() on the sentinel returned string.
• mock.IsType(""), for the same reason.
• The literal string "(Missing)", because it compared equal to the sentinel.

Two further paths leaked the sentinel. mock.MatchedBy(func(s string) bool) had its function called with "(Missing)", which is what the reporter's example demonstrates. mock.FunctionalOptions() in a surplus position panicked with reflect: call of reflect.Value.Pointer on uint8 Value, because assertOpts walked the sentinel string as if it were a slice of options.

Making the sentinel a private type is what @brackendawson suggested in the issue. The guard in Diff is what actually fixes the matching, and the private type is what stops the sentinel from being confused with user data if it ever reaches a new code path.

Example usage

Using the example from #1211:

func (t *MyMock) Do() {
	t.Called()
}

func TestFoo(t *testing.T) {
	m := &MyMock{}
	m.Test(t)
	m.On("Do", mock.Anything, mock.AnythingOfType("string"), "(Missing)", mock.MatchedBy(func(s string) bool {
		t.Errorf("%q", s)
		return true
	})).Return()
	m.Do()
	m.AssertExpectations(t)
}

Do takes no arguments, so all four expected arguments are surplus. Before this change the mock reported nothing wrong. The only output came from the reporter's own matcher, which testify had called with the sentinel string:

=== RUN   TestFoo
    kata_test.go:21: "(Missing)"
    kata_test.go:21: "(Missing)"
--- FAIL: TestFoo (0.00s)

After this change the matcher is not called at all and the mock reports the mismatch:

=== RUN   TestFoo
    mock.go:361:
        mock: Unexpected Method Call
        -----------------------------

        Do()

        The closest call I have is:

        Do(string,mock.anythingOfTypeArgument,string,mock.argumentMatcher)
                0: "mock.Anything"
                1: "string"
                2: "(Missing)"
                3: mock.argumentMatcher{...}

        Provided 4 arguments, mocked for 0 arguments
        Diff: 0: FAIL:  (Missing) != (string=mock.Anything)
                1: FAIL:  (Missing) != (mock.anythingOfTypeArgument=string)
                2: FAIL:  (Missing) != (string=(Missing))
                3: FAIL:  (Missing) != (mock.argumentMatcher=func(string) bool)
--- FAIL: TestFoo (0.00s)

Effect on printed output

A missing argument still prints as (Missing). The missingArgument type has a String method that returns that text, and Diff formats it with the same FAIL: %s != %s line it already used for a surplus argument on the call side.

Messages that existed before are byte for byte the same. Test_Arguments_Diff_DifferentNumberOfArgs still asserts (string=extra) != (Missing) and was not modified. The only new output is for the cases that used to produce no output because they wrongly passed.

Compatibility

No exported API changes. go doc -all ./mock is identical before and after.

A test that currently passes only because a surplus matcher swallowed a missing argument will now fail. That is the point of the change, but it does mean someone who wrote the literal string "(Missing)" in an expectation to assert that an argument is absent will need to drop that argument instead. That usage was never documented.

Testing

go test -race ./... passes on all packages. .ci.gogenerate.sh, .ci.gofmt.sh, and .ci.govet.sh pass. Each new test was also run against master with only the test file applied, and each one fails there, so they are real regression tests rather than tests written to fit the new behavior.

Related issues

Fixes #1211

This also fixes #587, which reports the same root cause for mock.Anything specifically.

#588 is an earlier open attempt at #587 by @dnathe4th. It has been waiting on requested changes since 2018. It took a different route, dropping the sentinel entirely and appending a separate inconsistent function arity line, which meant changing the existing (string=extra) != (Missing) assertion. In review of that PR @ernesto-jimenez suggested keeping the loop as it is and adding actualMissing and expectedMissing booleans used by the normal checking. That is the shape taken here, which is why existing messages are unchanged. Happy to close this in favor of #588 if that one is revived instead.

[brackendawson]

Can you write a PR summary using our template please.