Confusing error message for nil argument in mock expectation

ratkins commented 1 year ago

Description

Mockery's error message when you forget (or don't know) to invoke the correct cast when EXPECT()ing a nil argument makes no sense and doesn't indicate what the resolution might be.

Mockery Version

v2.36.0

Golang Version

go version go1.21.3 darwin/arm64

Installation Method

[ ] Binary Distribution
[ ] Docker
[x] brew
[ ] go install
[ ] Other: [specify]

Steps to Reproduce

I'm mocking a function declared thus:

DoThing(operatorID string, classTypeID *string, input []TextInput) error

So the obvious way to express that I expect a nil for the second argument looks like this:

serviceAPI.EXPECT().DoThing("operator-id", nil, []serviceAPI.TextInput{
    {ReportID: reportID.String(), Text: "report-text"},
}).Return(nil)

Expected Behavior

I expect the above expectation to pass if the code does indeed pass a nil into DoThing()'s second argument.

Actual Behavior

Instead, it gives me a confusing error message:

  mock: Unexpected Method Call
  -----------------------------

  IngestTexts(string,*string,[]serviceAPI.TextInput)
        0: "operator-id"
        1: (*string)(nil)
        2: []serviceAPI.TextInput{serviceAPI.TextInput{ReportID:"530c93c2-7ec3-4477-8d5f-bbe39e298fdf", Text:"report-text"}}

  The closest call I have is: 

  IngestTexts(string,<nil>,[]serviceAPI.TextInput)
        0: "operator-id"
        1: <nil>
        2: []serviceAPI.TextInput{serviceAPI.TextInput{ReportID:"530c93c2-7ec3-4477-8d5f-bbe39e298fdf", Text:"report-text"}}

I read this as "a nil string in argument 2 is not the same as nil". Which is clearly ridiculous; furthermore, there's no indication what the problem might be or how to fix it.

I have a vague idea why this happens (something to do with go's very confused conception of what nil means) but as someone without a huge depth of experience with go, this error took me about forty five minutes to puzzle through and fix.

Is there any way for Mockery to detect this situation and emit a more useful error message?

LandonTClipp commented 1 year ago

The difference is coming from the type of nil you're using. The key is in the error message itself:

what you specified:

        1: <nil>

what it received:

        1: (*string)(nil)

This is an edge case of the expectation methods. The arguments in the type signature of the expectation methods are interface{} because of the need to possibly specify mock.Anything. So your nil pointer is essentially untyped (or more specifically, it's a nil pointer to interface{}) and you need to cast it into the appropriate type like you said.

(something to do with go's very confused conception of what nil means)
Which is clearly ridiculous; furthermore, there's no indication what the problem might be or how to fix it.

Well, nil becomes a typed value just like anything else in Go, contrasted to other languages where you can compare nil of different types. It's not ridiculous, this is a pretty consistent way to handle types.

Is there any way for Mockery to detect this situation and emit a more useful error message?

Not really, no. This particular message is coming from testify. Ultimately the issue happens because of a combination of shortcomings in the model that mockery mocks use (that being matching arguments to return values... not easy to do correctly), and a lack of "band-aids" we could apply (like making the arguments in the expectation methods strongly typed, which is a feature I've been wanting to add, but haven't yet because most people can work around it just fine).

I will say that the moq-style mocks we're folding into this project is a better model overall IMO, and you would not encounter this problem here. I don't want to drive you away from mockery but I'm just being honest with the current state of things.

ratkins commented 1 year ago

The difference is coming from the type of nil you're using.

As I said, I find "the type of nil" to be a very strange phrase; at the very least the original declaration specifies the function takes a pointer-to-string so that should be enough information for the compiler to infer what I meant without me having to specify the cast manually.

In any case, I understand the limitation is in the underlying model and I appreciate you taking the time to explain. Happy to close this now and leave it as a signpost for any others who are similarly confused.

vektra / mockery