Disallow mock.Anything from satisfying missing function parameter

[dnathe4th]

Fixes #587

[devdinu]

One suggestion would be to move away from usage of the string, you could change the missing to var missingArgument = errors.New("(Missing)"), so we will be comparing on error values, and its easy to change output if required. like this function could return error (custom type) and based on it, the output should be formatted.

[devdinu]

You could add a test case for Arguments([]interface{}{}).Diff(Arguments([]interface{}{Anything})), expected is missing

[dnathe4th]

How about removing the missing string sentinel all together and returning a new error outright? I kept this new check inside the loop rather than before it in case there are additional errors the user should be informed about.

[devdinu]

@dnathe4th We should ideally do this as part of mock.On function, and we need to consider varargs, and have to be tested extensively, lets create an issue and raise PR separately. I would suggest just with previous commit along with test case would be good to close the PR.

[dnathe4th]

I replied on #587 I am not sure it is possible to do this check as part of mock.On because there is no way for a function on *mock.Mock to be aware of what struct has embedded it as far as I could tell.

Functionally these two should be the same, but if you like the previous commit better with the extra checks in the conditional at the end I can revert back to that. This version seems strictly better to me from a readability perspective since it has an explicit check on arg length.

From my digging through the existing tests and my understanding of the reflect package, it doesn't seem like varargs needs any special handling since tests like Test_Mock_On_WithMixedVariadicFunc pass on this PR, but maybe I am misunderstanding what is needed here.

[devdinu]

yep, checked varargs are passed as slice, so we're good.

lets just pull this out of loop, or print it only once when required. you could also add a test to assert the output for this case.

[dnathe4th]

Even though this is inside the loop it should only print once because I return early. I implemented it like this do the differences counter would still return the total number of errors against the mock, which seemed more correct than returning before the loop with a return value of differences=1

If you feel strongly I can pull it outside the loop.

[dnathe4th]

Hey @devdinu , where did we end up on this? Is this change only acceptable with the check before the loop?

[devdinu]

@dnathe4th The change looks good. @ernesto-jimenez could help in merging this.

[ernesto-jimenez]

Could we simplify the logic here?

If we are going to short-circuit the loop when it goes beyond the length of either of the arrays. Could we:

• Check for inconsistent arity before the loop
• Remove maxArg and iterate through len(args)
• Remove the length checks

The main issue is that we would miss wether the other arguments matched.

An alternative might be that we keep the loop as it is, add a couple of bool variables: actualMissing and expectedMissing, and use them on the normal checking.

[dnathe4th]

I submit for your review a slight modification to your suggestion @ernesto-jimenez, instead iterating to the shortest argument length, and then checking arity after the loop. This way the existing parameters can all still be compared. This also has the benefit of no longer needing many of those inner conditionals related to the "missing" strings.

[dnathe4th]

Hm that travis-CI failure looks unrelated to my change. Any recommendations there?

[dnathe4th]

Hey team, how can I move this forward?

[dolmen]

This seems still relevant as of 37a3cb7.